Need comments on code

This is a discussion on Need comments on code within the C++ Programming forums, part of the General Programming Boards category; Hey guys. I've been working a bit on this piece of code lately and I wanted your opinion. This is ...

  1. #1
    Registered User
    Join Date
    May 2006
    Posts
    903

    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.
    Last edited by Desolation; 01-01-2007 at 10:42 PM.

  2. #2
    Registered User
    Join Date
    May 2006
    Posts
    903
    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.
    Last edited by Desolation; 01-02-2007 at 10:45 AM.

  3. #3
    Kiss the monkey. CodeMonkey's Avatar
    Join Date
    Sep 2001
    Posts
    907
    I need to brush up on my French.
    "If you tell the truth, you don't have to remember anything"
    -Mark Twain

  4. #4
    dac
    dac is offline
    Registered User
    Join Date
    Jan 2006
    Location
    North Yorkshire, England
    Posts
    147
    OMG! even the variable names are written in french! lol

  5. #5
    Its hard... But im here swgh's Avatar
    Join Date
    Apr 2005
    Location
    England
    Posts
    1,479
    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'm just trying to be a better person - My Name Is Earl

  6. #6
    Registered User
    Join Date
    May 2006
    Posts
    903
    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

  7. #7
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,936
    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.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  8. #8
    Registered User
    Join Date
    May 2006
    Posts
    903
    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.

  9. #9
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,936
    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?
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #10
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    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.

  11. #11
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,936
    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.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 23
    Last Post: 04-20-2009, 07:35 AM
  2. Replies: 1
    Last Post: 03-21-2006, 06:52 AM
  3. Seems like correct code, but results are not right...
    By OmniMirror in forum C Programming
    Replies: 4
    Last Post: 02-13-2003, 12:33 PM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. code and comments check
    By Unregistered in forum C Programming
    Replies: 1
    Last Post: 01-17-2002, 11:45 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21