1
\$\begingroup\$

I had a coding assignment to write a Rock Paper Scissors game. The main task is not to show a solution but rather to test the coding style. The rules are such:

Problem:

rock is "O", paper is "[]", scissors is "8<". George and John play RPC(rock, paper and scissors) a couple of times. I receive 2 input strings containing their actions. it is guaranteed that both strings contain the same number of actions and the actions will be in {'O', '[]', '8<'}. Each game gets scored. If a player wins, he gets 2 points, if the game is draw, both get 1 point. the program has to calculate which player has how many points and what is the most common match up and how many times it occured.

Example:

input:

OOOOOO[][]8<

[][]8<8<8<O[]O[]

output:

George 12 John 6 O8< 3

explanation:

George plays O John plays [] the game outcome is O[] George points: 0 John points: 2


George plays O John plays [] the game outcome is O[] George points: 0 John points: 4


George plays O John plays 8< the game outcome is O8< George points: 2 John points: 4


George plays O John plays 8< the game outcome is O8< George points: 4 John points: 4


George plays O John plays 8< the game outcome is O8< George points: 6 John points: 4


George plays O John plays O the game outcome is OO George points: 7 John points: 5


George plays [] John plays [] the game outcome is [][] George points: 8 John points: 6


George plays [] John plays O the game outcome is []O George points: 10 John points: 6


George plays 8< John plays [] the game outcome is 8<[] George points: 12 John points: 6


most common match up is rock vs scissors and it happened 3 times. Final output:

George 12 John 6 O8< 3

My thoughts and code:

Thoughts:

Initially I made the solution in plain functions. After that I thought that I should wrap the code into classes, because it would be good to have variables like george's points available between different functions. I did not add any validators for the input data. I am not sure if that was correct or not. In the statement it was clearly explained that the input data will be passed correctly. I did not thought on edge cases like having empty inputs. The code that calculates who won the game maybe looks hardcoded as it again relies on correct input.

Code:

SCISSORS = "8<"
PAPER = "[]"
ROCK = "O"
GEORGE_LOSES = ["8<0", "O[]", "[]8<"]

class Player:
    def __init__(self, hands="") -> None:
        self.hands = hands
        self.index = 0
        self.hand_len = len(self.hands)
        self.points = 0
        self.current_hand = ""

    def has_hands(self):
        return self.index < self.hand_len

class RPS_solver:
    def __init__(self, george=Player(), john=Player()) -> None:
        self.george = george
        self.john = john
        self.george_result = ""
        self.john_result = ""
        self.matchup_outcome = ""
        self.matchup_dict = {"default": 0}
        self.most_common_matchup = "default"

    def next_item(self, player):
        if(player.hands[player.index] == "O"):
            player.index += 1
            return ROCK 
        if(player.hands[player.index] == "8"):
            player.index += 2
            return SCISSORS 
        else:
            player.index += 2
            return PAPER

    def count_match_points(self):
        if self.george.current_hand == self.john.current_hand:
            self.george.points += 1
            self.john.points += 1

        elif self.matchup_outcome in GEORGE_LOSES:
             self.john.points += 2

        # assuming all input data is correct
        else:
             self.george.points += 2

    def get_most_common_matchup(self):
        if self.matchup_outcome in self.matchup_dict:
            self.matchup_dict[self.matchup_outcome] += 1
        else:
            self.matchup_dict[self.matchup_outcome] = 1
        if(self.matchup_dict[self.matchup_outcome] > self.matchup_dict[self.most_common_matchup]):
            self.most_common_matchup = self.matchup_outcome


def solution(george: str, john: str)  -> str:
    # refactoring ca be added to assert correction of data
    # 
    #The input of your function consists of two strings - 
    # the symbols that George showed in the order he showed
    #  them and a second string with the
    #  set of symbols that John showed in the order he showed them.

    george_player = Player(george)
    john_player = Player(john)

    solver = RPS_solver(george_player, john_player)
    while(solver.george.has_hands()  and solver.john.has_hands()):
        solver.george.current_hand = solver.next_item(solver.george)
        solver.john.current_hand = solver.next_item(solver.john)

        #match outcome
        solver.matchup_outcome = solver.george.current_hand + solver.john.current_hand

        # count match points:
        solver.count_match_points()

        # determine current most common matchup:
        solver.get_most_common_matchup()

    result_str = f"George {solver.george.points} John {solver.john.points} {solver.most_common_matchup} {solver.matchup_dict[solver.most_common_matchup]}"
    return result_str

