10
\$\begingroup\$

I guess I'm a little late with my entry to this code challenge mentioned here, but I decided the world must need yet another RPSLS implementation to critique.

#include <vector>
#include <string>
#include <iostream>
#include <stdlib.h>
#include <algorithm>
#include <time.h>

/* Here are your rules:
"Scissors cuts paper,
paper covers rock,
rock crushes lizard,
lizard poisons Spock,
Spock smashes scissors,
scissors decapitate lizard,
lizard eats paper,
paper disproves Spock,
Spock vaporizes rock.
And as it always has, rock crushes scissors."
-- Dr. Sheldon Cooper */

class gesture {
    size_t val;
    static char const *names[5];
public:
    gesture() : val(rand() % 5) {}

    bool operator<(gesture const &other) const {
        bool winners [5][5] = {
            /* rock */  /* paper */ /* scissors */  /* lizard */    /* Spock */
            /*rock*/    { false, true, false, false, true },
            /*paper*/{ false, false, true, false, false },
            /*scissors*/{ true, false, false, false, true },
            /*Lizard */{ true, false, true, false, false },
            /* spock */ { false, true, false, true, false }
        };

        return winners[val][other.val];
    }

    friend std::ostream &operator<<(std::ostream &os, std::pair<gesture, gesture> const &p) {
        char const *titles [5][5] = {
                    /* rock */  /* paper */ /* scissors */  /* lizard */    /* Spock */
            /*rock*/    { "ties", "covers", "crushes", "crushes", "vaporizes" },
            /*paper*/   { "covers", "ties", "cuts", "eats", "disproves" },
            /*scissors*/{ "crushes", "cuts", "ties", "decapitates", "smashes" },
            /*Lizard */{ "crushes", "eats", "decapitates", "ties", "Poisons" },
            /* spock */{ "vaporizes", "disproves", "smashes", "Poisons", "Ties" }
        };

        gesture const &a = std::max(p.first, p.second);
        gesture const &b = std::min(p.first, p.second);
        return os << a << " " << titles[a.val][b.val] << " " << b;
    }

    friend std::istream &operator>>(std::istream &is, gesture &g) {
        char const **pos;
        std::string user_choice;
        do {
            std::getline(std::cin, user_choice);
        } while ((pos = std::find(std::begin(names), std::end(names), user_choice)) == std::end(names)
            && std::cout << "Unrecognized choice. Please re-enter: ");
        g.val = pos - std::begin(names);
        return is;
    }

    friend std::ostream &operator<<(std::ostream &os, gesture const &g) {
        return std::cout << names[g.val];
    }
};

char const *gesture::names[5] = { "rock", "paper", "scissors", "lizard", "Spock" };

int main() {
    srand(time(NULL));
    std::vector<size_t> scores(3);
    std::string again;

    do {
        gesture user;
        std::cout << "Choose your weapon: ";
        std::cin >> user;

        gesture computer;

        std::cout << "user: " << user << "\tcomputer: " << computer << "\n";

        std::cout << std::make_pair(user, computer) << "\n";

        if (user < computer)  
            ++scores[0];        
        else if (computer < user) 
            ++scores[1];        
        else 
            ++scores[2];        
        std::cout << "Score: Computer: " << scores[0] << "\tYou: " << scores[1] << "\tties: " << scores[2] << "\n";
        std::cout << "Play again (y/n)?";
        std::getline(std::cin, again);
    } while (again == "y");
}

I do expect some real critiques here--this code definitely isn't perfect. :-)

\$\endgroup\$
  • \$\begingroup\$ Safe to assume you're not using C++11? \$\endgroup\$ – Jamal Mar 7 '14 at 21:20
  • \$\begingroup\$ @Jamal: I don't think it currently requires C++11, but suggestions to improve it that use C++11 are entirely welcome. \$\endgroup\$ – Jerry Coffin Mar 7 '14 at 21:24
  • \$\begingroup\$ You're much more experienced than me, so I don't think I'll have anything new to add. Plus, the only thing that sticks out to me now, regarding C++11, is NULL. \$\endgroup\$ – Jamal Mar 7 '14 at 21:31
  • \$\begingroup\$ Nitpick: <ctime> instead of <time.h>, same for stdlib \$\endgroup\$ – TemplateRex Mar 15 '14 at 12:03
7
\$\begingroup\$

Running it through g++ using -Wall -Wextra we see:

