7
\$\begingroup\$

I did a job application to a Unity developer position and i was given a coding challenge which was implementing a Rock Paper Scissors game or a variation of it. I have implemented the game using Unity and C# and the feed back I got was my implementation being "too mathematical" and "lacking object oriented principles". So i am a bit confused so i thought i should ask four your feedback.

I have actually implemented Rock Paper Scissors Lizard Spock from The Big Bang Theory show and had a simple logic having elements in a list and calculating who wins based on these indexes in the list. So it works like this:

If the absolute difference between Player and Opponent element index is even, then element with smaller index wins and if it is odd, the element with bigger index wins.

I have two scripts for achieving this, one is attached to all elements(Rock,Paper etc.) in the game which just handle IO operations and send the selected input to GameController like this:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class ElementController : MonoBehaviour
{

   public GameObject Controller;

   private Renderer rend;
   private GameController GameController;
   private Color previousColor;
   private List<string> Elements = new List<string>() { "Spock", "Lizard", "Rock", "Paper", "Scissor" };

   void Start()
   {
       rend = gameObject.GetComponent<Renderer>();
       previousColor = rend.material.color;
       GameController = Controller.GetComponent<GameController>();
   }

   void OnMouseEnter()
   {
       rend.material.color = Color.yellow;
       previousColor = Color.yellow;
   }
   void OnMouseExit()
   {
       rend.material.color = Color.white;
       previousColor = Color.white;
   }
   void OnMouseDown()
   {
       //Mouse can be clicked only if Coroutine is not running
       if (!GameController.isRunning)
       {
           // Passing the selected element by user to GameController
           GameController.Game(Elements.IndexOf(gameObject.name));
           rend.material.color = Color.red;
       }
   }
}

And then the GameController runs a coroutine and gives the result of each round like this:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.UI;

public class GameController : MonoBehaviour
{
    #region UIElements
    public Button Restart;
    public Text RoundResult;
    public Text PlayerScoreText;
    private int PlayerScore = 0;
    public Text OpponentScoreText;
    private int OpponentScore = 0;
    #endregion UIElements

    [HideInInspector]
    public bool isRunning = false;
    public GameObject Opponent;
    public float flipDuration;

    //Indexes are required to calculate winner of each round 
    private readonly List<string> Elements = new List<string>() { "Spock", "Lizard", "Rock", "Paper", "Scissor" };
    private int OppMove;


    void Start()
    {
        Restart.onClick.AddListener(RestartGame);
    }

    public void Game(int PlayerElement)
    {
        StartCoroutine(RoundResultCoroutine(PlayerElement));
    }

    //Coroutine running every round
    IEnumerator RoundResultCoroutine(int PlayerElement)
    {
        //Since Range with integers are not maximally inclusive
        OppMove = Random.Range(0, 5);

        //Activate child Opponent played
        Opponent.transform.GetChild(OppMove).transform.gameObject.SetActive(true);

        float t = 0;
        //isRunning is used in order to prevent user inteference during each round is held.

        isRunning = true;

        while (t < flipDuration)
        {
            t += Time.deltaTime;
            Opponent.transform.eulerAngles = Vector3.Lerp(new Vector3(0, 0, 0), new Vector3(0, 180, 0), t / flipDuration);
            yield return null;
        }

        /*
         * Player has equal probability of winning or losing since there are odd number of elements.
         * From the rules we can infer with the indexing of Spock = 0, Lizard = 1, Rock = 2, Paper = 3, Scissor = 4
         * if the absolute difference between Player and opponent element is even, then element with smaller index wins
         * if it is odd, the element with bigger index wins. Because as we can see from the graph and imagine it as a directed graph
         * an element always wins against the next element and loses against the previous element. Also it wins against next element of its directed 
         * neighbor. This mathematical solution is implemented in order to eliminate keeping track of all possible Rules between two chosen elements.
         * Also this aproach would work with any N = #elements given that N is odd. N should be odd to have same probability for each element in order to
         * have a fair game.
         */
        if (PlayerElement == OppMove)
        {
            RoundResult.text = "It is a tie";
            RoundResult.GetComponent<Text>().color = Color.white;
        }
        //Having even absolute difference
        else if (Mathf.Abs(OppMove - PlayerElement) % 2 == 0)
        {
            if (PlayerElement > OppMove)
            {
                UpdateScore("Opponent", PlayerElement, OppMove);
            }
            else
            {
                UpdateScore("Player", PlayerElement, OppMove);
            }
        }
        //Having odd absolute difference
        else
        {
            if (PlayerElement > OppMove)
            {
                UpdateScore("Player", PlayerElement, OppMove);
            }
            else
            {
                UpdateScore("Opponent", PlayerElement, OppMove);
            }
        }

        yield return new WaitForSeconds(flipDuration * 2);
        RoundResult.text = " ";
        t = 0;

        while (t < flipDuration)
        {
            t += Time.deltaTime;
            Opponent.transform.eulerAngles = Vector3.Lerp(new Vector3(0, 180, 0), new Vector3(0, 0, 0), t / flipDuration);
            yield return null;
        }

        Opponent.transform.GetChild(OppMove).transform.gameObject.SetActive(false);
        isRunning = false;
    }

