HackerRank - Virtual Function Practice

Question

This is just simple code to practice the use of virtual functions from a practice challenge in HackerRank. Could you give me some advice about the best practices and style?

This is just a simple exercise from HackerRank

#include <string>
#include <array>
#include <numeric>
#include <iostream>
#include <algorithm>

class Person {
    protected:
    std::string name;
    int age;
    public:
    virtual void getdata() = 0;
    virtual void putdata() const = 0;
};
class Student final : public Person {  
    static inline int id_count = 0;
    std::array<int,6> marks;
    int cur_id;
    public:
    Student(){
        cur_id = ++Student::id_count;
    }
    void getdata() override {
        std::cin >> this->name;
        std::cin >> this->age;
        for (auto &mark : marks) {
            std::cin >> mark;
        }
    }
    void putdata() const override{
        std::cout << name << " " << age << " " << std::accumulate(marks.begin(), marks.end(), 0) << " " << cur_id << std::endl;
    }
};

class Professor final : public Person {
    static inline int id_count = 0;
    int publications;
    int cur_id;

    public:
    Professor(){
        cur_id = ++Professor::id_count;
    }
    void getdata() override {
        std::cin >> this->name;
        std::cin >> this->age;
        std::cin >> this->publications;
    }
    void putdata() const override {
        std::cout << name << " " << age << " " << publications << " " << cur_id << endl;
    }
};

Code provided by the question (cannot be edited):

 int main(){
    int n, val;
    std::cin>>n; //The number of objects that is going to be created.
    Person *per[n];

    for(int i = 0;i < n;i++){

        std::cin>>val;
        if(val == 1){
            // If val is 1 current object is of type Professor
            per[i] = new Professor;

        }
        else per[i] = new Student; // Else the current object is of type Student

        per[i]->getdata(); // Get the data from the user.

    }

    for(int i=0;i<n;i++)
        per[i]->putdata(); // Print the required output for each object.

    return 0;

}

Answer 1

• Be consistent.
  You use a blank line to separate classes Student and Professor: Person and Studentdeserve no less. (Same goes for change in accessibility; alternatives to a blank line include (less) indent.)
• Don't write, never present uncommented code.
  Use all machine/tool support you can get: have a look at doxygen.
• make classes (and functions) final when further derivation/overriding is bound to break something, only.
• Think of derivation as extension or specialisation.
  You declared common attributes for Person: try and do the same for behaviour, even if it is more cumbersome in C++ (& influenced languages) than in Simula: There, a virtual function gets to call inner - with common code before&after.
  In C++, I know no better than starting Derived::f() with Base::f();.
  In this example, getdata() & putdata() shouldn't be pure virtual:
  Person::getdata() should input name and age; Person::putdata() output them. We are stuck without a method to handle specialised output is complete imposing. One can define void putdata() const { std::cout << toString() << std::endl; } (with toString() facing the exact problem with cur_id "on the other side").

• Avoid overly long lines.

  name + " " + std::to_string(age)
       + " " + std::accumulate(marks.begin(), marks.end(), 0)
       + " " + std::to_string(cur_id)
  

  is easier to read and probably works just as well as a one-liner.

