Need comments on code

Hey guys. I've been working a bit on this piece of code lately and I wanted your opinion. This is part of a software I want to write that will store notes for each assignments of each courses as well as average and other informations. I know this is a service that is commonly offered to students by schools but I just wanted to my version and stuff it a little bit more than what my school offers.

This part of the code (which is the only part written yet by the way) is the part that deals with the classes and assignments. I have implemented only the search feature as well as adding courses and assignments. I wanted your opinion on what could be improved and if there are bugs that I haven't noticed yet. I compiled the code and it runs fine with a little bit of dummy code in main() to test and it seems to work as expected. Suggestions are welcome as well.

There's a bit of code that I don't really like in the TrouveTravaux() function. It checks if the current Course has been added to the list of courses that match the search criteria. I find it odd because I have to do it on each search criteria and it doesn't seem really elegant. Not that any of my code is *that* elegant either :P This is the part that I have colored in blue.

[code removed, see next post]

Sorry because my English is quite poor and all the code and comments are in French =P.

Anyway, all suggestions and comments are welcome.

I have coded some more and that part is getting more complete. Now there's a compiler error but I'll make another thread for that. Here's the new code (I removed the code from my other thread). I think it's *very* hack-ish in some ways and I can't even tell if it works for sure right now.

Code:

---

#include "school.h"



// returns false if it's already in the list
bool CourseeMgr::AddCourse(const Course& c)
{
        std::list<Course>::iterator it;
        for(it = CourseList.begin(); it != CourseList.end(); it++)
        {
                if(*it == c)
                        return false;
        }

        CourseList.push_back(c);
        return true;
}



bool CourseeMgr::AddCourse(std::string name, std::string num)
{
        return AddCourse(Course(name, num));
}



void CourseeMgr::AddAssignment(Course* ptr, const Assignment& trav)
{
        ptr->Assignments.push_back(trav);
}


       
void CourseeMgr::FindCourse(std::list<Course>& ls, const std::string str, unsigned int type)
{
        std::list<Course>::const_iterator it;
        for(it = CourseList.begin(); it != CourseList.end(); it++)
        {
                if(type & C_NAME)
                {
                        if(FindWithoutCase(str, it->Name))
                        {
                                ls.push_back(*it);
                                continue;
                        }
                }

                if(type & C_NUMBER)
                {
                        if(FindWithoutCase(str, it->Number))
                        {
                                ls.push_back(*it);
                                continue;
                        }
                }
        }
}



// returns the first course that matches the search criteria
Course* CourseeMgr::FindCourseUnique(std::string name)
{
        std::list<Course>::iterator it;
        for(it = CourseList.begin(); it != CourseList.end(); it++)
        {
                if(FindWithoutCase(name, it->Name))
                        return &(*it);
        }
        return 0;
}



void CourseeMgr::FindAssignments(std::list<Course>& ls_result, const std::string str, unsigned int type)
{
        // iterate through all courses
        std::list<Course>::iterator courses_it;
        for(courses_it = CourseList.begin(); courses_it != CourseList.end(); courses_it++)
        {
                std::list<Assignment>& ref = courses_it->Assignments;

                // iterate through all assignments
                std::list<Assignment>::iterator trav_it;
                for(trav_it = ref.begin(); trav_it != ref.end(); trav_it++)
                {
                        if(type & C_ASSIGNMENT_TITLE)
                        {
                                if(FindWithoutCase(str, trav_it->Title))
                                {
                                        ls_result.push_back(Course(courses_it->Name, courses_it->Number));
                                        ls_result.back().Assignments.push_back(*trav_it);
                                        continue;
                                }
                        }

                        //        POSSIBLE CASES FOR C_ASSIGNMENT_NOTE
                        //        - add assignment if greater than or equal to x                        ">=x"
                        //        - add assignment if lesser then or equal to x                        "<=x"
                        //        - add assignment if exactly equal to x                                        "x"
                        //        - add assignment if at +- 'y' of 'x'                                        "x+-y"
                        if(type & C_ASSIGNMENT_NOTE)
                        {
                                CMP_FUNC cmp_func = FindCmpFunc(str);

                                if(cmp_func != 0)
                                {
                                        if(cmp_func == NULL_FUNCTION)
                                        {
                                                int a = 0;
                                                int b = 0;

                                                EraseCharactersPM(a, b, str);
                                                if(ComparePPE(trav_it->Note, a + b) && ComparePGE(trav_it->Note, a - b))
                                                {
                                                        ls_result.push_back(Course(courses_it->Name, courses_it->Number));
                                                        ls_result.back().Assignments.push_back(*trav_it);
                                                        continue;
                                                }
                                                else continue;
                                        }
                                        else
                                        {
                                                int data = 0;
                                                EraseCharacters(data, str);
                                                if(cmp_func(trav_it->Note, data))
                                                {
                                                        ls_result.push_back(Course(courses_it->Name, courses_it->Number));
                                                        ls_result.back().Assignments.push_back(*trav_it);
                                                        continue;
                                                }
                                                else continue;
                                        }
                                }
                        }
                } // for(trav_it..
        } // for(courses_it..
}



