7
\$\begingroup\$

I've completed a program as a learning experience to make sure I've got the concepts down. I found a practice project online and decided to try it out. The program compiles and runs exactly how it should with no problems.

The program is a game called Artillery. You have to fire a cannon at one enemy at a time until you hit him. The player inputs the angle at which to shoot the cannon.

My question is this: Am I using correct coding practices? For instance, if I was on a team of people working on this, would my use of classes and objects be efficient?

Also, I would like any input on how to improve my code for this program or any tips for easier ways to write any part of the code. The code is as follows:

distang.h

#pragma once
#ifndef DISTANG_H
#define DISTANG_H

// Calculates angle and distance of shot
class DistAng
{
public:
    double distAng(double);
private:
    double velocity = 200;
    double gravity = 32.2;
    double shotDistance;
    double timeInAir;
    double pi = 3.1415;
};

#endif

game.h

#pragma once
#ifndef GAME_H
#define GAME_H

// The main loop for playing the game
class Game
{
public:
    void game();
private:
    int killed = 0;
    int enemyDistance = 0;
    double inAngle;
    char done;
    int shots = 0;
    bool killedEnemy = false;
};

#endif

highscore.h

#pragma once
#ifndef HIGHSCORE_H
#define HIGHSCORE_H

// Reads/saves high score from/to file
class HighScore
{
public:
    void highScore(int);
private:
    int intHighScore;
};

#endif

startup.h

#pragma once
#ifndef STARTUP_H
#define STARTUP_H

// Displays the game introduction
class Startup
{
public:
    void startup();
private:
};

#endif

DistAng.cpp

#include <iostream>
#include <math.h>
#include "distang.h"

using namespace std;

// Calculates distance and angle of shot
double DistAng::distAng(double inAngle)
{
    inAngle = (inAngle * pi) / 180; // Converts angle from std::cin into radians

    // A bit of simple physics to calculate the distance of shot
    timeInAir = (2.0 * velocity * sin(inAngle)) / gravity;
    shotDistance = round((velocity * cos(inAngle)) * timeInAir);

    return shotDistance;
}

Game.cpp

#include <iostream>
#include "game.h"
#include <time.h>
#include "distang.h"
#include "highscore.h"

using namespace std;

// Main game loop
void Game::game()
{
    do
    {
        // RNG to generate enemy distance each round :: least distance can be 200, max distance can be 900 :: and std::cout the enemey distance
        srand(time(0));
        enemyDistance = 200 + (rand() % 900);
        cout << "\nThe enemy is " << enemyDistance << " feet away!\n";

        // Creating a couple class object here
        DistAng calculate;
        HighScore submit;

        // Determines if enemy was killes, resets to false each round
        killedEnemy = false;

        // Loop for user input until enemy is hit or runs out of shots
        do
        {
            cout << "What angle? ";

            // Input check
            while (!(cin >> inAngle))
            {
                cin.clear();
                cin.ignore(numeric_limits<streamsize>::max(), '\n');
                cout << "Invalid input.  Try again: ";
            }

            // Keeps track of shots
            shots++;

            // Hit condition counts kills and displays shots taken
            if (enemyDistance == calculate.distAng(inAngle))
            {
                killed++;
                killedEnemy = true;
                cout << "\nYou hit him!\n";
                cout << "It took you " << shots << " shots.\n";
                break;
            }

            // Allows for shot correction if missed
            else if (enemyDistance < calculate.distAng(inAngle))
                cout << "\nYou over shot by " << calculate.distAng(inAngle) - enemyDistance << endl;

            else if (enemyDistance > calculate.distAng(inAngle))
                cout << "\nYou under shot by " << enemyDistance - calculate.distAng(inAngle) << endl;

        } while (shots != 10);

        // End of round if/else conditions for enemy killed or out of shots
        if (shots == 10 && killedEnemy == false)
        {
            shots = 0;
            cout << "You ran out of cannon balls!\n";
            cout << "You've killed " << killed << " of the enemy.\n";
            submit.highScore(killed); // Record enemies killed for duration of game
            killed = 0;

            // Input/New game check
            do
            {
                cin.clear();
                cin.ignore(numeric_limits<streamsize>::max(), '\n');
                cout << "Play again?(Y/N) ";
                cin >> done;
            } while (done != 'y' && done != 'n');

        }

        else
        {
            shots = 0;
            cout << "You've killed " << killed << " of the enemy.\n";

            // Input/Next round check
            do
            {
                cin.clear();
                cin.ignore(numeric_limits<streamsize>::max(), '\n');
                cout << "I see another one, care to shoot again?(Y/N) ";
                cin >> done;
            } while (done != 'y' && done != 'n');
        }

    } while (done != 'n');
}