    // Method used for updating score and modifying round result text
    private void UpdateScore(string winner, int PlayerMove, int OpponentMove)
    {
        if (winner == "Player")
        {
            PlayerScore++;
            PlayerScoreText.text = PlayerScore.ToString();
            RoundResult.text = Elements[PlayerMove] + " wins against " + Elements[OpponentMove];
            RoundResult.GetComponent<Text>().color = Color.green;
        }
        else if (winner == "Opponent")
        {
            OpponentScore++;
            OpponentScoreText.text = OpponentScore.ToString();
            RoundResult.text = Elements[PlayerMove] + " loses against " + Elements[OpponentMove];
            RoundResult.GetComponent<Text>().color = Color.red;
        }
    }

    private void RestartGame()
    {
        if (!isRunning)
        {
            PlayerScore = 0;
            PlayerScoreText.text = PlayerScore.ToString();
            OpponentScore = 0;
            OpponentScoreText.text = OpponentScore.ToString();
        }
    }
}

In general, i wanted to ask you how would i complete this task with better oo handling. Should i have used Singleton for game controller or instead of this mathematical solution have a static list of rules and check from that list.

\$\endgroup\$
4
  • 3
    \$\begingroup\$ I don't understand, what do they mean by being Too mathematical ? \$\endgroup\$ Mar 26, 2019 at 14:02
  • \$\begingroup\$ I am not sure. I got the feed back from the recruiter but i am guessing they wanted me to have a static list of rules instead of that mathematical approach for deciding on winner of the round. \$\endgroup\$
    – Ali Kanat
    Mar 26, 2019 at 14:04
  • \$\begingroup\$ Could you post complete classes instead of just their content please? \$\endgroup\$
    – t3chb0t
    Mar 26, 2019 at 16:45
  • 1
    \$\begingroup\$ @t3chb0t I updated the scripts. \$\endgroup\$
    – Ali Kanat
    Mar 26, 2019 at 18:11

3 Answers 3

6
\$\begingroup\$

... "too mathematical" and "lacking object oriented principles"

These go hand in hand. An algorithm is what it is. Not that it is or is not too mathematical but that necessarily busy, complex, or obscure code is not wrapped in code that behaves (exposed) in subject/business terms.