rpsls.cpp:66:26: warning: unused parameter 'os' [-Wunused-parameter]
friend std::ostream &operator<<(std::ostream &os, gesture const &g) {

Which is a bit of a "whoopsie":

 friend std::ostream &operator<<(std::ostream &os, gesture const &g) {
    return std::cout << names[g.val];
           ^^^^^^^^
         // Should be os
}

I'd prefer to use std::array<std::string> over char * arrays, mainly because this way there's no mucking around with both std::string and char * (or char **):

const static std::array<std::string, 5> names;

const std::array<std::string, 5> gesture::names = { "rock", "paper", "scissors", "lizard", "Spock" };

This also changes std::istream& operator>> a bit: you can either do the (verbose):

std::array<std::string, 5>::const_iterator pos;

Or if you're feeling a little bit lazy:

decltype(&names[0]) pos;

I agree with Loki that I'm not a big fan of using operator< to make a decision, and I'd rather make a named function (like compare).

I think there's a bug: Lizard is supposed to eat paper, so this row:

/*paper*/ { false, false, true, false, false }

should probably be:

/*paper*/ { false, false, true, true, false }

I'm actually not a big fan of the 2D array of bool to decide on a winner. Sure, it's simple, but it's a bit of a pain to read. There's also a separation between winners and titles when they have the same structure, just different types.

I'd change this to use an enum class for the outcome, and a combination of outcome and title for the result (these should probably be private to the gesture class):

enum class outcome { WIN, DRAW, LOSS };

struct result
{
    outcome out;
    std::string title;
};

(The naming of these is actually pretty poor, I admit, but I made sure my changes compile with this, and I can't be bothered changing them to something better now). We can then define a map of possibilities, and exploit the fact that they are antisymmetric in win/loss: if A wins against B, then necessarily B loses against A. So we can cut down on the number of possibilities we need to cover (and also gives us less chance to make mistakes):

using string_pair = std::pair<std::string, std::string>;
static std::map<string_pair, result> possibilities; 

std::map<typename gesture::string_pair, result> gesture::possibilities = 
{
    {{"rock", "paper"}, {outcome::LOSS, "covers"}},
    {{"rock", "scissors"}, {outcome::WIN, "crushes"}},
    {{"rock", "lizard"}, {outcome::WIN, "crushes"}},
    {{"rock", "Spock"}, {outcome::LOSS, "vaporizes"}},
    {{"paper", "scissors"}, {outcome::LOSS, "cuts"}},
    {{"paper", "lizard"}, {outcome::LOSS, "eats"}},
    {{"paper", "Spock"}, {outcome::WIN, "disproves"}},
    {{"scissors", "lizard"}, {outcome::WIN, "decapitate"}},
    {{"scissors", "Spock"}, {outcome::LOSS, "smash"}},
    {{"lizard", "Spock"}, {outcome::WIN, "poisons"}}
};

I'd rather a gesture had a name rather than a val that is tied to an array value:

class gesture {
    std::string name;
    const static std::array<std::string, 5> names;

    using string_pair = std::pair<std::string, std::string>;
    static std::map<string_pair, result> possibilities; 

public:
    gesture() : name(names[rand() % names.size()]) { }
    explicit gesture(const std::string& nm) : name(nm) { }

    ...
}

This makes the comparison function a bit more complex, but I think it's worth it:

result compare(gesture const& other) const {
    if (name == other.name) {
        return {outcome::DRAW, "ties"};
    }

    auto it = possibilities.find(std::make_pair(name, other.name));
    if(it == possibilities.end()) {
        // Wasn't in our map, so parameters must be in the opposite order
        auto res = possibilities[std::make_pair(other.name, name)];
        // Don't forget to invert the result
        return {(res.out == outcome::WIN ? outcome::LOSS : outcome::WIN),
                res.title};
    }
    return it->second;
}

operator<< also changes a bit:

friend std::ostream &operator<<(std::ostream &os, std::pair<gesture, gesture> const &p) {
    result r = (p.first).compare(p.second);
    if(r.out == outcome::WIN || r.out == outcome::DRAW) {
        return os << p.first << " " << r.title << " " << p.second;
    }
    return os << p.second << " " << r.title << " " << p.first;
}
\$\endgroup\$
  • \$\begingroup\$ Lizards eat bugs too don't they? \$\endgroup\$ – Malachi Mar 11 '14 at 19:00
6
\$\begingroup\$

Not sure I like the use of operator< to test for a winner.

bool operator<(gesture const &other) const

Here

   if (user < computer)  
        ++scores[0];        
    else if (computer < user) 
        ++scores[1];        
    else 
        ++scores[2];        

I suppose it works but not that intuitive. I would have gone for a named method.

Your data stores of constant data could also be static.

static bool winners [5][5] 

static char const *titles

I doubt it makes a difference. But semantically it reads better.

\$\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.