HighScore.cpp

#include <iostream>
#include <fstream>
#include "highscore.h"

using namespace std;

// Reads/saves high score from/to file
void HighScore::highScore(int score)
{
    ifstream readScoreSheet("HighScore.art");

    // Checks current high score first
    if (readScoreSheet.is_open())
    {
        readScoreSheet >> intHighScore;
        readScoreSheet.close();
    }

    else // Condition for first game played
        cout << "No current high score.\n\n"; 

    // Player did not beat high score
    if (intHighScore > score)
        cout << "The current high score is " << intHighScore << " kills!\n\n";

    // If player beat high score
    else if (intHighScore < score)
    {
        ofstream writeScoreSheet("HighScore.art");

        // Replace old score with new score
        if (writeScoreSheet.is_open())
        {
            cout << "New high score!\n\n";
            writeScoreSheet << score;
            writeScoreSheet.close();
        }

        else cout << "Unable to open file"; // Never happens
    }
}

Main.cpp

#include <iostream>
#include "game.h"
#include "startup.h"
#include <fstream>

using namespace std;

// Main function, obviously
int main()
{
    // Class objects
    Startup run;
    Game begin;


    run.startup(); // Call startup function
    begin.game(); // Call main game loop function

    return 0;
}

Startup.cpp

#include <iostream>
#include "startup.h"

using namespace std;

// Displays introductory text
void Startup::startup()
{
    cout << "Welcome to Artillery!\n\n";
    cout << "You are in the middle of a war and being charged by thousands of enemies!\n";
    cout << "You have one cannon, which you can shoot at any angle.\n";
    cout << "You only have 10 cannonballs for this target.\n\n";
}
\$\endgroup\$
1
  • \$\begingroup\$ This seems sooo long for such a simple game, it looks like you overengeneered it. Just a first impression. \$\endgroup\$
    – Caridorc
    Aug 15 '15 at 16:54
3
\$\begingroup\$

Why does Main.cpp have namespace std? I don't see it used anywhere


DistAng.cpp

#include <math.h>

I would prefer to use the c++ header, which is


using namespace std;

This could be move into the function, though I'm fuzzy on if it makes a difference or not, I'd need clarification that it is a good idea to stop namespace pollution


// Calculates distance and angle of shot

The fact it calculates the distance is important, that it converts from degrees to radians is not, maybe something like

// Takes in the angle in degrees and returns the distance of the shot

// Converts angle from std::cin into radians

I would prefer

// Degrees to radians formula

As it is short and sweet, but very clear in what it does


// A bit of simple physics to calculate the distance of shot
timeInAir = (2.0 * velocity * sin(inAngle)) / gravity;
shotDistance = round((velocity * cos(inAngle)) * timeInAir);

Maybe a note that this formula assumes that the cannon is on ground level, it is a simplification of the formula for range

Now this is up to you, I would just write the formula in comments, and remove the timeInAir

/*  Assumes the cannon is at ground level
    durationOfFlight = 2 * velocity * sin(x) / gravity
        t = 2*v*sin(x)/g
    distance = v*t*cos(x) 
        = (2 / g) * v*v * sin(x) * cos(x)
*/
return round((2.0 / g) * velocity * velocity * sin(inAngle) * cos(inAngle)); 

But this may not be everyone's cup of tea, and your way is perfectly readable


Game.cpp

calculate.distAng(inAngle)

You call this 3 or 4 times a round, put it in a variable so it is called once and only once a round.

I would change the logic a little bit, check if there is no difference between the shot and the enemy

int ballDistance = calculate.distAng(inAngle);
int difference = enemyDistance - ballDistance;

// If there is no distance between the ball and the enemy, its a hit
if (difference == 0)
{
    killed++;
    killedEnemy = true;
    cout << "\nYou hit him!\n";
    cout << "It took you " << shots << " shots.\n";
    break;
}
// Tell the player the distance between the ball and the enemy
else if (difference > 0)
    cout << "\nYou over shot by " << difference << endl;
else //no need to check, we know it is not > 0 or == 0, so it has to be < 0
    cout << "\nYou under shot by " << -difference << endl;
