Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is this a good approach of designing a class or is there some other way that I am not aware of?

Student.h specification file

#ifndef STUDENT_H
#define STUDENT_H

#include <string>
using namespace std;

class Student
{
    private:
        int ID;
        string name;
        double GPA; 
        char gender;
    public:
        Student();
        Student(int ID, string name, double GPA, char gender);
        void setStudent(int ID, string name, double GPA, char gender);
        int getID();
        string getName();
        double getGPA();
        char getGender();
        void print();
};
#endif

Student.cpp implementation file

#include "Student.h"
#include <iostream>
using namespace std;

Student :: Student()
{
    ID = 0;
    name = "";
    GPA = 0;
    gender = ' ';
}
Student :: Student(int ID, string name, double GPA, char gender)
{
    this -> ID = ID;
    this -> name = name;
    this -> GPA = GPA;
    this -> gender = gender;
}

void Student :: setStudent(int ID, string name, double GPA, char gender)
{
    this -> ID = ID;
    this -> name = name;
    this -> GPA = GPA;
    this -> gender = gender;
}

int Student ::  getID()
{
    return ID;
}

string Student :: getName()
{
    return name;
}

double Student :: getGPA()
{
    return GPA;
}

char Student ::  getGender()
{
    return gender;
}

void Student :: print()
{
    cout << "ID : " << ID << endl;
    cout << "Name : " << name << endl;
    cout << "GPA : " << GPA << endl;
    cout << "Gender : " << gender << endl;
}

StudentDemo.cpp

#include <iostream>
#include "Student.h"
using namespace std;

int main()
{
    Student s;
    int ID;
    string name;
    double GPA; 
    char gender;

    cout << "Enter ID ";
    cin >> ID;
    cout << "Enter name ";
    cin >> name;
    cout << "Enter GPA ";
    cin >> GPA;
    cout << "Enter gender ";
    cin >> gender;
    s.setStudent(ID, name, GPA, gender);
    s.print();
    return 0;
}
share|improve this question

3 Answers 3

Student.cpp

You should use initializer lists in place of these constructors. They offer some advantages, such as being able to initialize const members. Normal constructors do not allow that.

Initializer lists

The initializer list is in this form:

Class::Class() : member(value) /* additional members separated by commas */ {}

Here's what it'll look like with the overloaded constructor:

Student::Student(int ID, std::string const& name, double GPA, char gender)
    : ID(ID)
    , name(name)
    , GPA(GPA)
    , gender(gender)
{}

You should then apply the same idea to the other constructor.

Overloading operator<<

Consider overloading operator<< for Student so that it'll be easier to output.

Declare it in the header:

friend std::ostream& operator<<(std::ostream& out, Student const& obj);

Define it in the implementation:

std::ostream& operator<<(std::ostream& out, Student const& obj)
{
    out << "ID: " << obj.ID << "\n";
    out << "Name: " << obj.name << "\n";
    out << "GPA: " << obj.GPA << "\n";
    out << "Gender: " << obj.gender << "\n";

    return out;
}

Overloading operator>>

You can also do the same with operator>> to make inputting cleaner.

Declare it in the header:

friend std::istream& operator>>(std::istream& in, Student& obj);

Define it in the implementation:

std::istream& operator>>(std::istream& in, Student& obj)
{
    return in >> obj.ID >> obj.name >> obj.GPA >> obj.gender;
}