print(solution("OOOOOO[][]8<", "[][]8<8<8<O[]O[]"))

Question:

How can I improve this code for style (maybe even performance). Should I add validators if input is being guaranteed? Should I add comments ?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Where is the code for the output? As there seems to be a bug in it. George plays O John plays O the game outcome is O[] George points: 0 Should be George plays O John plays [] the game outcome is O[] George points: 0 \$\endgroup\$
    – George R
    Sep 17, 2021 at 12:16
  • \$\begingroup\$ @LuciferUchiha my mistake, I have fixed it \$\endgroup\$
    – Harton
    Sep 17, 2021 at 16:18
  • \$\begingroup\$ If you have a class assignment where the goal (of the class, not yours) is to have good coding style, you should disclose that you got a coding review online and used that advice when turning it in. This may (correctly) be considered cheating otherwise. \$\endgroup\$ Sep 17, 2021 at 21:43

1 Answer 1

4
\$\begingroup\$
  • You should not bake hand literal strings in your logic; instead refer to your constants or better yet an Enum
  • You should try to reduce the amount of class state floating around. index and current_hand for instance are not good class members; instead they should just be local variables.
  • Consider refactoring your Player class to be an iterator over its hands, and taking responsibility for parsing its hand string.
  • Rather than matchup_dict, use a Counter. This will greatly simplify get_most_common_matchup.
  • solution should be a method on RPS_solver.
  • Rework this to be a unit test that asserts expected output.
  • Do not hard-code the player names; pass them into members on the player objects.

Suggested

from collections import Counter
from enum import Enum
from typing import Iterable


class Hand(Enum):
    SCISSORS = "8<"
    PAPER = "[]"
    ROCK = "O"

    def loses_to(self, other: "Hand") -> bool:
        return LOSES_TO[self] == other


LOSES_TO = {
    Hand.SCISSORS: Hand.ROCK,
    Hand.ROCK: Hand.PAPER,
    Hand.PAPER: Hand.SCISSORS,
}


class Player:
    def __init__(self, name: str, hands: str) -> None:
        self.name, self.hands = name, hands
        self.points = 0

    def __iter__(self) -> Iterable[Hand]:
        index = 0
        while index < len(self.hands):
            for hand in Hand:
                if self.hands[index:].startswith(hand.value):
                    yield hand
                    index += len(hand.value)
                    break
            else:
                raise ValueError("Invalid hand string")

    def __str__(self) -> str:
        return f"{self.name} {self.points}"


class RPS_solver:
    def __init__(self, george: Player, john: Player) -> None:
        self.george = george
        self.john = john
        self.matchup_counts = Counter()

    def count_match_points(self, george_hand: Hand, john_hand: Hand) -> None:
        if george_hand == john_hand:
            self.george.points += 1
            self.john.points += 1
        elif george_hand.loses_to(john_hand):
            self.john.points += 2
        else:
            # assuming all input data is correct
            self.george.points += 2

    def update_counts(self, george_hand: Hand, john_hand: Hand) -> None:
        self.matchup_counts[george_hand, john_hand] += 1

    def solve(self) -> str:
        # refactoring can be added to assert correction of data
        #
        # The input of your function consists of two strings -
        # the symbols that George showed in the order he showed
        #  them and a second string with the
        #  set of symbols that John showed in the order he showed them.

        for george_hand, john_hand in zip(self.george, self.john):
            self.count_match_points(george_hand, john_hand)
            self.update_counts(george_hand, john_hand)

        ((george_common, john_common), count), = self.matchup_counts.most_common(1)

        return (
            f"{self.george} {self.john} "
            f"{george_common.value}{john_common.value} {count}"
        )


def test() -> None:
    solver = RPS_solver(
        Player("George", "OOOOOO[][]8<"),
        Player("John", "[][]8<8<8<O[]O[]"),
    )
    assert solver.solve() == "George 12 John 6 O8< 3"


if __name__ == "__main__":
    test()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.