\$\endgroup\$
2
  • \$\begingroup\$ Thank you, @spyr03! You're input is much appreciated. What I took from it most was your modification to Game.cpp. Calling distAng(); only once is much more efficient. I was also curious about my comments. You definitely helped there as well. Final thing, seeing as you did not mention my classes I'm assuming they were implemented properly. Again, thank you very much! \$\endgroup\$ Aug 15 '15 at 15:23
  • \$\begingroup\$ @MatthewWalls no problem, I'm still learning too. I came from the maths side to computer science, so its nice to work with formulas and expressions instead of implementation improvements \$\endgroup\$
    – spyr03
    Aug 15 '15 at 15:25
9
\$\begingroup\$

I see a number of things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It isn't necessarily wrong to use, but be aware of when you absolutely shouldn't do it (such as in header files).

Make sure you have all required #includes

The code uses std::numeric_limits in Game.cpp but doesn't #include <limits>. Also, srand is used in multiple places, but <cstdlib> is not included.

Use C++ style includes

Instead of including time.h you should instead use #include <ctime>. The difference is in namespaces as you can read about in this question.

Use consistent naming

I would expect the header file for an object named HighScore that is defined in HighScore.cpp to be named HighScore.h but strangely, the header files all have lowercase versions of the corresponding implementation files.

Separate I/O from program logic

Right now most functions have both game logic and I/O. It's often better design to separate the two so that the game logic is independent of the I/O with the user. That way, if you wanted, say, to port the code to a GUI, only the I/O portions of the code would need to revised, and the game logic could stay identical.

Consider consolidating objects

The Startup class does only one simple thing which is to print a fixed string to std::cout. To me, it would make much more sense for that to simply be done in main (without any object) or perhaps as a member function of Game.

Consider refactoring object interfaces

The HighScore class has only a single function in its interface, which is a somewhat badly named function called highScore(int). The text description of the class says that it reads/saves the high score from/to a file. It may make more sense to actually have separate functions that are named read and save rather than one big function that does both. Even if you elect to keep the one big function, a name such as update would make more sense, and could use the read and save functions.

Don't duplicate important constants

The filename in HighScore.cpp is hardcoded right now, but worse than that, it's done in two completely indpendent places. Better would be to create a constant:

static const char *FILENAME = "HighScore.art";

Use string concatenation

The Startup.cpp file includes these lines:

cout << "Welcome to Artillery!\n\n";
cout << "You are in the middle of a war and being charged by thousands of enemies!\n";
cout << "You have one cannon, which you can shoot at any angle.\n";
cout << "You only have 10 cannonballs for this target.\n\n";

Each of those is a separate call to operator<< but they don't need to be. Another way to write that would be like this:

std::cout << "Welcome to Artillery!\n\n"
"You are in the middle of a war and being charged by thousands of enemies!\n"
"You have one cannon, which you can shoot at any angle.\n"
"You only have 10 cannonballs for this target.\n\n";

This reduces the entire thing to a single call to operator<< because consecutive strings in C++ (and in C, for that matter) are automatically concatenated into a single string by the compiler.

Try to consolidate loop exit conditions at the top

Prefer a for or while loop to do {...} while() loop. Because the reader is forced to search for the loop terminating condition, it's more effort to understand the latter construct. The do {...} while() loops in Game.cpp are particularly long and could easily be restructured to put the exit condition at the top.

Consider using a better random number generator

If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::uniform_real_distribution and friends in the <random> header.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

Simplify the mathematics

The code really doesn't need the DistAng object. Instead, I would refactor that calculation as a simple member function of the Game object. Since I think Game is an entirely too generic name, I decided to call it CannonGame instead. The formulas that you're using are these:

$$ t = \frac{2 v_0 \sin \theta}{g} $$ $$ d = v_0 t \cos \theta $$ A little algebra can simplify this: $$ d = \frac{2 v_0 \sin \theta}{g} v_0 \cos \theta $$ $$ d = \frac{2 v_0^2 \sin \theta \cos \theta}{g} $$ Since \$2 \sin\theta \cos\theta = \sin(2\theta)\$, this yields $$ d = \frac{v_0^2 \sin(2 \theta)}{g} $$ Since we are converting from degrees to radians for the calculation, the code is this:

constexpr double velocity = 200.0;
constexpr double gravity = 32.2; 
constexpr double pi = 3.1415926;

