Thread: 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;
}

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.

Thanks Salem. Points noted. Should of thought about the allocation thing better. Il edit my code and make the changes. Thanks for being cruel!

Code:

Base::Base()
{
   string tempSchoolName = "";
   schoolName = tempSchoolName;
   unsigned short tempStudentAmount = 0;
   studentAmount = tempStudentAmount;
}

Why so many code here?

Code:

Base::Base():
schoolName (""),
studentAmount (0)
{
}

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.

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...

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.

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!

'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.

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.