11
\$\begingroup\$

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;

}
\$\endgroup\$
  • \$\begingroup\$ It seems you forgot std:: in some places in main() \$\endgroup\$ – yuri Apr 25 '18 at 14:39
  • \$\begingroup\$ @yuri Thanks, I confess that I wrote the program with using namespace std \$\endgroup\$ – Gitana Apr 25 '18 at 14:43
  • 6
    \$\begingroup\$ I wrote the program with using namespace std I'll just pretend I didn't read that ;) \$\endgroup\$ – yuri Apr 25 '18 at 14:48
  • 2
    \$\begingroup\$ this should explain the inherited object problem. Basically Objects can be sliced but pointers won't. \$\endgroup\$ – bruglesco Apr 25 '18 at 17:14
  • 2
    \$\begingroup\$ I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you. \$\endgroup\$ – Phrancis Apr 25 '18 at 18:52
10
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ Definitely don't use -Weverything in clang. It's useful to discover warnings that could potentially be helpful, but it's a bad idea to try to be -Weverything warning free. I tend to use -Wall -Wextra -Wpedantic (and potentially turn on a few more helpful warnings) \$\endgroup\$ – Justin Apr 25 '18 at 19:56
  • \$\begingroup\$ @Justin I agree that it's not a good idea to try to be -Weverything warning free but IMO it's a good idea for the discovering part you mentioned. Most people asking for guidance here have never seen a compiler warning in their life so this would certainly be a step forward. I'll edit the answer to point out this is merely designed to be a helper. \$\endgroup\$ – yuri Apr 25 '18 at 20:02
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You could probably even mark it as final if nothing is supposed to be derived from Intercity. \$\endgroup\$ – yuri Apr 25 '18 at 21:14
  • \$\begingroup\$ Writing Station(const Station & s) =default; may cause some problems, as it disables the move constructor. That's fine in this case, as moving is just as cheap as copying, but it's usually not what you want. \$\endgroup\$ – Justin Apr 25 '18 at 23:53
  • 1
    \$\begingroup\$ +1 for mentioning the C++ core guidelines! \$\endgroup\$ – Edward Apr 26 '18 at 11:34
  • 1
    \$\begingroup\$ @Justin true. If declaring special functions just to show that they are meant to exist rather than having them be invisible, yiu should list all of them as either default or deleted, to make your intentions clear. In general I'm used to move ctor not being implicitly defined since any copy/assign/destroy function will disable it, so I never really count on it. \$\endgroup\$ – JDługosz Apr 26 '18 at 16:47
6
\$\begingroup\$

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.

\$\endgroup\$
  • 10
    \$\begingroup\$ So shouldn’t you be m_Coal and you think your own name is ugly?? \$\endgroup\$ – JDługosz Apr 25 '18 at 20:21
  • 2
    \$\begingroup\$ Private is unnecessary but many dislike omitting it. \$\endgroup\$ – Joshua Apr 26 '18 at 2:29
5
\$\begingroup\$

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_);
}
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ The current trend promoted by the experts is to make things free functions where possible. If distance can be calculated using only the public interface of the objects, it should be. See for example Free Your Functions video from CppCon 2017. That does not change the encapsulation: if everyone used the function rather than computing it ad hoc themselves, the formula would be neatly encapsulated. \$\endgroup\$ – JDługosz Apr 26 '18 at 16:53
  • 1
    \$\begingroup\$ Since he wrote a friend rather than a function using the public api, I’m guessing that he was looking at the symmetry of operands, as is taught for e.g. operator+ that can do implicit conversions on each operand the same way. \$\endgroup\$ – JDługosz Apr 26 '18 at 16:57
  • 1
    \$\begingroup\$ I'll check out that CppCon talk, thanks! As for operator+, basing it on a member function operator+= is the canonical implementation method. \$\endgroup\$ – Edward Apr 26 '18 at 16:59
  • \$\begingroup\$ I'm only about half way through the CppCon talk, but I think the lesson is to prefer non friend functions. \$\endgroup\$ – Edward Apr 26 '18 at 17:24
  • \$\begingroup\$ That's what I said: «If distance can be calculated using only the public interface of the objects, it should be» \$\endgroup\$ – JDługosz Apr 26 '18 at 17:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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