Please tear this apart

This is a discussion on Please tear this apart within the C++ Programming forums, part of the General Programming Boards category; Hi, I have just finished this large program that reads information about a person and prints the results to a ...

  1. #1
    Its hard... But im here swgh's Avatar
    Join Date
    Apr 2005
    Location
    England
    Posts
    1,475

    Please tear this apart

    Hi, I have just finished this large program that reads information about a person and prints the results to a txt file for output. I was wondering if there is anything I can do to make it look better, ie: more comments, better use of memory allcoation ect. The code does compile and I do get the desired output I wanted. Please be cruel One thing i should of done is placed the classes into sepeate files to increase program readability.

    Code:
    // student database
    // by peter watts
    
    #include <iostream>
    using std::cout;
    using std::endl;
    using std::cin;
    using std::cerr;
    using std::fixed;
    #include <string>
    using std::string;
    using std::getline;
    #include <iomanip>
    using std::setprecision;
    using std::setw;
    #include <fstream>
    using std::ofstream;
    using std::ifstream;
    
    // base class holds shared data for all classes
    class Base
    {
    public:
       Base();
       ~Base();
       
    protected:
       string schoolName;
       unsigned short studentAmount;
    };
    
    // set member values to zero
    Base::Base()
    {
       string tempSchoolName = "";
       schoolName = tempSchoolName;
       unsigned short tempStudentAmount = 0;
       studentAmount = tempStudentAmount;
    }
    
    // destructor does nothing
    Base::~Base()
    {
       cout << "\nBase class destructor called...\n";
    }
    
    // student class holds data regarding individual student
    class Student : public Base
    {
    public:
       Student();
       ~Student();
       
       string getStudentName() const; 
       string getAddressLine1() const;
       string getAddressLine2() const;
       string getAddressLine3() const;
       string getPostcode() const;
       unsigned short getAge() const;
       
       // shared data acsessor functions
       string getSchoolName() const;
       unsigned short getStudentAmount() const;
       
       void setStudentName ( string );
       void setAddressLine1 ( string );
       void setAddressLine2 ( string );
       void setAddressLine3 ( string );
       void setPostcode ( string );
       void setAge ( unsigned short );
       
       // shared data acsessor functions
       void setSchoolName ( string );
       void setStudentAmount ( unsigned short );
       
       void printReport ( string, string, string, string, string, unsigned short, string, unsigned short );
       
    private:
       string studentName;
       string addressLine1;
       string addressLine2;
       string addressLine3;
       string postcode;
       unsigned short *age;
    };
    
    // assign all data members to zero
    Student::Student()
    {
       age = new unsigned short(0); // memory on heap space
       
       string tempStudentName = "";
       studentName = tempStudentName;
       string tempAddressLine1 = "";
       addressLine1 = tempAddressLine1;
       string tempAddressLine2 = "";
       addressLine2 = tempAddressLine2;
       string tempAddressLine3 = "";
       addressLine3 = tempAddressLine3;
       string tempPostcode = "";
       postcode = tempPostcode;
    }
    
    // destroy age pointer
    Student::~Student()
    {
       delete age;
       age = NULL;
    }
    
    ////////////////////// student class return functions //////////////////////////
    string Student::getStudentName() const
    {
       return studentName;
    }
     
    string Student::getAddressLine1() const
    {
       return addressLine1;
    }
    
    string Student::getAddressLine2() const
    {
       return addressLine2;
    }
    
    string Student::getAddressLine3() const
    {
       return addressLine3;
    }
    
    string Student::getPostcode() const  
    {
       return postcode;
    }
    
    unsigned short Student::getAge() const
    {
       return *age;
    }
    
    string Student::getSchoolName() const
    {
       return schoolName;
    }
    
    unsigned short Student::getStudentAmount() const
    {
       return studentAmount;
    }
    
    ///////////////////////// student class setter functions ///////////////////////
    void Student::setStudentName ( string tempStudentName )
    {
       cout << "\nEnter student full name: ";
       getline ( cin, tempStudentName );
       
       studentName = tempStudentName;
       
       setAddressLine1("");
       
       return;
    }
    
    void Student::setAddressLine1 ( string tempAddressLine1 )
    {
       cout << "\nEnter first address line: ";
       getline ( cin, tempAddressLine1 );
       
       addressLine1 = tempAddressLine1;
       
       setAddressLine2("");
       return;
    }
    
    void Student::setAddressLine2 ( string tempAddressLine2 )
    {
       cout << "\nEnter second address line: ";
       getline ( cin, tempAddressLine2 );
       
       addressLine2 = tempAddressLine2;
       
       setAddressLine3("");
       
       return;
    }
    
    void Student::setAddressLine3 ( string tempAddressLine3 )
    {
       cout << "\nEnter third address line: ";
       getline ( cin, tempAddressLine3 );
       
       addressLine3 = tempAddressLine3;
       
       setPostcode("");
       
       return;
    }
    
    void Student::setPostcode ( string tempPostcode )
    {
       cout << "\nEnter postcode: ";
       getline ( cin, tempPostcode );
       
       postcode = tempPostcode;
       
       setAge(0);
       
       return;
    }
    
    void Student::setAge ( unsigned short m_age )
    {
       cout << "\nEnter student age: ";
       cin >> m_age;
       
       *age = m_age;
       
       cin.ignore();
       setSchoolName("");
       
       return;
    }
    
    void Student::setSchoolName ( string tempSchoolName )
    {
       cout << "\nEnter name of college: ";
       getline ( cin, tempSchoolName );
       
       schoolName = tempSchoolName;
       
       setStudentAmount(0);
       
       return;
    }
    
    void Student::setStudentAmount ( unsigned short tempStudentAmount )
    {
       cout << "\nEnter total student amount: ";
       cin >> tempStudentAmount;
       
       studentAmount = tempStudentAmount;
       
       printReport("","","","","",0,"",0);
       
       return;
    }
    
    // on the cboard editor, the printFunction  parameters are missing some details, but they are
    // all included in the orgigial program
    
    void Student::printReport ( string tempStudentName, string tempAddressLine1,
                                               string tempAddressLine2, string tempAdressLine3,
                                               string tempPostcode, unsigned short m_age,
                                               string tempSchoolName, unsigned short tempStudentAmount )
    {
       ofstream OUT_FILE ("Student.txt");
       OUT_FILE<<getStudentName() << endl
              <<getAddressLine1() << endl
       	  <<getAddressLine2() << endl
       	  <<getAddressLine3() << endl
       	  <<getPostcode() << endl
       	  <<getAge() << endl
       	  <<getSchoolName() << endl
       	  <<getStudentAmount() << endl;
       
       return;
    }
    
    // main - driver ///////////////////////////////////////////////////////////////
    int main ( void )
    {
       int choice = 0;
       
       Student *stu = new Student;
       
       // loop to detect correct input
       while (( choice >= 0 ) || ( choice <= 3 ))
       { 
          cout << "\n\tSTUDENT DATABASE\n\n"
                  << "1 Add a student to the database\n"
                  << "2 Print final report\n"
                  << "3 Quit\n"
                  << "> ";
          cin >> choice;
          
          switch ( choice )
          {
          case 1:
             cin.ignore(); // ignore previous input
             stu->setStudentName("");
             break;
             
          case 2:
             break;
             
          case 3:
             return 0;
             break;
    	   
          // don't really use this
          default:
             break;
          }
       }
       
       // ensure heap pointer is properly
       // deleted on program termination
       delete stu;
       stu = NULL;
       
       return 0;
    }
    Last edited by Salem; 12-31-2006 at 04:58 AM. Reason: wrap long code lines
    I'm just trying to be a better person - My Name Is Earl

  2. #2
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,420
    1. Your class isn't adding any value if all it contains is just set/get for every single member variable of the class.

    2. Why are you using parameters to declare local variables?
    If you used locals, then you wouldn't have calls like this
    printReport("","","","","",0,"",0);

    3. Why is age even allocated?
    unsigned short *age;
    should be
    unsigned short age;

    4. getAddressLine1 2 3
    What about an array here, or a vector?
    Or maybe even a better abstraction to cover many more possibilities.

    5. cin >> choice;
    Use getline() to read a whole line into a string, then use a stringstream to extract from that line. Then you don't have to worry about all that cin.ignore() hackery.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  3. #3
    Its hard... But im here swgh's Avatar
    Join Date
    Apr 2005
    Location
    England
    Posts
    1,475
    Thanks Salem. Points noted. Should of thought about the allocation thing better. Il edit my code and make the changes. Thanks for being cruel!
    I'm just trying to be a better person - My Name Is Earl

  4. #4
    CSharpener vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,456
    Code:
    Base::Base()
    {
       string tempSchoolName = "";
       schoolName = tempSchoolName;
       unsigned short tempStudentAmount = 0;
       studentAmount = tempStudentAmount;
    }
    Why so many code here?
    Code:
    Base::Base():
    schoolName (""),
    studentAmount (0)
    {
    }
    The first 90% of a project takes 90% of the time,
    the last 10% takes the other 90% of the time.

  5. #5
    Its hard... But im here swgh's Avatar
    Join Date
    Apr 2005
    Location
    England
    Posts
    1,475
    Good point Vart, I just always set constructors up the "old fasioned way" I guess. Perhaps one of my biggest faults in programming like you suggested is pl;acing too much code to do a simple thing. Cutting down on my function lengths and using the shorter verisons of constructor implementation can only reap bennifts in the future, espeacially for larger programs.
    I'm just trying to be a better person - My Name Is Earl

  6. #6
    CSharpener vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,456
    It not just a code length it also a number of oparations
    instead of one copy constructor you call 1 default constructor and two operators =...
    For complex classes it can be very valueable, till you use a good optimisation compiler...
    The first 90% of a project takes 90% of the time,
    the last 10% takes the other 90% of the time.

  7. #7
    Its hard... But im here swgh's Avatar
    Join Date
    Apr 2005
    Location
    England
    Posts
    1,475
    Good points vart. Thanks again. Quick question, would you call MINGW compiler a good one for optimisation? I do use DevC++ 4.992. But I also have MSVC++2003.net which I use for large projects. I suppose the reposabilty is really down to the programmer though, not the compiler.
    I'm just trying to be a better person - My Name Is Earl

  8. #8
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,288
    Okay, you asked for it... I hope this is about what you were looking for...

    Get rid of all those "tempBlah" statements in your constructors. Just assign things directly, and use initialiser lists while you're at it.

    Don't pass in lots of parameters to printReport if you don't need them. I mean don't even put them in the function definition. Don't pass in parameters to other functions either if you aren't going to use the values.

    You don't need a return statement at the end of a void function. Get rid of them!
    You don't need void within the brackets in int main() either.

    OUT_FILE is a bad name for a variable, by usual convention all capitals is used for Macros and constants etc. try using just outFile.

    Don't null stu after deleting it, your program is ending anyway! Don't set age to NULL in the destructor either as it is just as pointless.

    Why use dynamically allocated memory for age? The pointer is bigger than the data you are allocating. This is crazy!

    In your switch statement, return 0 followed by break is redundant. Just remove the break.

    Your set functions have unexpected side effects of clearing the "next" member variable. This is bad as it means you can no longer use the functions individually. In which case, it would make sense to combine them.

    Passing in strings to your setBlah functions should be by const reference. But really you shouldn't have lots of getters and setters.

    Using statements should be used sparingly. I have never seen anyone used them so much. Consider cutting them down a lot by using explicit qualification.

    The main menu is misleading as you can never actually store more than one student details.


    On the plus side: Well done for some const correctness!
    Last edited by iMalc; 12-31-2006 at 08:56 PM.

  9. #9
    Registered User
    Join Date
    Feb 2006
    Posts
    312
    These are more design issues than programming issues, however you may want to think about them -

    What if school data changes, and you have 100's of students from the same school..?
    and what if a student attended more than one school?

    School data isn't really an attribute of the student IMHO - the problem is that if any information about the school changes, currently you'd need to alter that data for every single student. Aside from which, you have, potentially, alot of unnecessarily duplicated data (redundancy) - or worse, two different students of the same school with different school data to their name!

    Depending on the kind of queries & functions the database is going to perform, You may consider a totally seperate 'school' class, and provide each student class with a pointer to their respective schools. If you don't like pointers, the alternative is to provide each school & student with an ID number, and use these to cross-reference them - similar to the idea of Primary/Foreign keys as used in SSADM (If you're not sure about SSADM, think of the way MS-Access handles database relationships).

  10. #10
    dac
    dac is offline
    Registered User
    Join Date
    Jan 2006
    Location
    North Yorkshire, England
    Posts
    147
    i am just curious but, how big is a large program to you guys?

    edit: in lines,

  11. #11
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,623
    i am just curious but, how big is a large program to you guys?

    edit: in lines,
    I think something like that depends on your expertise. If a project seems large, it probably is.

  12. #12
    Registered User
    Join Date
    May 2006
    Posts
    903
    'Large' depends on what you are used to do. For some, a program starts being large at 10,000+ lines. My biggest project was 1,000 lines so I'd say large for me would be 1,000+ lines. It's relative.

    About the code, the first thing that comes to mind is the long list of using std:: ... whatever. I personally prefer writing the std:: prefix in front of anything I use out of the std namespace so I don't mess with the global namespace by using 'using' declarations. Oh and unless you are going to derive other classes from Base, the class Base is absolutely useless since only Student is derived from it. If you do derive other classes from Base, then I suggest you to make Base a pure virtual class. Otherwise, get rid of it.

    Edit: Oh and get rid of all those parameters in printReport().. and why are you calling GET methods for variables that are directly accessible ? I mean, instead of getStudentName() you can simply access the studentName variable.
    Last edited by Desolation; 01-01-2007 at 11:45 AM.

  13. #13
    Its hard... But im here swgh's Avatar
    Join Date
    Apr 2005
    Location
    England
    Posts
    1,475
    Great advise guys, Il look into all the pointers ( bad pun ) you gave me. Thanks for looking over it, and thank you for being so cruel. I know I have design issues that I am slowly working on. It will take me a long time to become an C++ expert. So anything I can work on now in the short term will make me a better coder in a few years. Even the proffesionals had simple code ripped apart when they started out.
    I'm just trying to be a better person - My Name Is Earl

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. libcurl wrapper (tear it to shreds)
    By Wraithan in forum C++ Programming
    Replies: 8
    Last Post: 03-13-2008, 08:41 PM
  2. Unit Actions
    By DavidP in forum Game Programming
    Replies: 20
    Last Post: 05-28-2004, 09:18 PM

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