3
\$\begingroup\$

I solved an online judge problem named Jollo. The full description of the problem can be found here: https://www.urionlinejudge.com.br/judge/en/problems/view/1321

It's a card game between 2 players.

Each player is given 3 cards.

Each round a player shows one card and removes it from hand.

The player who shows the highest card wins the round.

Prince and Princess play but Prince is really bad and cries loudly so the servant who deals the cards, after dealing Princess her 3 cards, and dealing Prince 2 cards, he deals Prince the lowest higher card available that no matter how badly he plays he ends up winning. If there is none return -1.

My logical approach is that:

  1. If Princess wins first 2 rounds, there's nothing to be done return -1
  2. If Prince wins first 2 rounds, return the lowest available card on deck
  3. If each wins 1 of the first 2 rounds, if there is an available card on deck higher than princess' remaining card return it, if not return -1

Each round logic, so that Prince always plays the worst possible game is achived via:

  1. Prince will always show his card first, he will play his highest card on hand
  2. If Princess does not have a higher card than Prince played she will play her lowest. If she does, she will player her lowest higher card.

E.g:

Princess cards: 5, 4, 3

Prince cards: 6, 1

Prince plays: 6 - Princess doesn't have a higher, play lowest: 3

Prince plays: 1- Princess has two higher cards, play lowest higher card: 4.

Tie: Princess remaining card is 5 therefore the next card available on deck to beat her is 7.

Now for the code review part. What could I do better? Is this code somewhat similar in what would be used in a professional environment?

(I know there is some hard coding to this since it's only being used to a very specific problem and that instead of having 2 hardcoded players prince and princess a more general approach would be to have a vector of players etc. I'm asking about the general code design/organization overall).

#include <iostream>
#include <sstream>
#include <array>
#include <algorithm>
#include <numeric>
#include <utility>

//used as flags
enum JolloPlayer
{
    PRINCE = 100,
    PRINCESS = 101,
    NONE = 102
};

template<int C> struct Player
{
    std::array<int,C>cards;
    friend std::istream& operator>>(std::istream& is, Player& p)
    {
        for(auto& i : p.cards){
            is >> i;
        }
        return is;
    }
    int amountOfCards = C;
};

template<unsigned int N>
class Jollo
{
private:
    //amount of cards, not player number
    Player<2> prince;
    Player<3> princess;
    std::array<int, N> deck;
    const int roundsToWin {2};
public:
    Jollo<N>(){
        static_assert(N>4, "Jollo: deck size must be higher than 4");
    }

    bool ReadCards();
    int GetPrinceMinimumWinningCard();
private:
    int GetNextAvailableCard(JolloPlayer ePlayer, int lastPlayedCard = 0); //gets next card and flips deck
};

template<unsigned int N>bool Jollo<N>::ReadCards()
{
    std::string line;
    std::getline(std::cin, line);
    std::istringstream issline(line);
    issline >> princess;
    issline >> prince;
    return (princess.cards[0] != 0);
}

template<unsigned int N>int Jollo<N>::GetNextAvailableCard(JolloPlayer ePlayer, int lastPlayedCard)
{
    if(ePlayer == JolloPlayer::PRINCE)
    {
        for(int i{0}; i<prince.amountOfCards; i++) {

            if(deck[prince.cards[i] - 1] != JolloPlayer::PRINCE){
               deck[prince.cards[i] - 1] =  JolloPlayer::PRINCE;
               return prince.cards[i];
            }
        }
    } else
    if(ePlayer == JolloPlayer::PRINCESS)
    {
        for(int i{0}; i<princess.amountOfCards; i++) {
            if(deck[princess.cards[i] - 1] != JolloPlayer::PRINCESS && deck[princess.cards[i] - 1] > lastPlayedCard) {
               deck[princess.cards[i] - 1] =  JolloPlayer::PRINCESS;
               return princess.cards[i];
            }
        }
        //no card was higher, return lowest available card
        for(int i{0}; i<princess.amountOfCards; i++) {
            if(deck[princess.cards[i] - 1] != JolloPlayer::PRINCESS) {
               deck[princess.cards[i] - 1] =  JolloPlayer::PRINCESS;
               return princess.cards[i];
            }
        }
    }

    //ePlayer == NONE
    for(unsigned int i{0}; i<N; i++)
    {
        if(deck[i] != JolloPlayer::PRINCE && deck[i] != JolloPlayer::PRINCESS && deck[i] > lastPlayedCard ) {
            return deck[i];
        }
    }
    return -1; //if the game is tied but there is no higher card available. e.g 50 49 51 48 52
}

