Calculating when trains will meet, iteratively and using OOP

Question

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
{
public:
    Train(int, int);
    int getLocation();
    void update();
private:
    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


    //input
    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++)
    {
        trainA.update();
        trainB.update();
    }

    //output
    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;
}

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.