what changes you would make?

Hi :)

I have the compiled and run the code given below. I'm interested to know what changes you would like to make like changes in syntax, formatting of the code, etc? Please let me know.

Code:

---

// student_data_read_practice.cpp
// program which reads data of some students

#include <iostream>
#include <cstdlib>
#include <string>
#include <cstring>

using namespace std;

/////////////////////////////////////////////////////
struct Date {int d; string m; int y;};
////////////////////////////////////////////////////

/////////////////////////////////////////////////////
struct Student {int rollno; string sex; string name; float gpa; Date DOB;};
/////////////////////////////////////////////////////

Student read();
void prnt(Student dummystud);

int main()
{
    Student stud[5];

        for(int i=0; i<5; i++)
        {
            cout << "enter student #" << (i+1) << "'s details below:-" << endl;
                stud[i] = read();
                prnt(stud[i]);
                cout << endl << endl;
        }

        system("pause");
        return 0;
}

//-----------------------------------------------
// Student read() definition

Student read()
{
    Student stud; string name;

    cout << "enter name: "; getline(cin, name);
        cout << "enter roll number: "; cin >> stud.rollno;
        cout << "enter sex: "; cin >> stud.sex;
        cout << "enter date of birth (e.g. 01 Jan 2000) below:-" << endl;
        cout << "enter day: "; cin >> stud.DOB.d;
        cout << "enter month: "; cin >> stud.DOB.m;
        cout << "enter year: "; cin >> stud.DOB.y;
        cout << "enter GPA: "; cin >> stud.gpa;
        cin.ignore();

        return stud;
}

//-------------------------------------------------
// void prnt() definition

void prnt (Student dummystud)
{
    cout << "\n\n\n************************************\n\n\n";
        cout << "roll no.: " << dummystud.rollno << endl;
        cout << "name: " << dummystud.name << endl;
        cout << "sex: " << dummystud.sex << endl;
        cout << "date of birth: " << dummystud.DOB.d << "-" << dummystud.DOB.m
        << "-" << dummystud.DOB.y << endl;

        if (dummystud.gpa >= 3.5)
        cout << "grade: A" << endl;

        else if (dummystud.gpa >= 3.0)
        cout << "grade: B" << endl;

        else if (dummystud.gpa >= 2.0)
        cout << "grade: C" << endl;

    else
        cout << "Pass" << endl;

    cout << "\n\n\n************************************";
}

//----------------------------------------------------

---

>>Student stud[5];
std::array<Student, 5> stud;
Requires <array>.

Or, std::vector<Student> stud(5);
Requires <vector>.

>>void prnt (Student dummystud)
Pass by reference
void prnt (const Student & dummystud)

Code:

---

// student_data_read_practice.cpp
// program which reads data of some students

#include <iostream>
#include <cstdlib>
#include <string>
#include <cstring>

using namespace std;

/* Move all this to a header file, using std:: as needed.*/
/////////////////////////////////////////////////////
struct Date {int d; string m; int y;}; //use longer variable names
////////////////////////////////////////////////////

/////////////////////////////////////////////////////
struct Student {int rollno; string sex; string name; float gpa; Date DOB;};
//don't use all caps for variable names
//Make this into a class with print and read methods
/////////////////////////////////////////////////////

Student read();
void prnt(Student dummystud);
/*end of area to move*/

int main()
{
    Student stud[5];//Don't mix tabs and spaces for indination.

        for(int i=0; i<5; i++)
        {
            cout << "enter student #" << (i+1) << "'s details below:-" << endl;
                stud[i] = read();
                prnt(stud[i]);
                cout << endl << endl;
        }

        system("pause");
        return 0;
}

//-----------------------------------------------
// Student read() definition

Student read()//I would make this a constructor for Student instead.
{
    Student stud; string name;

    cout << "enter name: "; getline(cin, name);
        cout << "enter roll number: "; cin >> stud.rollno;
        cout << "enter sex: "; cin >> stud.sex;
        cout << "enter date of birth (e.g. 01 Jan 2000) below:-" << endl;
        cout << "enter day: "; cin >> stud.DOB.d;
        cout << "enter month: "; cin >> stud.DOB.m;
        cout << "enter year: "; cin >> stud.DOB.y;
        cout << "enter GPA: "; cin >> stud.gpa;
        cin.ignore();

        return stud;
}

//-------------------------------------------------
// void prnt() definition