Regarding name: you should use std::getline() instead of std::cin >> so that spaces can be properly handled (it's also preferred in general for inputting into an std::string). However, it'll take additional tweaking as you cannot just mix both forms of input.

StudentDemp.cpp

You should construct the Student object instead of using a setter.

If you keep your user inputs as-is, this is how the construction should look:

Student s(ID, name, GPA, gender);

But if you decide to use operator>>, you just need the default Student:

Student s;

and your input will now look like this:

std::cout << "Enter student information (ID, name, GPA, gender):";
std::cin >> s;

With the latter, you should then remove the other variables from the top of the function.

Final notes:

  • Please do not use using namespace std in so many places. It can cause name-clashing issues, especially in larger programs. While it is okay to use it in a lower scope (such as a function), it's especially worse to use it in a header file. Any file including the header will be forced to use it, which could cause bugs. Read this for more information.

  • Declare variables as close in scope as possible to help ease maintainability. This is apparent with your variables declared at the top of main().

  • Avoid setters and getters as much as possible. They can break encapsulation as they expose the internals of the class. In this particular program, you won't even need them if you're just displaying values. Remove them all and it should make the code cleaner.

share|improve this answer
    
using namespace std in a header (or before including a header) is obviously problematic, but even Herb Sutter uses it in non-header files all the time so I strongly disagree with the first note. Apart from that using a char for gender is a bad design I'd say. –  Voo May 10 at 20:49
2  
@Voo: Then you need to read Why is “using namespace std;” considered bad practice?. Please provide an example of your speculation about Herb so we can comment on the situation (as all usage is dependent on the situation (in the general case it is a bad idea and the advice here holds)). –  Loki Astari May 10 at 20:57
    
@Voo: I do agree about the gender thing, and I haven't quite looked at that. I suppose an enum would be a better option? –  Jamal May 10 at 20:59
    
@Loki "You can and should use namespace using declarations and directives liberally in your implementation files after #include directives and feel good about it. Despite repeated assertions to the contrary, namespace using declarations and directives are not evil and they do not defeat the purpose of namespaces. Rather, they are what make namespaces usable." Item 59, in C++ Coding Standards. Also most of his example code you can find. I also distinctly remember STL using it in his coding during Going Native, etc. –  Voo May 10 at 21:38
    
I find an IDE with real time syntax checking makes the name clash argument largely passe. I've studied several examples and have yet to see a significant compiled size difference between code using namespace std, and code not using it. –  tinstaafl May 11 at 3:10

It's a fine starting point.

Overall Design

But if I was to classify it at the moment it really a property bag (a set of properties with no associated actions (thus not a class)). A class is a set of properties that have meaning together. The state of an object of that class is manipulated by actions that are associated with the class.

Getter/Setters

This is why I also hate Getters/Setters.

   int getID();
    string getName();
    double getGPA();
    char getGender();

They provide no intrinsic value and expose implementation details about your class. Sure there are sometimes reasons to have them but usually it is better to provide appropriate methods that manipulate the state of the class rather the just giving access to the members.

Extended Setter

I see no reason for a set method:

void Student :: setStudent(int ID, string name, double GPA, char gender)

What is the use case?

Student   loki(5, "Loki", 4.8 /* So much better than you would expect*/, 'A');


// Change the value of the variable?
loki.setStudent(8, "Deepak", 1.0, 'M');

Sounds like a resonable use case. But it has so many detractors (You have to manually set all the values). Why not just set if from another student object.

Student   loki(5, "Loki", 4.8 /* So much better than you would expect*/, 'A');

loki = Student(8, "Deepak", 1.0, 'M');  // Reset loki to a different student.

Default construction: Hmmmm

Student :: Student()
{
    ID = 0;
    name = "";
    GPA = 0;
    gender = ' ';
}

After construction an object is supposed to be in a valid state. But this does not look very valid to me. When you write other code you make assumptions about the objects you use. One of these assumptions is that the objects are in a valid state so you can just use them (you don't need to validate them because the object validated itself on construction and any members validate that mutations are correct).

What if I write a print function:

 void printGenScore(Student const& s1)
 {
      std::cout << "I am " << s1.getGender() << " - " << s1.getGPA() << "\n";
 }

If I use this with the default student:

 Student s1;
 printGenScoew(s1);

 // I get the output
 // I am  - 
 //

Does not look like a big problem. As a human I can read that as a blank user. But now you are assuming the text will be read by a human and not some other piece of code that expects the input to be in a very precise format as another group has written the code:

 void scoreByGenederBigDataParser(std::istream& f)
 {
     std::string  res;
     std::char    gender;
     float        score;

     f >> res >> res >> gender >> res >> score;
     data[gender] += score;
 }

Now all hell breaks loose as their assumptions have been broken.

Review of C++

Lets stop doing this.

using namespace std;

Its lazy and leads to bad habbits. There issues associated with bringing stuff into the current namespace. So don't do it. Look up why.

std::cout << "Hi\n"; // Its not that hard to add `std::` to stuff.

Prefer Initializer lists

Use the initializer list from the constructor. And don't bother with this-> its not very common in C++ circles.

Some people say if you use this-> it prevents errors. True; but then you are only relying on your eyes rather than the compiler to find errors.

If you turn the warning level of your compiler up and treat warnings as errors. Then you get even better error protection and you don't need the ugly this-> as it does not help you find errors anymore (as the compiler is doing it for you).

Student :: Student(int ID, string name, double GPA, char gender)
   : ID(ID)
   , name(name)
   , GPA(GPA)
   , gender(gender)
{}

Note Getters don't change the state. So make them const.

Student::print

Don't limit your printing to std::cout. Pass a stream that defaults to std::cout but allows the user to pass the stream type they want.

Don't use std::endl (prefer "\n") unless you want to flush the buffer. You usually don't want to flush the buffer. Flushing is an efficiency thing and the standard flush cycle is designed to make the stream very efficient. If you force a flush you will probably make the code slower (unless you know what you are doing). For I/O operations with the user std::cin and std::cout are synced. So if you ask a question the output is automatically flushed so they can read it before providing an answer.

print does not alter the state of the object. So it should be marked as const

class Student
{
    void print(std::ostream& str = std::cout) const
    {
        str << "ID : " << ID << "\n"
            << "Name : " << name << "\n"
            << "GPA : " << GPA << "\n"
            << "Gender : " << gender << "\n"
    }
};

Now you can also add the standard stream operator that most people in C++ use.

std::osteam& operator<<(std::ostream& str, Student const& data)
{
     data.print(str);
     return str;
}

Student   loki(5, "Loki", 4.8, 'A');
std::cout << loki;
share|improve this answer

Student.h specification file

Did you mean header?

  • You should not use using namespace std in header files: use std:: instead.
  • You don't need two constructors: you can use default parameters.
  • In function prototypes you could omit the names, because the compiler will ignore them.

Other things I'd change, IMHO:

  • Take advantage of class default access specification: remove the private specifier since they're already private.
  • If you allow the user to set data through setters but you don't check what he's actually putting, there's no difference if they were directly public.

Student.cpp

  • In print, you'd better to flush the buffer only once
  • Pass string as const &, if you aren't going to modify it.
  • Consider using the operator<< in order to make the output process sweeter. (you could just call print inside)

Blockquote StudentDemo.cpp

  • Where main resides, the file should be named main.cpp

If you follow all my advices, you'll get:

Student.h

#ifndef STUDENT_H
#define STUDENT_H

#include <string>
using std::string;

struct Student
{
        Student(int ID = 0, const string &name = " ", double GPA = 0.0f, char gender = ' ');
        void setStudent(int ID, string name, double GPA, char gender);
        void print();

    int ID;
    string name;
    double GPA; 
    char gender;

};
#endif

Student.cpp

#include "Student.h"
#include <iostream>
using namespace std;

Student :: Student(int ID , const string &name , double GPA, char gender )
{
    this -> ID = ID;
    this -> name = name;
    this -> GPA = GPA;
    this -> gender = gender;
}

void Student :: print()
{
    cout << "ID : " << ID << '\n'
     << "Name : " << name << '\n'
     << "GPA : " << GPA << '\n'
     << "Gender : " << gender << endl;
} 

main.cpp

#include <iostream>
#include "Student.h"
using namespace std;

int main()
{
    Student s;

    cout << "Enter ID ";
    cin >> s.ID;

    cout << "Enter name ";
    cin >> s.name;

    cout << "Enter GPA ";
    cin >> s.GPA;

    cout << "Enter gender ";
    cin >> s.gender;

    s.print();
    return 0;
}  
share|improve this answer
    
"In function prototypes you could omit the names, because the compiler will ignore them." If you have an IDE, the parameter names can be displayed when calling a function. It's not a replacement for a proper documentation, but helps at least to get the order right. –  dyp May 10 at 20:13

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.