I've done a little bit of programming in the past, mostly just dabbling. After a long time of not touching an IDE, I am getting back into it, with Visual Studio. I just threw this quick project together to make sure I remember how to use classes and objects before I start playing with bigger projects.

I would love a quick critique to make sure there arent any glaring poor practices that could develop into bad habbits in the future.

I know the standard way to solve this problem is just time = distance/(velA+velB), but what would the point of using objects be if i didnt have the objects do something and change their status in someway?

What do you think, looks good?


The goal for this practice program is to solve the common math word problem below using classes and objects.
I feel that using iterations rather than the basic math formula to solve, although clearly less efficient, would be more true to thinking in terms of objects.

Train A, traveling X miles per hour (mph), leaves Westford heading toward Eastford, 260 miles away.
At the same time Train B, traveling Y mph, leaves Eastford heading toward Westford.
When do the two trains meet? How far from each city do they meet?


#include "pch.h"
#include <iostream>
using namespace std;

class Train
    Train(int, int);
    int getLocation();
    void update();
    int location, velocity;

Train::Train(int loc, int vel) //to initialize the train object and set its location and velocity
    location = loc;
    velocity = vel;

int Train::getLocation() // returns the location of the train object
    return location;

void Train::update() // updates the train object for one iteration
    location += velocity;

int main()
    int velA, velB, distance; //to take the values from the user input
    int time;   // to keep track of the number of iterations
                // time is declared here so it can be used outside of the for loop

    cout << "Enter velocity of the train from Westford:\n";
    cin >> velA;
    cout << "\n\nEnter velocity of the train from Eastford:\n";
    cin >> velB;
    cout << "\n\nEnter the distance between Westford and Eastford:\n";
    cin >> distance;

    //initialize each train
    Train trainA(0, velA);
    Train trainB(distance, 0 - velB);   //location of trainB is distance because the distance between an x coordinate at 0 and another x coordinate is equal to the second x coordinate
                                        //the velocity of trainB is the negative of velB because it is traveling in the opposite direction of trainA

    //run the sim
    for (time = 0; trainA.getLocation() < trainB.getLocation(); time++)

    cout << "\n\nThe Trains pass eachother after " << time << " hours."
        << "\nAt that time, the Westford train is " << trainA.getLocation() << " miles from Westford\n"
        << "and the Eastford train is " << distance - trainB.getLocation() << " miles from Eastford.\n\n";

    return 0;
  • \$\begingroup\$ I guess a do while loop could make more sense here than a for loop? \$\endgroup\$ Mar 22, 2019 at 3:05
  • \$\begingroup\$ At this stage of your learning, an IDE like Visual Studio is likely overkill. This is my preference, of course, but I would personally never use Visual Studio for C++ unless I was specifically developing for Windows using the C++ Windows API. With simple applications like this one, a basic text editor like Atom would be all you really need, and you can compile and run from the command line. For more advanced projects, I would choose Clion over Visual Studio any day of the week for portable C++ applications. YMMV. \$\endgroup\$ Mar 22, 2019 at 12:58

1 Answer 1

  • Note that pch.h header is not standard, and probably should not be part of this review.

  • Don't do using namespace std;.

  • Use initializer lists. That is, the constructor should be written as Train::Train(int loc, int vel) : location(loc), velocity(vel) { }. For POD types this might not matter, but it is idiomatic.

  • Because int getLocation() does not modify the state of the object, it should be made const, i.e., int getLocation() const.

  • In your main program, don't strive to declare all variables as soon as possible as this is C++ and not C. Instead, introduce variables as late as possible and as close to their site of usage as possible. As a remark, you should also initialize e.g., time when you define it to improve readability.

  • Yes, you could use a while-loop, but the for-loop has the advantage that you have to make an explicit decision as to what happens at the end of the loop in order for the program to compile. Because you need the time variable outside of the loop, you could do say for(; trainA.getLocation() < trainB.getLocation(); ++time) { ... }. A while-loop is also perfectly fine, and this boils down mostly to personal preference.

  • Perhaps you should enforce the train speeds to be positive. Otherwise you can get stuck in an infinite loop as the trains will never meet.

  • Your comments are too verbose. Good code is self-documenting via variables names and logical structure. Prefer comments that answer "why" to "what". For example, a comment like "input", "initialize each train", and "run the sim" are unnecessary, and only hurt readability.

  • \$\begingroup\$ Only in strictly standard-conforming pre-C99 C you would declare all the variables at the start of a block and later assign. \$\endgroup\$ Mar 22, 2019 at 17:33
  • \$\begingroup\$ @Deduplicator Exactly. \$\endgroup\$
    – Juho
    Mar 22, 2019 at 17:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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