CMP_FUNC CourseeMgr::FindCmpFunc(const std::string& criteria)
{
        CMP_FUNC func = 0;

        // check if ">=x"
        boost::regex e_pge("^>=\\b[0-9]{1,2}\\b$");
        if(boost::regex_match(criteria, e_pge))
                func = ComparePGE;

        // check if "<=x"
        boost::regex e_ppe("^<=\\b[0-9]{1,2}\\b$");
        if(boost::regex_match(criteria, e_ppe))
                func = ComparePPE;

        // check if "x"
        boost::regex e_exact("^\\b[0-9]{1,2}\\b$");
        if(boost::regex_match(criteria, e_exact))
                func = CompareExact;

        // check if "x+-y"
        boost::regex e_pm("^\\b[0-9]{1,2}\\b\\+-\\b[0-9]{1,2}\\b$");
        if(boost::regex_match(criteria, e_pm))
                func = NULL_FUNCTION;

        return func;
}



bool ComparePGE(int a, int b)
{
        return a >= b;
}



bool ComparePPE(int a, int b)
{
        return a <= b;
}



bool CompareExact(int a, int b)
{
        return a == b;
}
       


bool NULL_FUNCTION(int, int)
{
        return 0;
}


       
void CourseeMgr::EraseCharacters(int& data, const std::string& criteria)
{
        std::string::const_iterator it;
        for(it = criteria.begin(); it != criteria.end(); it++)
                if(isdigit(*it))
                        break;

        std::string clean;
        clean.append(it, criteria.end());

        std::stringstream ss;
        ss << clean;
        ss >> data;
}



// HACK !!
// Particular case ( x+-y )
void CourseeMgr::EraseCharactersPM(int& x, int& y, const std::string& criteria)
{
        std::string::const_iterator it;
        for(it = criteria.begin(); it != criteria.end(); it++)
                if(*it == '+')
                        break;

        std::string clean;
        clean.append(criteria.begin(), it);

        std::stringstream ss;
        ss << clean;
        ss >> x;

        clean.clear();
        clean.append(it + 2, criteria.end());

        ss.clear();
        ss << clean;
        ss >> y;
}



// Get rid of the dot which will mess with the regex query
void CourseeMgr::RemoveRegexCh(std::string& query)
{
        const char* backslash = "\\";

        for(int i = 0; i < query.size(); i++)
        {
                if(query[i] == '.')
                        query.insert(i++, backslash);
        }
}



// returns true if 'research' is found inside element without
// consideration to case
bool CourseeMgr::FindWithoutCase(std::string research, std::string element)
{
        RemoveRegexCh(research);

        std::stringstream tmp;
        tmp << ".*" << research << ".*";
        boost::regex e(tmp.str(), boost::regex::icase);
        return boost::regex_match(element, e, boost::match_default);
}

---

The red code is what I consider totally hack-ish.

Edit: Removed a lot of code.
Edit: Translated.

I need to brush up on my French.