For example I think method and variable naming has a distinct algorithmic feel. Also the good start with {"Spock, "Lizard" ...} is not exploited, it's all index referencing.

--------------

Object Oriented DNA

The general goal of writing in subject matter terms starts with data structures that can be (indeed must be) exploited in an OO way. Subsequent code layers naturally tend to exhibit OO tendencies when we start with OO. Should technical limitations require "less robust" structures like a list of strings (is there anything worse!?), wrapping that in its own class will pay dividends.


class Element { 
    protected List<string> elements = new List<string>() {"Spock"...};

    public string Spock { get { return elements[0]; } }

    // exposing the low-level, abstract foundation
    // is definitely a code smell
    int indexOf (string thisGuy) { ... }

If we really must have a list of strings. Benefits for readability, error avoidance, and ease of use are easy to imagine.


enum Element = {Spock, Lizard, ...}

has better coding expressive power, avoids all string pitfalls (typos, CasInG), is type safe, and we can work with underlying int values. Also, enum values can be XOR'ed to group them; way cool.


Dictionary<Element, Element> Rules = new Dictionary<Element, Element>()
    { Element.Rock, Element.Scissors},
    { Element.Scissors, Element.Paper}, ...

This defines "Rock beats Scissors", "Scissors beats Paper", etc. Since some Elements defeat 2 or more things the dictionary value maybe should be a collection of some kind or even an enum grouping (see XOR comment above).

If the Rules value must be a List<Element>, let's say, then a class is probably in order. Client code should not be allowed or required(!) to rifle through Rules objects.


\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your feedback. Enum is something i definitely should have considered. I just have one thing to correct. Each element wins against exactly 2 elements and loses against 2 as well to have a fair game. Otherwise, you would go for elements with higher probability of winning. \$\endgroup\$
    – Ali Kanat
    Mar 27, 2019 at 7:32
4
\$\begingroup\$

I am neither a C# nor Unity coder. With that in mind, here goes:

You don't seem to be following Microsoft's C# coding conventions, and it's not obvious to me what you are following. In the absence of some documented reason, I'd suggest going with the industry standard.

I don't understand the logic of keeping rend as a "global" variable.

private Renderer rend;

// ...

void Start()
{
    rend = gameObject.GetComponent<Renderer>();
    previousColor = rend.material.color;
    GameController = Controller.GetComponent<GameController>();
}

Why not just make rend a local var:

void Start()
{
    var rend = gameObject...
}

Since the events being handled are "human scale" events, you can afford to repeat the call to GetComponent at the top.

I would suggest that you break your RoundResultCoroutine into smaller pieces: the rule of thumb about subroutines fitting on a single 25-line screen still applies. Certainly it seems like there is a part that makes the opponent's move, which could be a separate function, and a part that determines the outcome of play, which could be a separate function. So that area seems ripe for improvement.

In my opinion (which is worthless, since I'm not involved in your hiring decision) the "mathematical" approach to determining the result of play is quite nice. I would definitely suggest that you not change that part! You might want to include a better explanation of how/why it works, and maybe some notes about its efficiency...

There seems to be some other stuff missing. I don't see the declarations of the top-level objects, for instance. That makes it hard to suggest how you could be more OO. But you don't define any classes yourself, so that may be why you got dinged for not OO enough. Perhaps creating some kind of GameResult class would help? I really don't feel like it's needed. Try just cleaning up your non-OO code first.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the recommendations. I used rend as a global variable thinking not to use GetComponentrepeatedly but you are actually right. I also modified the scripts with full classes. I will definitely dig into C# conventions it was a nice suggestion! \$\endgroup\$
    – Ali Kanat
    Mar 26, 2019 at 18:20
  • \$\begingroup\$ In Unity, GetComponent is an expensive call, and while it being called here only in OnMouseXYZ methods probably won't have as much gameplay impact as if it were called on every frame, it is conventional to cache the output of GetComponent calls whenever the result is expected to be used multiple times. \$\endgroup\$
    – Ruzihm
    Jul 26, 2019 at 19:52
3
\$\begingroup\$

Other reviews already cover a lot of ground so this will be a bit wordy.

Software developers create abstractions - at some point, the code will be complicated but it's our job to encapsulate that and make it easier to understand. Remember: you will spend as much time reading code as you will writing it.

Not everyone will be aware of the rules of Rock, Paper, Scissors, Lizard, Spock. How would you go about displaying some help text? Will you type it out by hand or resort to convoluted logic over the array values? Consider how you might fill in this code:

string gameExplanation = "The rules are: " + ...

With radarbob's suggestion of a dictionary, it's easy:

string gameExplanation = "The rules are: " + string.Join(Environment.NewLine, Game.Rules.Select(kvp => $"{kvp.Key} beats {kvp.Value");

You can see that even this isn't a perfect abstraction because the verb is usually different. E.g. "Rock crushes Scissors", "Paper disproves Spock". Your list cannot encode this difference. It is already at the limit of what it can describe. Your comment mentions that the rules are a graph. It's probably overkill to model it as a graph but your rules should be first class citizens - not buried in some logic to decide who wins.

I appreciate that this was an example bit of code for an interview so you wouldn't want to spend too much time on it but you also need to show what you can do. If I see a candidate with code that is extensible, even if that extension hasn't been added, I'm going to think that they've thought it through before starting.

\$\endgroup\$
3
  • \$\begingroup\$ I actually thought of the abstraction you mentioned but my attitude was towards having a more robust solution which will work even with 7, 9 or more odd number of elements. In those cases abstraction would be almost impossible because for example in case of 9, you need 36 rules. I thought of solving this problem by having the rules as graph and as a UI element in the game. \$\endgroup\$
    – Ali Kanat
    Mar 28, 2019 at 11:01
  • 1
    \$\begingroup\$ What I like about your comment @AliKanat is that you clearly had thought about it (and well) but it's not clear from the code that you had. I realise on second reading that it is a note in your comment but it's buried. Make your thinking and choices obvious to the interviewer :) \$\endgroup\$
    – RobH
    Mar 28, 2019 at 11:07
  • \$\begingroup\$ :) Thanks i will keep that in mind for next interview. \$\endgroup\$
    – Ali Kanat
    Mar 28, 2019 at 11:14

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.