template<unsigned int N>int Jollo<N>::GetPrinceMinimumWinningCard()
{
    std::iota(deck.begin(), deck.end(), 1); //must be re-set every time, as indexes are set to PRINCE or PRINCESS

    std::sort(prince.cards.begin(), prince.cards.end(), std::greater<int>());   //decreasing
    std::sort(princess.cards.begin(), princess.cards.end());                    //increasing

    int princeWins {0};
    int princessWins {0};

    for(int round {0}; round<roundsToWin; round++) //play two first rounds
    {
        int princeCard = GetNextAvailableCard(JolloPlayer::PRINCE);
        int princessCard = GetNextAvailableCard(JolloPlayer::PRINCESS, princeCard);

        if(princessCard > princeCard){
            princessWins++;
        } else {
            princeWins++;
        }
    }
    int lastPrincessCard = GetNextAvailableCard(JolloPlayer::PRINCESS); //important to flip the last card on the deck before continuing
    if(princessWins == roundsToWin){
        return -1;
    }
    if(princeWins == roundsToWin){
        return GetNextAvailableCard(JolloPlayer::NONE);
    }
    return GetNextAvailableCard(JolloPlayer::NONE, lastPrincessCard);
}

int main()
{
    Jollo<52> JolloInstance;
    while(true)
    {
        if(!JolloInstance.ReadCards()) {
            break;
        }
        std::cout << JolloInstance.GetPrinceMinimumWinningCard() << "\n";
    }
    return 0;
}

Some test cases as requested.

3 2 1 5 4   o:6
6 1 3 4 5   o:7
6 2 3 4 7   o:5
5 3 2 6 7   o:1
5 3 2 6 4   o:7
5 3 1 2 4   o:-1
1 2 3 4 5   o:6
3 2 1 4 5   o:6
3 2 1 4 7   o:5
1 2 3 5 4   o:6
6 2 3 1 5   o:-1
52 1 2 3 50 o:4
52 1 3 4 50 o:5
4 2 52 3 50 o:-1
52 48 47 49 50  o:51
52 49 48 50 51  o:-1
48 49 52 51 50  o:-1
48 49 50 51 52  o:1
1 49 50 51 52   o:2
2 49 50 51 52   o:1
0 0 0 0 0
\$\endgroup\$
0
3
\$\begingroup\$

Preface: A word about competitive programming

Now for the code review part. What could I do better? Is this code somewhat similar in what would be used in a professional environment?

(I know there is some hard coding to this since it's only being used to a very specific problem and that instead of having 2 hardcoded players prince and princess a more general approach would be to have a vector of players etc. I'm asking about the general code design/organization overall).

Actually, this is real problem. Competitive programming sites fail to teach you skills you need to do a real world project. In general, learning C++ from "competitive programming" is like learning English from a rap contest. I suggest that you start your own scalable project and go through the design process. That will be pretty fun :)

In fact, I'd go ahead and say that the program is created to solve a very specific problem and that the generic code review method hardly applies to it.

From comment:

Do you think it's good to continue solving these problems, or by being focused on getting a good level of C++ I should focus on something else?

It's fine to do some CP occasionally (for fun, maybe), but that shouldn't be your primary focus. Remember that doing CP does not help you learn real programming a whole lot. Instead, do some relatively large-scale, long-term projects to learn more about maintaining code. And you are always free to post on Code Review. The Definitive C++ Book Guide and List may also be helpful to you.

Use consistent spacing