int CannonGame::cannonDistance(double degrees) const {
    return velocity * velocity / gravity * std::sin(degrees * pi / 90);
}   
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Your cannondistance function could be made more clear with a few (). \$\endgroup\$
    – akaltar
    Aug 16 '15 at 10:55
  • \$\begingroup\$ @Edward Thank for your input. Definitely encouraging information for me. Anything that helps better my code is always a plus. \$\endgroup\$ Sep 1 '15 at 1:37
5
\$\begingroup\$

There were a few things I noticed you could do a bit differently:

First I would suggest having a naming convention for member variables. Some people like "m" other people like using underscore, its entirely up to you but it makes the source of the variable a bit clearer.

For example, instead of highscore = value, you could have mHighscore = value.

DistAng:

pragma once and ifndef define:

This is a point of debate for different people (and I am probably going to get burned at the stake for this), but personally I dislike using both. I feel if you know your compiler and it definitely works with #pragma once, just use #pragma once. It's entirely your choice though.

I would name this class differently, as it isn't immediately clear what this class does, it's also not clear what the distAng method does.

distAng() could be self documenting and ideally should describe an action, consider "verbs", for example renaming it to something like calculateDistanceFromAngle.

The inAngle parameter doesn't specify if the angle is in degrees or radians, consider renaming inAngle to something like angleDegrees.

Consider creating a utils class and provide the degree->radian conversion as a re-usable static member.

Having Pi as a private member variable doesn't really make sense since Pi is never going to be any other value. You could make it a constant and move it into the new utils class.

gravity could also be a constant instead of a variable.

double distAng(double);:

Personal preference more than a rule, but consider giving the parameter a name, while it isn't needed, providing it means other coders can tell at a glance what this method expects to receive, it also displays in IntelleSense.

All of the variables in this class are private yet never read anywhere. Should they really be members?

I would consider internalizing the many of the variables as local variables inside the distAng method, or making them optional parameters and make the distAng a static member function.

Consider eliminating using namespace std;.

Game:

game():

Consider renaming this to something more descriptive. From outside this class, I have no way of knowing what it does or that it enters into a loop.

It was already mentioned, but I personally also dislike do-while loops.

game():

This method is used a lot and is quite chaotic. Try to isolate "actions" and extract them into private methods. For example, the rng method could be extracted.

 // Creating a couple class object here
DistAng calculate;
HighScore submit;

The comment here is probably redundant. I can see you are creating some objects, but the comment could describe why you are creating them. These also could potentially be members of the game Class, then they wouldn't need to be recreated every update.

Try to keep the user IO, game logic, and logging separate wherever you can. The loop that deals with killingEnemies is doing quite a lot of very different things.

inAngle

This value could potentially be more descriptive. I would also assume that inAngle would be a parameter if I didn't look at the rest of the code.

Could the input check here be extracted into a separate method?

if (enemyDistance == calculate.distAng(inAngle)):

I would be concerned about the double to int comparison here, it would be worth checking to see if the distance of the shot is within a certain range of the enemy distance instead, either that or explicitly floor the distAng.

killed:

Perhaps this variable could be more descriptive.

If the first condition of an if statement uses braces, it's good practice to provide them for the else, even if it is just a one line check, it keeps things consistent, and makes them a bit clearer to read.

Consider making all of the strings you cout into constants and putting them at the top of the class instead. It's not really needed or anything, and I'm probably being a bit obsessive, but it just keeps them together, if you wish to change the messages at some point. It also means there isn't literal text in your code.

shot count:

The shot count is checked against the number 10 in more than one place. Could the 10 be represented as a constant instead? also could the check be separated into a method?

if (shots == 10 && killedEnemy == false)

It looks like a lot of the code inside this if block could be separated into its own method.

The "yes or no" input check could be separated into its own method. The method could then return a true or a false, then the string checks outside of the method could be simplified to bool checks.

I would expect the "done" member variable, to be a boolean, value, or to be named differently.

I would also suggest the variable doesn't need to be a member, instead consider having an update() method. The update could then return the boolean as an error code instead.

For example:

  bool update()
 {
    return done;
 }
 
 
 while(1)
 {
  if(update() == false)
  {
    break;
  }
 }

The input new game check, and input new round check, are almost identical. If the cout was done first, the input check could be extracted into its own method and re-used in both cases.

HighScore:

