4
\$\begingroup\$

In order to help with learning C++, I have been re-implementing games from old ZX80 books in C++. Here' the description of one I'm currently working on:

You are a starship captain. You have crashed your ship on a strange planet and must take off again quickly in the alien ship you have captured. The ship's computer tells you the gravity on the planet. You must guess the force required for a successful take off. If you guess too low, the ship will not lift off the ground. If you guess too high, the ship's fail-safe mechanism comes into operation to prevent it being burnt up. If you are still on the planet after ten tries, the aliens will capture you.

Any feedback on whether my implementation is good and idiomatic C++, and if not how it could be improved, would be greatly appreciated.

It is probably over-engineered for what it is, but I tried to use it as an exercise in things like:

  • Operator overloading
  • Using the type system to help with validating user input (see the Guess class.
  • I haven't included the unit tests here, but the code was written so that the main logic could be tested.

game.h

#ifndef GAME_H
#define GAME_H

#include <string>

enum class GuessResponse { TooLow, TooHigh, TakeOff, GameOver };

std::ostream& operator<<(std::ostream& os, const GuessResponse response);

class CountDown {
public:
    CountDown(int start);
    void decrement();
    bool is_finished() const;

private:
    int value_;
};

class Guess final {
public:
    Guess() {};
    Guess(int value);
    void setValue(int value);
    friend bool operator>(const Guess& guess, const int other);
    friend bool operator<(const Guess& guess, const int other);
    friend bool operator>=(const Guess& guess, const int other);
    friend bool operator<=(const Guess& guess, const int other);
    friend bool operator==(const Guess& guess, const int other);
    friend bool operator!=(const Guess& guess, const int other);
private:
    int value_{1};
};

std::istream& operator>>(std::istream& is, Guess& guess);

class SpaceTakeoffGame {
public:
    SpaceTakeoffGame(int gravity, int weight);
    GuessResponse make_guess(const Guess& guess);
    bool over() const;

private:
    const int gravity_;
    const int weight_;
    const int force_;
    CountDown tries_remaining_{10};
    bool over_{false};

};

#endif

game.cpp

#include "game.h"
#include <string>
#include <stdexcept>
#include <ios>
#include <istream>

SpaceTakeoffGame::SpaceTakeoffGame(const int gravity, const int weight)
    : gravity_{ gravity },
      weight_{ weight },
      force_{ weight * gravity } {}

GuessResponse SpaceTakeoffGame::make_guess(const Guess& guess) {
    if (tries_remaining_.is_finished()) {
        over_ = true;
        return GuessResponse::GameOver;
    }
    tries_remaining_.decrement();
    if (guess > force_) {
        return GuessResponse::TooHigh;
    }
    if (guess < force_) {
        return GuessResponse::TooLow;
    }
    over_ = true;
    return GuessResponse::TakeOff;
}

bool SpaceTakeoffGame::over() const {
    return over_;
}

CountDown::CountDown(const int start): value_{ start } {}

void CountDown::decrement() {
    if (value_ > 0) {
        value_ -= 1;
    }
}

bool CountDown::is_finished() const {
    return value_ == 0;
}

std::ostream& operator<<(std::ostream& os, const GuessResponse response) {
    switch(response) {
        case GuessResponse::TooHigh: {
            os << std::string{"TOO HIGH, TRY AGAIN"};
            break;
        }
        case GuessResponse::TooLow: {
            os << std::string{"TOO LOW, TRY AGAIN"};
            break;
        }
        case GuessResponse::TakeOff: {
            os << std::string{"GOOD TAKE OFF"};
            break;
        }
        default: {
            os << std::string{"YOU FAILED - THE ALIENS GOT YOU"};
            break;
        }
    }
    return os;
}

Guess::Guess(int value) {
    setValue(value);
}

void Guess::setValue(int value) {
    if (value < 1) throw std::invalid_argument{ "Guess must be a positive integer" };
    value_ = value;
}

std::istream& operator>>(std::istream& is, Guess& guess) {
    int n;
    is >> n;
    if (is.fail()) {
        return is;
    }
    try {
        guess.setValue(n);
    } catch (std::invalid_argument&) {
        is.setstate(std::ios::failbit);
    }
    return is;
}

bool operator>(const Guess& guess, const int other) {
    return guess.value_ > other;
}

bool operator<(const Guess& guess, const int other) {
    return guess.value_ < other;
}

bool operator>=(const Guess& guess, const int other) {
    return !(guess < other);
}

bool operator<=(const Guess& guess, const int other) {
    return !(guess > other);
}

bool operator==(const Guess& guess, const int other) {
    return !(guess < other || guess > other);
}

bool operator!=(const Guess& guess, const int other) {
    return !(guess == other);
}

main.cpp

#include <iostream>
#include <random>
#include <limits>
#include <ios>
#include "game.h"

int main()
{
    std::random_device rd;
    std::mt19937_64 eng(rd());

    std::uniform_int_distribution<int> gravity_distr{1, 20};
    std::uniform_int_distribution<int> weight_distr{1, 40};

    const auto gravity =  gravity_distr(eng);
    const auto weight =  weight_distr(eng);

    SpaceTakeoffGame game(gravity, weight);

    std::cout << "STARSHIP TAKE-OFF\n"
              << "GRAVITY=" << gravity << "\n"
              << "TYPE IN FORCE" << std::endl;
    do {
        Guess guess;
        std::cin >> guess;
        if (std::cin.fail()) {
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
            std::cout << "INVALID GUESS, TRY AGAIN" << std::endl;
            continue;
        }
        std::cout << game.make_guess(guess) << std::endl;
    } while (!game.over());
    return 0;
}
```
\$\endgroup\$
2
\$\begingroup\$

Define simple member functions in the header files

If you define member functions inside the class definition, they can be inlined by the compiler. For simple functions, like CountDown::is_finished(), this will generate much more efficient code, even on a Z80.

Unnecessary casts to std::string

There are several unnecessary casts to std::string in your code, for example:

os << std::string{"TOO HIGH, TRY AGAIN"};

You can just write this as:

os << "TOO HIGH, TRY AGAIN";

Choice of random number generator

While std::mt19937_64 is a very nice random number generator, it has a very large internal state of 19937 bits (hence its name), which is 2493 bytes. This is a huge amount of space to waste on an 8-bit machine. It also uses 64-bit integer arithmetic, which is less desirable on an 8-bit machine. On the other hand, it is one of the faster random number generator algorithms available in the standard library.

If space is of a premium, then consider using a simpler RNG, in particular the linear congruential based ones only need a handful of bytes. For a game like this, you don't need a high quality RNG.

Avoid using exceptions for handling input errors

Exceptions do have some cost associated to them, even if you don't cause anything to throw. The compiler must generate the required code or data to be able to handle stack unwinding. On a regular PC, I would normally consider that an acceptable price to pay, but on a Z80 machine this price will be relatively high. Exceptions should also be used only in very exceptional situations that the code cannot deal with, but in this case it is quite trivial to handle invalid inputs without having to throw. There are several ways to handle this, I would just remove setValue(), and make operator>> a friend function, like so:

class Guess {
public:
    ...
    friend std::istream& operator>>(std::istream& is, Guess& guess);
    ...
};

std::istream& operator>>(std::istream& is, Guess& guess) {
    int n;

    if ((is >> n) && n >= 1) {
        guess.value_ = n;
    } else {
        is.setstate(std::ios::failbit);
    }

    return is;
}            
\$\endgroup\$
2
  • \$\begingroup\$ Good catch on the random number generator. I overlooked that entirely. \$\endgroup\$
    – Edward
    May 2 at 16:15
  • \$\begingroup\$ Thanks. Just to clarify, I'm not writing this for ZX80 (didn't even know you could write C++ for ZX80), I'm just using the projects as code "writing prompts". \$\endgroup\$
    – samfrances
    May 5 at 18:24
2
\$\begingroup\$

Here are some things that may help you improve your code.

Make sure you have all required #includes

The code in game.h refers to std::ostream and std::istream but doesn't #include <iostream> where those are defined. Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation. The <string> header is only needed in game.cpp and not in game.h.

Not everything needs to be a class

Learning about object-oriented programming is useful, but it's also worth noting that sometimes a class is not necessarily the best approach. The Guess class is a rather elaborate wrapper around a plain int. I think I'd just use an int. The same is true of CountDown.

Don't save values you don't need

The SpaceTakeoffGame constructor takes gravity and weight as arguments and calculates force_ from that. First, from a physics standpoint, that should be mass rather than weight. Second, neither gravity nor weight are ever used again, so there's not much need to save them.

Don't use exceptions to validate user input

An exception should be exceptional. Having a user input a negative number is not particularly exceptional; I'd suggest just checking the value without using exceptions.

Don't construct objects needlessly

The code currently contains this line:

os << std::string{"TOO HIGH, TRY AGAIN"};

This forces the compiler to create a string, pass it to the stream and then destroy the string. Better would be to just write this:

os << "TOO HIGH, TRY AGAIN";

Avoid unnecessary complexity

Since this is a very simple game, I would probably avoid all of the classes and put the whole game into a simple function.

#include <iostream>
#include <limits>
#include <random>

void play(int force) {
    bool takeoff{false};
    for (unsigned tries{10}; !takeoff && tries; --tries) {
        int guess{0};
        while (guess < 1) {
            std::cin >> guess;
            if (guess < 1) {
                if (std::cin.fail()) {
                    std::cin.clear();
                    std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
                }
                std::cout << "INVALID GUESS, TRY AGAIN" << std::endl;
            }
        }
        if (guess == force) {
            takeoff = true;
        } else if (guess < force && tries > 1) {
            std::cout << "TOO LOW, TRY AGAIN\n";
        } else if (guess > force && tries > 1) {
            std::cout << "TOO HIGH, TRY AGAIN\n";
        }
    } 
    std::cout << (takeoff ? "GOOD TAKE OFF\n" :
        "YOU FAILED - THE ALIENS GOT YOU\n");
}

int main() {
    std::random_device rd;
    std::mt19937 eng(rd());

    std::uniform_int_distribution<int> gravity_distr{1, 20};
    std::uniform_int_distribution<int> mass_distr{1, 40};

    const auto gravity{gravity_distr(eng)};
    const auto force{gravity * mass_distr(eng)};

    std::cout << "STARSHIP TAKE-OFF\n"
              "GRAVITY=" << gravity 
              << "\nTYPE IN FORCE\n";
    play(force);
}
\$\endgroup\$

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.