HackerRank - Virtual Function Practice
up vote
6
down vote
favorite
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;
}
c++
add a comment |
up vote
6
down vote
favorite
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;
}
c++
(Welcome to posting on CR!) To make contents not authored by you instantly identifiable, please make it a block quote: mark an use the "the“”
/ block quote button" (or its keyboard short-cut) in the post editor or put>
before each line (works with nested contents like code-blocks, too).
– greybeard
Dec 19 '17 at 9:34
3
Wow. That uneditable snippet of code is terrible. It lacks spaces between operators. Has bad namesn, val, per
. Leaks memory without even a comment about it. Bad naming of the virtual functionsgetdata()
andputdata()
. When we seegetfoo
, we expect that it is a getter function, and doesn't modify state, andputfoo
is expected to be some kind of setter function (setfoo
is a more common name), which does modify state; instead, they do the opposite.
– Justin
Dec 22 '17 at 23:40
@Justin hahaha that's good feedback, I also thought the same about the provided code!
– WooWapDaBug
Dec 24 '17 at 9:18
add a comment |
up vote
6
down vote
favorite
up vote
6
down vote
favorite
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;
}
c++
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;
}
c++
c++
edited Jan 18 at 7:45
asked Dec 19 '17 at 9:21
WooWapDaBug
362314
362314
(Welcome to posting on CR!) To make contents not authored by you instantly identifiable, please make it a block quote: mark an use the "the“”
/ block quote button" (or its keyboard short-cut) in the post editor or put>
before each line (works with nested contents like code-blocks, too).
– greybeard
Dec 19 '17 at 9:34
3
Wow. That uneditable snippet of code is terrible. It lacks spaces between operators. Has bad namesn, val, per
. Leaks memory without even a comment about it. Bad naming of the virtual functionsgetdata()
andputdata()
. When we seegetfoo
, we expect that it is a getter function, and doesn't modify state, andputfoo
is expected to be some kind of setter function (setfoo
is a more common name), which does modify state; instead, they do the opposite.
– Justin
Dec 22 '17 at 23:40
@Justin hahaha that's good feedback, I also thought the same about the provided code!
– WooWapDaBug
Dec 24 '17 at 9:18
add a comment |
(Welcome to posting on CR!) To make contents not authored by you instantly identifiable, please make it a block quote: mark an use the "the“”
/ block quote button" (or its keyboard short-cut) in the post editor or put>
before each line (works with nested contents like code-blocks, too).
– greybeard
Dec 19 '17 at 9:34
3
Wow. That uneditable snippet of code is terrible. It lacks spaces between operators. Has bad namesn, val, per
. Leaks memory without even a comment about it. Bad naming of the virtual functionsgetdata()
andputdata()
. When we seegetfoo
, we expect that it is a getter function, and doesn't modify state, andputfoo
is expected to be some kind of setter function (setfoo
is a more common name), which does modify state; instead, they do the opposite.
– Justin
Dec 22 '17 at 23:40
@Justin hahaha that's good feedback, I also thought the same about the provided code!
– WooWapDaBug
Dec 24 '17 at 9:18
(Welcome to posting on CR!) To make contents not authored by you instantly identifiable, please make it a block quote: mark an use the "the
“”
/ block quote button" (or its keyboard short-cut) in the post editor or put >
before each line (works with nested contents like code-blocks, too).– greybeard
Dec 19 '17 at 9:34
(Welcome to posting on CR!) To make contents not authored by you instantly identifiable, please make it a block quote: mark an use the "the
“”
/ block quote button" (or its keyboard short-cut) in the post editor or put >
before each line (works with nested contents like code-blocks, too).– greybeard
Dec 19 '17 at 9:34
3
3
Wow. That uneditable snippet of code is terrible. It lacks spaces between operators. Has bad names
n, val, per
. Leaks memory without even a comment about it. Bad naming of the virtual functions getdata()
and putdata()
. When we see getfoo
, we expect that it is a getter function, and doesn't modify state, and putfoo
is expected to be some kind of setter function (setfoo
is a more common name), which does modify state; instead, they do the opposite.– Justin
Dec 22 '17 at 23:40
Wow. That uneditable snippet of code is terrible. It lacks spaces between operators. Has bad names
n, val, per
. Leaks memory without even a comment about it. Bad naming of the virtual functions getdata()
and putdata()
. When we see getfoo
, we expect that it is a getter function, and doesn't modify state, and putfoo
is expected to be some kind of setter function (setfoo
is a more common name), which does modify state; instead, they do the opposite.– Justin
Dec 22 '17 at 23:40
@Justin hahaha that's good feedback, I also thought the same about the provided code!
– WooWapDaBug
Dec 24 '17 at 9:18
@Justin hahaha that's good feedback, I also thought the same about the provided code!
– WooWapDaBug
Dec 24 '17 at 9:18
add a comment |
2 Answers
2
active
oldest
votes
up vote
6
down vote
accepted
- Be consistent.
You use a blank line to separate classesStudent
andProfessor
:Person
andStudent
deserve 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 forPerson
: try and do the same for behaviour, even if it is more cumbersome in C++ (& influenced languages) than in Simula: There, avirtual
function gets to callinner
- with common code before&after.
In C++, I know no better than startingDerived::f()
withBase::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 definevoid putdata() const { std::cout << toString() << std::endl; }
(withtoString()
facing the exact problem withcur_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;
(Without a half-decent C++-environment in working condition, I have no idea whether, for example,cur_id
works (please feel invited to edit): I want it unmodifiable after instantiation, I want one declaration for the entire hierarchy - I have a nagging feeling it not only doesn't work as presented: I must be missing a way not to repeat its initialisation.) (I keep forgetting about on-line environments - age showing:)
– greybeard
Dec 24 '17 at 10:43
Thank you very much for your answer. I'll work on the tips you have given me
– WooWapDaBug
Dec 24 '17 at 15:01
(I never quite gotostream
operator<<
to be the method to append human readable representation.)
– greybeard
Dec 25 '17 at 8:49
add a comment |
up vote
6
down vote
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 #include
d 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 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 enerally 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.
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f183165%2fhackerrank-virtual-function-practice%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
6
down vote
accepted
- Be consistent.
You use a blank line to separate classesStudent
andProfessor
:Person
andStudent
deserve 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 forPerson
: try and do the same for behaviour, even if it is more cumbersome in C++ (& influenced languages) than in Simula: There, avirtual
function gets to callinner
- with common code before&after.
In C++, I know no better than startingDerived::f()
withBase::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 definevoid putdata() const { std::cout << toString() << std::endl; }
(withtoString()
facing the exact problem withcur_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;
(Without a half-decent C++-environment in working condition, I have no idea whether, for example,cur_id
works (please feel invited to edit): I want it unmodifiable after instantiation, I want one declaration for the entire hierarchy - I have a nagging feeling it not only doesn't work as presented: I must be missing a way not to repeat its initialisation.) (I keep forgetting about on-line environments - age showing:)
– greybeard
Dec 24 '17 at 10:43
Thank you very much for your answer. I'll work on the tips you have given me
– WooWapDaBug
Dec 24 '17 at 15:01
(I never quite gotostream
operator<<
to be the method to append human readable representation.)
– greybeard
Dec 25 '17 at 8:49
add a comment |
up vote
6
down vote
accepted
- Be consistent.
You use a blank line to separate classesStudent
andProfessor
:Person
andStudent
deserve 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 forPerson
: try and do the same for behaviour, even if it is more cumbersome in C++ (& influenced languages) than in Simula: There, avirtual
function gets to callinner
- with common code before&after.
In C++, I know no better than startingDerived::f()
withBase::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 definevoid putdata() const { std::cout << toString() << std::endl; }
(withtoString()
facing the exact problem withcur_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;
(Without a half-decent C++-environment in working condition, I have no idea whether, for example,cur_id
works (please feel invited to edit): I want it unmodifiable after instantiation, I want one declaration for the entire hierarchy - I have a nagging feeling it not only doesn't work as presented: I must be missing a way not to repeat its initialisation.) (I keep forgetting about on-line environments - age showing:)
– greybeard
Dec 24 '17 at 10:43
Thank you very much for your answer. I'll work on the tips you have given me
– WooWapDaBug
Dec 24 '17 at 15:01
(I never quite gotostream
operator<<
to be the method to append human readable representation.)
– greybeard
Dec 25 '17 at 8:49
add a comment |
up vote
6
down vote
accepted
up vote
6
down vote
accepted
- Be consistent.
You use a blank line to separate classesStudent
andProfessor
:Person
andStudent
deserve 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 forPerson
: try and do the same for behaviour, even if it is more cumbersome in C++ (& influenced languages) than in Simula: There, avirtual
function gets to callinner
- with common code before&after.
In C++, I know no better than startingDerived::f()
withBase::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 definevoid putdata() const { std::cout << toString() << std::endl; }
(withtoString()
facing the exact problem withcur_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;
- Be consistent.
You use a blank line to separate classesStudent
andProfessor
:Person
andStudent
deserve 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 forPerson
: try and do the same for behaviour, even if it is more cumbersome in C++ (& influenced languages) than in Simula: There, avirtual
function gets to callinner
- with common code before&after.
In C++, I know no better than startingDerived::f()
withBase::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 definevoid putdata() const { std::cout << toString() << std::endl; }
(withtoString()
facing the exact problem withcur_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;
edited 21 mins ago
community wiki
5 revs, 3 users 98%
greybeard
(Without a half-decent C++-environment in working condition, I have no idea whether, for example,cur_id
works (please feel invited to edit): I want it unmodifiable after instantiation, I want one declaration for the entire hierarchy - I have a nagging feeling it not only doesn't work as presented: I must be missing a way not to repeat its initialisation.) (I keep forgetting about on-line environments - age showing:)
– greybeard
Dec 24 '17 at 10:43
Thank you very much for your answer. I'll work on the tips you have given me
– WooWapDaBug
Dec 24 '17 at 15:01
(I never quite gotostream
operator<<
to be the method to append human readable representation.)
– greybeard
Dec 25 '17 at 8:49
add a comment |
(Without a half-decent C++-environment in working condition, I have no idea whether, for example,cur_id
works (please feel invited to edit): I want it unmodifiable after instantiation, I want one declaration for the entire hierarchy - I have a nagging feeling it not only doesn't work as presented: I must be missing a way not to repeat its initialisation.) (I keep forgetting about on-line environments - age showing:)
– greybeard
Dec 24 '17 at 10:43
Thank you very much for your answer. I'll work on the tips you have given me
– WooWapDaBug
Dec 24 '17 at 15:01
(I never quite gotostream
operator<<
to be the method to append human readable representation.)
– greybeard
Dec 25 '17 at 8:49
(Without a half-decent C++-environment in working condition, I have no idea whether, for example,
cur_id
works (please feel invited to edit): I want it unmodifiable after instantiation, I want one declaration for the entire hierarchy - I have a nagging feeling it not only doesn't work as presented: I must be missing a way not to repeat its initialisation.) (I keep forgetting about on-line environments - age showing:)– greybeard
Dec 24 '17 at 10:43
(Without a half-decent C++-environment in working condition, I have no idea whether, for example,
cur_id
works (please feel invited to edit): I want it unmodifiable after instantiation, I want one declaration for the entire hierarchy - I have a nagging feeling it not only doesn't work as presented: I must be missing a way not to repeat its initialisation.) (I keep forgetting about on-line environments - age showing:)– greybeard
Dec 24 '17 at 10:43
Thank you very much for your answer. I'll work on the tips you have given me
– WooWapDaBug
Dec 24 '17 at 15:01
Thank you very much for your answer. I'll work on the tips you have given me
– WooWapDaBug
Dec 24 '17 at 15:01
(I never quite got
ostream
operator <<
to be the method to append human readable representation.)– greybeard
Dec 25 '17 at 8:49
(I never quite got
ostream
operator <<
to be the method to append human readable representation.)– greybeard
Dec 25 '17 at 8:49
add a comment |
up vote
6
down vote
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 #include
d 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 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 enerally 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.
add a comment |
up vote
6
down vote
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 #include
d 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 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 enerally 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.
add a comment |
up vote
6
down vote
up vote
6
down vote
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 #include
d 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 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 enerally 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.
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 #include
d 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 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 enerally 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.
edited Dec 24 '17 at 18:07
answered Dec 24 '17 at 15:41
Edward
45.6k377207
45.6k377207
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f183165%2fhackerrank-virtual-function-practice%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
(Welcome to posting on CR!) To make contents not authored by you instantly identifiable, please make it a block quote: mark an use the "the
“”
/ block quote button" (or its keyboard short-cut) in the post editor or put>
before each line (works with nested contents like code-blocks, too).– greybeard
Dec 19 '17 at 9:34
3
Wow. That uneditable snippet of code is terrible. It lacks spaces between operators. Has bad names
n, val, per
. Leaks memory without even a comment about it. Bad naming of the virtual functionsgetdata()
andputdata()
. When we seegetfoo
, we expect that it is a getter function, and doesn't modify state, andputfoo
is expected to be some kind of setter function (setfoo
is a more common name), which does modify state; instead, they do the opposite.– Justin
Dec 22 '17 at 23:40
@Justin hahaha that's good feedback, I also thought the same about the provided code!
– WooWapDaBug
Dec 24 '17 at 9:18