void prnt (const Student &dummystud)
{
    cout << "\n\n\n************************************\n\n\n";
        cout << "roll no.: " << dummystud.rollno << endl;
        cout << "name: " << dummystud.name << endl;
        cout << "sex: " << dummystud.sex << endl;
        cout << "date of birth: " << dummystud.DOB.d << "-" << dummystud.DOB.m
        << "-" << dummystud.DOB.y << endl;

        if (dummystud.gpa >= 3.5)
        cout << "grade: A" << endl;

        else if (dummystud.gpa >= 3.0)
        cout << "grade: B" << endl;

        else if (dummystud.gpa >= 2.0)
        cout << "grade: C" << endl;

    else
        cout << "Pass" << endl;

    cout << "\n\n\n************************************";
}

//----------------------------------------------------

---

I would do a lot more input validation. What makes a date of birth OK? Because right now I could enter in calendar dates that don't even exist, like Feb 30. What should sex really contain ("Male" or "Female", or something else) and how are you going to make sure that's what it has? Aren't you going to restrict GPA? Issues like this require thoughtful answers.

Thanks a lot, Elysia, King Mir, whiteflags.

@Elysia: I haven't read anything about vectors so I don't know how to use them.

What's the benefit of const'ing the data type Student?

Code:

---

void prnt (const Student & dummystud)

---

Even without it the function won't accept anything else except Student data type structure. I understand the reason for passing arguments by reference.

@whiteflags: What you say is absolutely correct and valid. But I'm a beginner so I don't know how to really restrict the inputs. Obviously, it will make the code complex and longer.

@King Mir: How do you move that part of the code into a header? I didn't know there existed user created header files. Could you please tell me how to do this, if it's simple?

Thank you for the help.

Best wishes
Jackson

Quote:

---

Originally Posted by jackson6612

What's the benefit of const'ing the data type Student?

Code:

---

void prnt (const Student & dummystud)

---

Even without it the function won't accept anything else except Student data type structure. I understand the reason for passing arguments by reference.

---

If you try to modify "dummystud" inside "prnt()", your compiler will complain. The same will happen if you call a "Student" member function in "prnt()" which is non-constant.

Quote:

---

Originally Posted by jackson6612

@King Mir: How do you move that part of the code into a header? I didn't know there existed user created header files. Could you please tell me how to do this, if it's simple?

---

*) You create a new file as *.h or *.hpp or *.hh ( Stick with *.h ;) )
*) You cut-paste your code there, making the necessary changes
*) Include your new header file in your project like this:

Code:

---

#include "myHeader.h"

---

Don't forget to wrap your header code in a preprocessor flag:

Code:

---

#ifndef __MY_HEADER_H__
#define __MY_HEADER_H__

// Your code goes here

#endif // __MY_HEADER_H__

---

Quote:

---

Originally Posted by GReaper

Don't forget to wrap your header code in a preprocessor flag:

Code:

---

#ifndef __MY_HEADER_H__
#define __MY_HEADER_H__

// Your code goes here

#endif // __MY_HEADER_H__

---

---

That is not a "preprocessor flag". It is more usually described as an "include guard".

Also, in C++, never use identifiers (or macro names) that contain double underscores, or that begin with a single underscore. The standard reserves such identifiers for use by the implementation. That means, if you use them in your code (or header files) your code might break if the implementation (for example, in standard header files) uses the identifier you have.

Thank you, GReaper, grumpy.

GReaper said:

Quote:

---

If you try to modify "dummystud" inside "prnt()", your compiler will complain. The same will happen if you call a "Student" member function in "prnt()" which is non-constant.

---

I'm sorry but I don't understand the red part. In my code I didn't call "Student read()" inside the "prnt()". Anyway, by this:

Code:

---

void prnt (const Student & dummystud)

---

I have only made the variable "dummystud" of constant type which cannot be messed with. Please explain it if possible. Thank you.

If you think the answer is too obvious and you think I'm just stupid, then please leave it as it is, and please don't make any hurtful comments. I'm already having very bad time these days. Thank you.

What's with all the abbreviations? Why "prnt" - is the one letter required to form the word "print" really so hard to type? I would actually go for longer and clearer names - "readStudent" and "printStudent" instead of "read" and "print", for example. What happens when you add another class, e.g. Lecture? Sure, you can overload print with one version taking a Student and one taking a Lecture, and it could even be convenient to simply use print() for everything. But you can't overload on the return type, so you can't have both "Student read()" and "Lecture read()". Then you have to rename them anyway - and if you have readStudent(), then you should have printStudent() simply for consistency.

More stuff like this is all over the place. Why are Date's members called "d", "m" and "y"? Why not "day", "month" and "year"? It makes the code read a lot better. In Student, why "DOB", which is not only cryptic but also inconsistent in capitalization with everything else? "dateOfBirth" is a great name.
Why call the master array of students "stud"? Why not "students"?

This isn't the 70s. You're no longer limited to 8 or less characters in a name. Make use of that!

Quote:

---

