A simple program with trains and inheritance

Question

Here it is my program. I'm especially worried about the main because I know that one must be careful while instancing vectors of "inherited" objects. The code I have works fine (as far as I can see) but I saw the original program made by my teacher and his implementation of the vector in the main uses new and iterator. I'd like to have some piece of advice on how to improve!

#include <vector>
#include <iostream>
#include <cmath> 

class Station {
private:
    double x_, y_;

public:
    Station(double x, double y) : x_(x), y_(y) { }
    Station(const Station & s) : x_(s.x_), y_(s.y_) { }

    // dichiarazione funzione globale  
    friend double dist(const Station & s, const Station & t);
};

class Train {
private:
    double vm_;
public:
    Train(double vm) : vm_(vm) { }
    double getvm() {return vm_; } 

    virtual double time_of_travel(const Station & s, const Station & t) { 
        return( dist(s, t)/vm_);
    }
};

class Regionale : public Train {
public:
    Regionale() : Train(60.) { }
};

class Intercity : public Train {
private:
    Station stop_;
public:
    Intercity(const Station stop) : stop_(stop), Train(110.) { }

    double time_of_travel(const Station & s, const Station & t) {
        return( (dist(s, stop_) + dist(stop_, t))/this->getvm());
    }

};


double dist(const Station & s, const Station & t) {
    return(sqrt(pow(s.x_ - t.x_, 2) + pow(s.y_ - t.y_, 2)));
}

int main() {
    Station Milano (0. ,0.);
    Station Bergamo (35. ,25.);
    Station Piacenza (40. , -45.);

    std::cout << dist (Milano, Bergamo) << std::endl;
    std::cout << dist (Bergamo, Piacenza) << std::endl;
    std::cout << dist (Piacenza, Milano) << std::endl;

    Intercity i(Milano);
    Regionale r;
    std::vector<Train * > v = {&i, &r};

    for(auto elem : v) std::cout << elem->time_of_travel(Bergamo, Piacenza) << std::endl;

}

Answer 1

• Coal_ already made some good points. Let's just elaborate on the one about private.
  A class makes everything in it private by default that is why you don't need it if you order your interface this way. However it's recommended to order your interface from public to private so people who read it can see which methods you expose without having to read through all the private data first.

• If you are using a trailing underscore because you're worried about problems when assigning arguments to members then there is no need for that. This is because it's guaranteed to work even if the names are the same. Just make sure to be careful in the constructor body if you do this!

• Your variable names could be better. In general you should avoid single letter names and choose a descriptive name.

• Some parts are probably easier to read if you put a few more spaces inbetween operators. Indenting could also be improved for example in your ctors:

  Foo()
      : bar(0)
      , baz(1)
  

  is a lot more readable than putting everything in a single line.

• Some of your parenthese in the return statements are superfluous and can be removed.

• In C++ it's generally recommended to treat & as part of the type and therefore write foo& instead of foo &.

• Since you included <cmath> you should probably prefix pow and sqrt with std::.

• You seem to use double solely because of your math functions. Perhaps strategic casting would be better instead of using a type you don't fully make use of.

• When using the ranged-for loop it's recommended to use this form:

  for (auto const& value : container) {
      // ...
  }
  

  The const makes it clear you don't intent to modify things in this loop and passing by reference avoids unecessary copies.

• If you run your compiler with warnings (as you should! For example to discover some potential problems you can use an online compiler like Clang with -Weverything. For production code you want to pick your warning levels more carefully though) you will notice that there are some issues.
  You have a wrong initialization order and are missing a virtual destructor. Order can be important when you depend on something in the base class.

• The way you handle your objects in main is certainly interesting but this seems like a good place for smart[1] pointers[2] and their respective make_unique/shared functions.

Answer 2

First, show your teacher the C++ Core Guidelines and various videos from conferences saying that you should not use naked new or delete anymore.

std::vector<Train * > v = {&i, &r};

You are storing pointers to local (stack based) objects, which is correct in this example because the vector is in that same scope. But usually you will never do that in real code.

More realistic code would be to write

std::vector<unique_ptr<Train>> v;

---

double time_of_travel(const Station & s, const Station & t)

You should mark this as override. You have no idea how handy and useful it is to always have virtual overrides marked. It allows you to make changes to code more reliably, as well as initially making sure you are not accidentally making a different function by the same overloaded name instead!

---

return( (dist(s, stop_) + dist(stop_, t))/this->getvm());

Why the this->? Don’t do that.

Don’t put extra parens around the return expression. This has always been a point of note, but now with return type deduction etc. it matters rather than being a matter of "style".

---

Use const !!!!

double getvm() {return vm_; } 

should be a const member.

---

Station(const Station & s) : x_(s.x_), y_(s.y_) { }

That’s what happens naturally. Leave off the explicit definition (which is worse that the automatic code and also prevents the compiler from recognising the regularity) or write

Station(const Station & s) =default;

---

Station Milano (0. ,0.);

Use modern (after 2011) uniform initialization syntax.

    Station Milano {0.0 ,0.0};

that is, curly braces for initialization.

Answer 3

I'm especially worried about the main because I know that one must be careful while instancing vectors of "inherited" objects.

I'm really new to C++, so I'm not going to attempt to help you there. Sorry.

General

• As made clear in the comments (and as you probably knew already), using namespace std; is considered bad practice, especially if you use it in small projects, because it can quickly become a habit.

• Line 5:

  class Station {
  private:
  

  ... private: is unnecessary.

• Line 13:

  // dichiarazione funzione globale
  

  ... I don't know Italian, and, conservative guess, neither do 80% of people here. You already wrote your class, function, and method names in English. Please do the same with comments.

• You might want to use an 'm_'-prefix instead of using a trailing underscore to avoid name clashes. It's up to you, but I think a trailing underscore looks ugly.

• Don't use std::endl when you only need a newline. std::endl not only adds a newline character, but also flushes the output stream, which can degrade performance. While this isn't a huge issue in most cases, if you forget about it, it might bite you when you least expect it. And hey, it saves typing!

• Please use braces around for-loops. It's easier to read and less error prone, which more than makes up for the tiny portion of extra time spent typing.

Answer 4

Rather than using the distance formula with sqrt(dx^2 + dy^2), use std::hypot:

double dist(const Station & s, const Station & t) {
    return std::hypot(s.x_ - t.x_, s.y_ - t.y_);
}

Answer 5

Since most of what I would have said is already in other reviews, I'll just mention one point not already suggested.

Consider a small change in interface

There's nothing currently wrong with the freestanding function dist, but it could also be declared as a member function like this:

double dist(const Station &other) const {
    return std::hypot(x_ - other.x_, y_ - other.y_);
}

That can then be called directly like this:

std::cout << Milano.dist(Bergamo) << std::endl;

Or you can define the friend function in terms of this one:

double dist(const Station & s, const Station & t) {
    return s.dist(t);
}

There is not a big difference with the code as it is today, but if, for example, you wished to change the calculation to use great circle distance equations, the change would be neatly encapsulated within the class.