(While this review has been received exceedingly friendly,
the following code misses the mark correct contemporary C++ by a mile.
(Hopefully closing in with revisions (incorporating advice from Edward's answer);
Having "fun" with IDEone - don't hold your breath.))

Shots from the hip:

/*! \brief Person sporting common attributes
 *         and get/putdata().
 *
 *  Attributes include name, age, and an in-subclass ID.
 *  getdata() reads from std::cin, starting with name&age;
 *  putdata() writes a line to std::cout.
 */
class Person {
 protected:
    //    const int cur_id;
    std::string name;
    int age;
    /*! terse human readable representation.
    */
    virtual std::string toString() const {
        return name + " " + std::to_string(age);
    }
    ~Person(){}
 public:
    /*! read name&age from std::cin.
     */
    virtual void getdata() {
        std::cin >> name >> age;
    }
    /*! putdata() writes a line to std::cout.
     */
    virtual void putdata(std::ostream &out = std::cout) const {
        out << toString() << std::endl;
    }
};

/*! Student adds marks for subjects to the common attributes.
 */
class Student : public Person {
    const int cur_id;
//    static std::atomic<int> id_count(0);
    std::array<int, 6> marks;
 protected:
    std::string toString() const override {
        return Person::toString()
        + " " + std::to_string(
            std::accumulate(marks.begin(), marks.end(), 0))
        + " " + std::to_string(cur_id);
    }
 public:
    static int id_count;// = 0;
    Student() : cur_id(++id_count) {}
    /*! read as Person from std::cin; add marks.
     */
    void getdata() override {
        Person::getdata();
        for (auto &mark : marks)
            std::cin >> mark;
    }
};

/*! Professor adds a publication count to the common attributes.
 */
class Professor final : public Person {
    const int cur_id;
//    static std::atomic<int> id_count(0);
    int publications;
 protected:
    std::string toString() const override {
        return Person::toString()
        + " " + std::to_string(publications)
        + " " + std::to_string(cur_id);
    }
 public:
    static int id_count;// = 0;
    Professor() : cur_id(++id_count) {}
    /*! read as Person from std::cin; add publications.
     */
    void getdata() override {
        Person::getdata();
        std::cin >> publications;
    }
};

int Student::id_count = 0;
int Professor::id_count = 0;

Answer 2

It's unfortunate that we can't fix the provided code, as there is a lot more wrong with it than with your code, in my view. Because this is a learning exercise and because it may benefit others who read this later, I'm going to review both pieces of code, starting with the code you actually wrote.

Fix the bug

There is one instance in which endl is used without std:: namespace. It won't compile that way on my machine. In any case, see the next suggestion.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Understand inline

The inline specifier is new with C++17 and would allow one to, in this case, put the initialization of the member variable id_count within the header file, even if the header file is #included in multiple other files (technically translation units). In the context of the original code, there's not much real benefit to it, since there appears to be only one translation unit. Using C++11 (or C++14) syntax, one could simply omit the inline keyword and then write

int Student::id_count{0};

outside the class definition. Maybe you already knew all of that, but I think it's worthwhile clarifying what it means and how it works.

Prefer modern initializers for constructors

The constructor could use the more modern initializer style rather than the old style you're currently using. Instead of this:

Student(){
    cur_id = ++Student::id_count;
}

You could use this:

Student() :
    cur_id{++id_count}
{}

There is not a significant difference in this code, but it's a good habit to get into using.

Avoid using explicit this

The current code includes this:

std::cin >> this->name;
std::cin >> this->age;

There's really no compelling reason to have this-> here explicitly and no good reason not to chain the two extractions. I'd write it like this:

std::cin >> name >> age;

Think carefully about common code

Both the Student and Professor classes read in and write out the name and age first. Since that's common to both, I'd be inclined to put that into the base Person class. As an example, within the Person class:

virtual void getdata() {
    std::cin >> name >> age;
}

Then within the Student class:

void getdata() override {
    Person::getdata();           // call the base class
    for (auto &mark : marks) {   // do things unique to Student
        std::cin >> mark;
    }
}

This would also have the benefit that you could have Person member data private instead of protected.

Consider explicitly defining all member data

What is the value of name just after a Professor object is created? If you don't know, or if you know but think it ould be improved, I'd suggest explicitly defining all member data such that a constructed object has a known, rational state.

Make your base class destructor virtual

By default, a compiler-generated destructor is concrete rather than virtual, but this leads to problems with collections of objects. See this question for more details on why.

Consider the user

It would be convenient to be able to specify streams other than std::cin and std::cout for your classes. One way to do that and still be able to use the given inteface code would be to have a default parameter like this:

void putdata(std::ostream &out = std::cout) const override;

That way, calling code that does not specify an argument will get std::cout but it's still possible to explicitly specify some other stream.

Comments on given code harness

Match new with delete

If you allocate memory using new, you should also free it using delete or your program will leak memory. Since main uses new to create each Person object, it should also delete each such object. Yes, the operating system will generally clean up such messes, but there's little advantage in creating them in the first place.

Use more descriptive variable names

Single-letter variable names and extremely generic names like val make it harder to understand the program. More meaningful variable names make the code easier to read and maintain.

Check your if statements for proper braces

It's not helpful to use curly braces on the if clause and then omit them on the else clause. Generally, I prefer to always have the braces. It makes the intent clear and prevents bugs being introduced in maintenance.