This is my first impression: the spacing is inconsistent and follows an unpopular style. Here is the (in general) standard style:

  • Template declarations should look like this:

    template <typename T>
    class C;
    

    not

    template<typename T> class C;
    
  • Member declarations should look like this:

    std::array<int, N> arr;
    

    not

    std::array<int,N>arr;
    
  • Loops should look like this:

    for (auto& x : y) {
    

    not

    for(auto& x : y){
    
  • Constructors should use the injected class name:

    template <class T>
    struct S {
        S() = default;
    };
    

    not

        S<T>() = default;        
    
  • If chains should be lain out in this way:

    if (...) {
        /* ... */;
    } else if (...) {
        /* ... */;
    }
    

    not

    if(...)
    {
        /* ... */;
    } else
    if(...)
    {
        /* ... */;
    }
    

A walk through

In this section, I select some interesting snippets to comment on.

#include <iostream>
#include <sstream>
#include <array>
#include <algorithm>
#include <numeric>
#include <utility>

Please sort the include directives according to alphabetical order:

#include <algorithm>
#include <array>
#include <iostream>
#include <numeric>
#include <sstream>
#include <utility>

This helps navigation.

//used as flags
enum JolloPlayer
{
    PRINCE = 100,
    PRINCESS = 101,
    NONE = 102
};

The magic numbers 100, 101, and 102 are confusing. Why? At least leave a comment. Also, the enum should probably be named Player_ID.

template<int C> struct Player
{
    std::array<int,C>cards;
    friend std::istream& operator>>(std::istream& is, Player& p)
    {
        for(auto& i : p.cards){
            is >> i;
        }
        return is;
    }
    int amountOfCards = C;
};

int appears for two distinct purposes in this code: card count and card ID. Type aliases make this clearer:

using Card_ID = int;
using Card_count = std::size_t; // counting should be done with std::size_t

Then, I'm not sure this class is even necessary. Keep it simple:

template <Card_count N>
using Cards = std::array<Card_ID, N>;

template <Card_count N>
void read_cards(std::istream& is, Cards& cards)
{
    for (auto& card : cards)
        is >> card;
}
template<unsigned int N>
class Jollo
{
private:
    //amount of cards, not player number
    Player<2> prince;
    Player<3> princess;
    std::array<int, N> deck;
    const int roundsToWin {2};
public:
    Jollo<N>(){
        static_assert(N>4, "Jollo: deck size must be higher than 4");
    }

    bool ReadCards();
    int GetPrinceMinimumWinningCard();
private:
    int GetNextAvailableCard(JolloPlayer ePlayer, int lastPlayedCard = 0); //gets next card and flips deck
};

Now it's unsigned int. That's inconsistent. Use the aforementioned Card_count instead.

roundsToWin should be static constexpr.

template<unsigned int N>int Jollo<N>::GetNextAvailableCard(JolloPlayer ePlayer, int lastPlayedCard)
{
    if(ePlayer == JolloPlayer::PRINCE)
    {
        for(int i{0}; i<prince.amountOfCards; i++) {

            if(deck[prince.cards[i] - 1] != JolloPlayer::PRINCE){
               deck[prince.cards[i] - 1] =  JolloPlayer::PRINCE;
               return prince.cards[i];
            }
        }
    } else
    if(ePlayer == JolloPlayer::PRINCESS)
    {
        for(int i{0}; i<princess.amountOfCards; i++) {
            if(deck[princess.cards[i] - 1] != JolloPlayer::PRINCESS && deck[princess.cards[i] - 1] > lastPlayedCard) {
               deck[princess.cards[i] - 1] =  JolloPlayer::PRINCESS;
               return princess.cards[i];
            }
        }
        //no card was higher, return lowest available card
        for(int i{0}; i<princess.amountOfCards; i++) {
            if(deck[princess.cards[i] - 1] != JolloPlayer::PRINCESS) {
               deck[princess.cards[i] - 1] =  JolloPlayer::PRINCESS;
               return princess.cards[i];
            }
        }
    }

    //ePlayer == NONE
    for(unsigned int i{0}; i<N; i++)
    {
        if(deck[i] != JolloPlayer::PRINCE && deck[i] != JolloPlayer::PRINCESS && deck[i] > lastPlayedCard ) {
            return deck[i];
        }
    }
    return -1; //if the game is tied but there is no higher card available. e.g 50 49 51 48 52
}
  • Use shorter lines. A horizontal scroll would not have shown up if you kept each line less than ~80 characters.

  • Make functions smaller. In general, a vertical scroll should not be needed to view the code of a function.

  • switch is appropriate here.

  • Traversing should be done with a range-based for loop, not with an index. In particular, neither int nor unsigned int is suitable for array indexing.

  • Use ++i, not i++.

int main()
{
    Jollo<52> JolloInstance;
    while(true)
    {
        if(!JolloInstance.ReadCards()) {
            break;
        }
        std::cout << JolloInstance.GetPrinceMinimumWinningCard() << "\n";
    }
    return 0;
}

This can be simpler:

for (Jollo<52> game; game.ReadCards();)
    std::cout << game.GetPrinceMinimumWinningCard() << '\n';

(Note that '\n' is used instead of "\n".)

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much. This is very very helpful. I'll apply all of it to my next programs. As for the competitive programming, when I joined the website, I thought that it was just a simple way of getting better at programming, but then after a while when some of my solutions were too slow because they were done(more or less) the c++ way, I read articles about it and realized what competitive programming is and how it is much more about speed than readability/maintainable code. There's some really weird stuff to it, so I guess I'll continue to solve problems just for the practice part really. \$\endgroup\$ – snoopy Nov 1 '19 at 15:22

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.