OMG! even the variable names are written in french! lol

The layout looks good from what I can tell, but would it possible to at least translate the comments into english? It would better than having to translate the entire program. In that case, unstanding it would be a little easier.

I translated all of it (variable names, comments, etc.). If something sounds really really weird, it's because I have used a lot the Search & Replace tool. Doing that manually would have taken me all day :P

It seems to me that you have the classes Course and Assignment, but do not make use of them very much. For example, in CourseeMgr::FindAssignments(), I would think that you should have a Course::FindAssignments() function, and then CourseeMgr::FindAssignments() would iterate over all the courses being managed and forward the task of actually finding the assignment to Course::FindAssignments(). Course::FindAssignments() in turn might iterate over the assignments for that course, and use some function provided by Assignment to do its job.

On this note consider say, CourseeMgr::EraseCharactersPM(). It seems to me that this is an implementation detail of Assignment, and is thus completely out of place in CourseeMgr. So in my opinion, you might want to reorganise your classes a little.

Well that is what I wanted to do in the first place but I didn't really know how I'd do it.. Humm... I'll check it out. Maybe I can rearrange all of this.

Quote:

---

Well that is what I wanted to do in the first place but I didn't really know how I'd do it.. Humm... I'll check it out. Maybe I can rearrange all of this.

---

Well, here's an idea of some restructuring that might be useful:

Code:

---

bool Assignment::findCaseInsensitive(const std::string& title)
{
    // return true if title matches assignment's title
}

void CourseeMgr::findAssignmentsByTitle(std::list<Course>& result, const std::string& title) const
{
    for (std::list<Course>::const_iterator i = CourseList.begin();
        i != CourseList.end(); ++i)
    {
        i->findAssignmentsByTitle(result, title);
    }
}

// returns a course with the same name and number
// but which only has assigments matching the title provided
void Course::findAssignmentsByTitle(std::list<Course>& result, const std::string& title) const
{
    // have a constructor to copy the name and number of a course.
    // make it private, if necessary.
    Course temp(Name, Number);

    // find all the assignments that match in this course
    for (std::list<Assignment>::const_iterator i = Assignments.begin();
        i != Assignments.end(); ++i)
    {
        if (i->findCaseInsensitive(title))
        {
            temp.addAssignment(*i);
        }
    }

    if (temp.assignmentCount() > 0)
    {
        result.push_back(temp);
    }
}

---

One idea I have here is to separate the finding of assignments by title and the finding of assignments by note. Sure, now you will have two functions instead of one, but then you no longer have to rely on C_ASSIGNMENT_TITLE and C_ASSIGNMENT_NOTE to determine what to do.

Instead of using CMP_FUNC, why not use std::binary_function?

Don't unnecessarily use postincrement for iterators. Prefer preincrement because it is more eficient. e.g

Code:

---

        for(it = CourseList.begin(); it != CourseList.end(); ++it)

---

You seem to be using the 'continue' keyword in a lot of places where it has no effect. 'continue' is used to skip executing any more code in the loop and proceed to the next iteration. However in your case there is no more code to be executed after any of the last four places you used it. It would be clearer to remove those ones.

Try to be consistent with your passing of strings to functions. Your functions take strings as std::string, const std::string, and const std::string &. Whenever possible, use the last one (const reference).

Oh and one last thing about functions that take pointers as arguments. e.g.:

Code:

---

void CourseeMgr::AddAssignment(Course* ptr, const Assignment& trav)
{
        ptr->Assignments.push_back(trav);
}

---

If you use a pointer then my own personal rule is that it must be valid to pass NULL. In your case you would have to check for it to not crash.
If you expect the parameter to never be NULL, then pass by reference instead. Or, in cases where you had used a pointer because it takes an array, use [] in the declaration instead.

I think that instead of passing a pointer, it makes more sense to think of adding an assignment to a course under management. So that function should take some course id (maybe a name/number combination) and the assignment, find the course, and add the assignment. The details of adding an assignment can be abstracted away in a function in the Course class, which in turn may rely on some function(s) in the Assignment class.