Consider renaming the highScore method to an action, for example, logHighScoreToFile.

Consider either passing the filename "HighScore.art" into the method as a parameter, or making it a constant.

As with game.cpp, it is good practice to put brackets on the else if you have provided them for the if, even if there is only one line.

Consider giving the HighScore class a constructor, then reading the highest score from the file on construction. You can check the new score against the member, and avoid opening and reading from the highscore table unless you need to.

Main:

Main doesn't need to include <fstream>.

Run seems like an odd instance name for the Startup class.

Begin seems like a strange instance name for Game. Consider renaming to game, and then renaming the game() method to something more descriptive.

Startup.cpp:

Separating startup into a separate method seems redundant, since it doesn't really "own" anything. Could the startup method simply go into the game class?

Then in main, there could be something like:

game.startup()
game.update()
\$\endgroup\$
1
  • \$\begingroup\$ @NGanst Thanks a bunch! Very good advice. It seems I need to better compartmentalize my code. Maybe I combine tasks into one without realizing. Also, my naming methods could improve. I try not to use long variable names. Apparently, I reduce their descriptive nature as well. Got to find the happy medium! \$\endgroup\$ Sep 1 '15 at 1:50
4
\$\begingroup\$

I think that the way you name all of your classes, variables, and methods reveals a fundamental misunderstanding of OOP. It's basically imperative programming dressed up to look like object-oriented programming. Examples of bad declarations include Startup run;, Game begin;, DistAng calculate;, and HighScore submit;.

Objects should be nouns. Furthermore, you should avoid inventing names that aren't words: instead of DistAng, prefer Trajectory or Shot — not that you need a class for such a simple calculation at all.

Here's an object-oriented outline that you can fill in. Note that Target, Artillery, and Scorekeeper are all tangible nouns.

#include <iostream>
#include <string>

class Target {
  public:
    Target(double min, double max, double tolerance=5);
    double distance() const;
    bool hit(double distance) const;
};

class Artillery {
  public:
    Artillery(int shots);
    int shotsRemaining() const;
    int shotsTaken() const;

    // Reports the overshoot (positive) or undershoot (negative).
    // Any hit that is close enough to the target is reported as 0.
    double fire(const Target &target, double degrees, double speed=200);
};

class Scorekeeper {
  public:
    Scorekeeper();
    void recordKill();
    int killCount() const;
    void submitScore();
};

const std::string INTRO = "…";

double promptDouble(const std::string &prompt);

bool promptBool(const std::string &prompt);

// True if hit the target with the available ammo
bool engage(Artillery &artillery, const Target &target) {
    std::cout << "\nThe enemy is " << target.distance() << " feet away!\n";

    while (artillery.shotsRemaining()) {
        double angle = promptDouble("What angle? ");
        double error = artillery.fire(target, angle);
        if (error == 0) {
            std::cout << "\nYou hit him!\n"
                      << "It took you " << artillery.shotsTaken() << " shots.\n";
            return true;
        } else if (error < 0) {
            std::cout << "\nYou undershot by " << -error << " feet.\n";
        } else if (error > 0) {
            std::cout << "\nYou overshot by " << error << " feet.\n";
        }
    }
    return false;
}

int main() {
    const char *againPrompt;
    Scorekeeper scorekeeper;
    std::cout << INTRO << std::endl;
    do {
        Artillery artillery(10);
        Target target(200, 1100);
        if (engage(artillery, target)) {
            scorekeeper.recordKill();
            std::cout << "You've killed " << scorekeeper.killCount() << " of the enemy.\n";
            againPrompt = "I see another one.  Care to shoot again? (Y/N) ";
        } else {
            std::cout << "You ran out of cannonballs!\n"
                      << "You've killed " << scorekeeper.killCount() << " of the enemy.\n";
            scorekeeper.submitScore();
            againPrompt = "Play again? (Y/N) ";
        }
    } while (promptBool(againPrompt));
}

I find your game loop logic to be rather convoluted. It helps to extract each artillery-target engagement into a separate function and to make the ammunition counter an instance variable of the artillery unit.

Note that I've included a 5-foot tolerance for hitting the target, since double-precision numbers almost never match exactly, and a 5-foot tolerance seems realistic.

\$\endgroup\$
1
  • \$\begingroup\$ This was eye-opening for sure. Thanks a bunch! \$\endgroup\$ Sep 1 '15 at 2:01

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.