Originally Posted by jackson6612

Thank you, GReaper, grumpy.

GReaper said:

I'm sorry but I don't understand the red part. In my code I didn't call "Student read()" inside the "prnt()".

---

Code:

---

class Student {
    int rollno;
    string sex;
    string name;
    float gpa;
    Date DOB;
public:
    void prnt() const; //this is a const member function
};

---

Quote:

---

Originally Posted by grumpy

That is not a "preprocessor flag". It is more usually described as an "include guard".

---

You say po-tay-to, I say po-tah-to. :)

Quote:

---

Originally Posted by grumpy

That means, if you use them in your code (or header files) your code might break if the implementation (for example, in standard header files) uses the identifier you have.

---

Let's wish that never happens. ;)

Quote:

---

Originally Posted by GReaper

You say po-tay-to, I say po-tah-to. :)

---

Not at all. You gave wrong information, I corrected you. The term "preprocessor flag" has a completely different meaning from what you described.

Quote:

---

Originally Posted by GReaper

Let's wish that never happens. ;)

---

If wishes were fishes, the ocean would be full.

Quote:

---

Originally Posted by King Mir

Code:

---

class Student {
    int rollno;
    string sex;
    string name;
    float gpa;
    Date DOB;
public:
    void prnt() const; //this is a const member function
};

---

---

Thank you, King Mir.

Please help me with some follow-on queries. Thanks.

I see that you used the word "public", what does it do? How would it differ if it were "private" instead?

You have used the word "const" after the function name although before this "const" used to precede anything it acts on. Okay. How would it made the function 'constant' this way?

I see you didn't include the "read()" function in the class. Any reason?

Thank you for the help and your time.

Regards
Jackson

PS: Sorry, CornedBee, I forgot to thank you.

Quote:

---

Originally Posted by jackson6612

I see that you used the word "public", what does it do? How would it differ if it were "private" instead?

---

Familiarize yourself with classes and you shall understand.

Quote:

---

You have used the word "const" after the function name although before this "const" used to precede anything it acts on. Okay. How would it made the function 'constant' this way?

---

Familiarize yourself with const correctness and you shall understand.

Quote:

---

Originally Posted by jackson6612

I see that you used the word "public", what does it do? How would it differ if it were "private" instead?

---

The default is private. But a private method can't be called by the user; it is only used internally. So if you created such an object, you would not be able to call obj.prnt(), unless prnt() is public.

Quote:

---

You have used the word "const" after the function name although before this "const" used to precede anything it acts on. Okay. How would it made the function 'constant' this way?

---

This just indicates prnt() cannot alter any member variables of the class. Ie, it makes all class members const within the context of prnt().

Quote:

---

I see you didn't include the "read()" function in the class. Any reason?

---

Because the code was converted to OO.

Quote:

---

Originally Posted by King Mir

Code:

---

// Student read() definition

Student read()//I would make this a constructor for Student instead.
{

---

---

Quote:

---

//I would make this a constructor for Student instead.

---

I wouldn't. I've found constructors that do I/O to be unclear in most situations where I've seen them.

Quote:

---

Originally Posted by CornedBee

I wouldn't. I've found constructors that do I/O to be unclear in most situations where I've seen them.

---

Just a question about that, would you still say that if the constructor took a stream object as an input ? ....(along with overloading, to provide another interface)....

Quote:

---

Originally Posted by manasij7479

Just a question about that, would you still say that if the constructor took a stream object as an input ? ....(along with overloading, to provide another interface)....

---

Yes. A constructor taking a stream does not indicate what kind of interaction it has with the stream. Will it interactively read a record from the user, along with input validation and prompting the user for correction of bad input? (In such a case you also need an output stream for such prompts.) Or will it read the record of a fixed format from a file, where throwing an exception is an acceptable response to bad input? The latter case is one where I would find a constructor taking a stream more appropriate, though it still leaves something to be desired in terms of flexibility, since the constructor can only support one format.
In all such cases, I would consider free functions a better choice for doing I/O. This includes the print() function, by the way.

When you put interactive I/O into your domain objects (Student clearly is of that kind) you saddle them with a lot of concerns. Input validation is just one of them - having user interaction also means localization, for example. Then there's the matter of choice of user interface. These things do not, of course, matter for toy programs, but can quickly become very relevant when programs grow and mature.

This is, in essence, what the model-view-controller (MVC) pattern is about: separating the model (your domain objects) from functionality specific to one kind of user interface. If Student were to have a print() function to print its contents to the console, you have mingled the view (console) with the model (Student). What will you do with that print() function when you want to convert the program to have a GUI instead of being a console program? Where will you put the functions to display a Student in the GUI? The answer should not be a modification to Student.