RPSLS game revisited. Is my design better than previous attempts?

Question

This question is a follow up to these questions:

1. RPSLS Game in C#

2. Ensuring user input is an integer in a range

3. RPSLS is less messy now, but is it clean?

Right now I have a couple of ideas in the think tank about how I want to code what I like to call "killMessage" the whole process seems a little intimidating at the moment, in other words I haven't decided exactly how it is going to work out yet.

There is plenty of code that I borrowed/altered/stole from other answers, but I think we all assumed that was going to happen a little bit, and I think that I understand most of what I included, so I still learned something from it and didn't just copy pasta in the code.

I really like the idea of a Gesture class so that I can actually give the Gesture abilities.

/// <summary>
/// Thanks to 
/// https://codereview.stackexchange.com/users/38054/benvlodgi
/// https://codereview.stackexchange.com/users/4318/eric-lippert
/// https://codereview.stackexchange.com/users/23788/mats-mug
/// https://codereview.stackexchange.com/users/30346/chriswue
/// </summary>
namespace RPSLSGame
{
    class MainClass
    {
        public static void Main (string[] args)
        {
            /* 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 */
            int win = 0;
            int lose = 0;
            int tie = 0;

            IList<Gesture> gameGestures = GetGestures();

            do{
                Console.WriteLine("Please choose your Gesture ");
                PrintMenu(gameGestures);
                var playerGesture = gameGestures[PromptForNumber("Please choose your gesture",1,gameGestures.Count)-1];
                var computerPlay = GetRandomOption(gameGestures);

                Console.WriteLine ("Computer: " + computerPlay);
                Console.WriteLine ("your Gesture: " + playerGesture);

                if (playerGesture.Equals(computerPlay)){
                    tie++;
                }else if(playerGesture.Defeats(computerPlay)){
                    win++;
                }else{
                    lose++;
                }

                Console.WriteLine ("Your Score is (W:L:T:) : {0}:{1}:{2}", win, lose, tie);
                if (Choice("Would you like to reset your score?"))
                {
                    win = 0;
                    lose=0;
                    tie=0;
                }
            }while (Choice("Would you like to play again? Y/N"));
            Console.WriteLine("Goodbye");
            Console.ReadLine ();
        } 

        public static void DecideWinner ()
        {
            //TODO: Create Method for Deciding the Winner.
            //I still need to move the decision logic here. 
            //but it won't be as big as I thought it was going to be
        }

        static int PromptForNumber (string prompt, int lower, int upper)
        {
            int? pick = null;
            while (pick == null) {
                Console.WriteLine(prompt);
                pick = Console.ReadLine().BoundedParse (lower, upper);
            }
            return pick.Value;
        }

        public static void PrintMenu (List<string> List)
        {
            for (int i=0; i<List.Count; i++) {
                Console.WriteLine ((i+1) + ": " + List[i]);
            }       
        }

        public static void PrintMenu (IList<Gesture> GestureList)
        {
            for (int i=0; i < GestureList.Count; i++) {
                Console.WriteLine ((i + 1) + ": " + GestureList [i]);
            }
        }

        public static string GetRandomOption (List<string> options)
        {
            Random rand = new Random();
            return options[rand.Next(0,options.Count)];
        }

        public static Gesture GetRandomOption (IList<Gesture> options)
        {
            Random rand = new Random();
            return options[rand.Next (0,options.Count)];
        }

        public static Boolean Choice (string prompt)
        {
            while(true)
            {
                Console.WriteLine (prompt);
                switch (Console.ReadKey (true).Key)
                {
                    case ConsoleKey.Y:
                    {
                        Console.Write ("Y\n"); 
                        return true;
                    }
                    case ConsoleKey.N:
                    {
                        Console.Write ("N\n");
                        return false;
                    }
                }
            }
        }

/// <summary>
/// This is some code that I borrowed from 
/// https://codereview.stackexchange.com/users/14749/peter-kiss
/// from an answer that he gave to one of my Questions, the answer has since been deleted.
/// </summary>

        static IList<Gesture> GetGestures()
        {
            var spock = new Gesture("Spock");
            var lizard = new Gesture("Lizard");
            var paper = new Gesture("Paper");
            var rock = new Gesture("Rock");
            var scissors = new Gesture("Scissors");

            spock.AddDefeats(new[] { scissors, rock });
            lizard.AddDefeats(new[] { paper, spock });
            paper.AddDefeats(new[] { rock, spock });
            rock.AddDefeats(new[] { lizard, scissors });
            scissors.AddDefeats(new[] { paper, lizard });

            var gestures = new List<Gesture> {rock, paper, scissors, lizard, spock};
            gestures.ForEach(x => x.Seal());
            return gestures;
        }
    }

    public class Gesture
    {
        public override string ToString()
        {
            return Name;
        }

        private HashSet<Gesture> _defeats = new HashSet<Gesture>();
        private bool _isSealed;
        public string Name { get; private set; }

        public Gesture(string name)
        {
            if (name == null) throw new ArgumentNullException("name");
            Name = name;
        }

        public void Seal()
        {
            _isSealed = true;
        }

        public void AddDefeats(IEnumerable<Gesture> defeats)
        {
            if (_isSealed)
            {
                throw new Exception("Gesture is sealed");
            }

            foreach (var gesture in defeats)
            {
                _defeats.Add(gesture);
            }
        }

        //public void AddKillMessage(Tuple<Gesture,Gesture,string>  
        public void AddKillMessage (Dictionary<Dictionary<Gesture,Gesture>,string> killMessage)
        {
            if (_isSealed) {
                throw new Exception ("Gesture is Sealed");
            }
        }

        protected bool Equals(Gesture other)
        {
            return string.Equals(Name, other.Name);
        }

        public override bool Equals(object obj)
        {
            if (ReferenceEquals(null, obj)) return false;
            if (ReferenceEquals(this, obj)) return true;
            if (obj.GetType() != this.GetType()) return false;
            return Equals((Gesture) obj);
        }

        public override int GetHashCode()
        {
            return Name.GetHashCode();
        }

        public bool Defeats(Gesture gesture)
        {
            return _defeats.Any(x => x.Equals(gesture));
        }
    }
}

static class Extensions
{
    public static int? BoundedParse (this string str, int lower, int upper)
    {
        int result;
        bool success;
        success = int.TryParse (str, out result);
        if (!(lower <= result && result <= upper) || (!success) || (str == null)){
            return null;
        }
        return result;
    }
}

I hope that repeat reviewers find that I have taken their advice to heart and hopefully I haven't slaughtered their code while making it my own.

There are several overloaded methods in the code that I left there from previous iterations.

Answer 1

I'm going to leave a broader review to other answers, and only focus on a single line of code:

throw new Exception("Gesture is sealed");

Don't do that.

There are really two alternatives to throwing exceptions.

One is to throw an existing exception, derived from System.Exception. This is trickier than it sounds, because you need to pick your exception carefully - you want to throw a meaningful exception.

In this case an InvalidOperationException seems in line with the Principle of Least Astonishment:

InvalidOperationException Class

The exception that is thrown when a method call is invalid for the object's current state. _{http://msdn.microsoft.com/en-us/library/system.invalidoperationexception(v=vs.110).aspx}

There are a few exception subclasses like this one (and ArgumentException and its derivatives), that are very often the most meaningful and written-for-that-purpose exception to use.

---

Another way would be to define your own. That's easier that it sounds, you just derive a new class from System.Exception:

public class GestureSealedException : Exception
{
    public GestureSealedException()
        : base("Gesture is sealed.") { }
}

And now you can throw and catch a GestureSealedException.

---

That said...

Don't throw System.Exception.

In my quick research for why that class wasn't abstract in the first place if it wasn't intended to be instanciated and thrown, I found an interesting discussion on Programmers.SE. Emphasis mine:

when coding small demo apps or proof-of-concepts, I don't want to start designing 10 different exception sub-classes, or spending time trying to decide which is the "best" exception class for the situation. I'd rather just throw Exception and pass a string that explains the details. When it's throw-away code, I don't care about these things [...]

_{https://softwareengineering.stackexchange.com/a/119683/68834}

The flipside of this out-of-context quote, is that when it is quality code, you do care about these things.

Answer 2

1. In the scope of this program, there is no benefit to using a Gesture class vs using an enum with some extension methods. I will draw upon my latest version of an OOP RPSLS for some examples.

   Using a Gesture enum

   enum Gesture
   {
       Rock,
       Paper,
       Scissors,
       Lizard,
       Spock
   }
   

   You can easily add an extension method to give you the Defeats method you implemented in your Gesture class. Below I assume you have a Dictionary of Rules like in my version.

   public static class Extensions
   {
       public static bool Defeates(this Gesture me, Gesture gesture)
       {
           return Game.Rules[me].Item1.ContainsKey(gesture);
       }
   }
   

2. I see that you have a few duplicated functions which do the exact same thing, only.. with a different data type. This is a bit of a red flag. Instead in this case you should just make one method. The one that takes a list of strings. Then make a function to convert an IList to a List ... this is super simple

    myGestureList.select(g=>g.ToString()).ToList();
   

   Or, better yet... make a method that uses Generic T, then you can use PrintMenu GetRandomOption with any data type.

   public static T GetRandomOption<T>(List<T> options)
   {
       return options[new Random().Next(0, options.Count)];
   }
   

   Now whenever you call this method the type T will be inferred from the type of List you pass in and will result with that type of object being returned. I also in-lined the declaration of your Random object with the call of its Next method.

   Using this same technique, your PrintMenu Function will look like this (I think you should call this PrintOptions).

   public static void PrintMenu<T>(List<T> List)
   {
       for (int i = 0; i < List.Count; i++)
       {
           Console.WriteLine((i + 1) + ": " + List[i]);
       }
   }
   

   Here is re-worked version of this code, which makes it easy to offset the base number if desired.

   public static void PrintMenu<T>(List<T> values, int baseIndex = 0)
   {
       values.ForEach(value => Console.WriteLine("{0}: {1}", baseIndex++, value));
   }
   

3. Your win/lose/tie variables should be plural.

   int wins = 0;
   int loses = 0;
   int ties = 0;
   

4. In your Choice Function you should take off the brackets from your case statements.

   switch (Console.ReadKey(true).Key)
   {
       case ConsoleKey.Y:
           Console.Write("Y\n");
           return true;
       case ConsoleKey.N:
           Console.Write("N\n");
           return false;
   }
   

5. In C# the convention is different than Java where you are encouraged to add your brackets at the end of a statement.. ie.

   if(Boolean value){
       statement;
   }
   

   Your if-else chain should not use that convention. And really you could just not have the brackets at all considering they are 1 liners. I wouldn't even be opposed to 1-lining each of these statements. This point is a little more opinion based, but that is the C# convention. If you just press CtrlED in Visual Studio, you will find that your code will be reformatted to expand those brackets (only works if your code has no errors).

   if (playerGesture.Equals(computerPlay))
       tie++;
   else if(playerGesture.Defeats(computerPlay))
       win++;
   else
       lose++;
   

6. You should use OO for players instead of for Gestures, you can see my old review for more info on that. Or look at my more recent CR post.

7. Anything else I would have pointed out, has already been said by @Mat's Mug

Answer 3

I find it a nuisance to define so many methods merely in order to override Equals.

Therefore I sometimes define and invoke a method named Matches instead.

Given that you only have one instance of each gesture, do you even need to define it? The default implementation of object.Equals is to call object.ReferenceEquals.

---

Your Seal functionality is ugly, for several reasons. Two alternatives:

• Define AddDefeats as private; create a static constructor of the Gesture class, and construct the list of Gesture instance in the static constructor (which can invoke the private method)

  class Gesture
  {
      internal static IList<Gesture> AllGestures;
  
      static Gesture
      {
          var spock = new Gesture("Spock");
          var lizard = new Gesture("Lizard");
          var paper = new Gesture("Paper");
          var rock = new Gesture("Rock");
          var scissors = new Gesture("Scissors");
  
          spock.AddDefeats(new[] { scissors, rock });
          lizard.AddDefeats(new[] { paper, spock });
          paper.AddDefeats(new[] { rock, spock });
          rock.AddDefeats(new[] { lizard, scissors });
          scissors.AddDefeats(new[] { paper, lizard });
  
          AllGestures = new List<Gesture> {rock, paper, scissors, lizard, spock};
      }
  

• Remove the AddDefeats method, and pass the array of defeats as a parameter to the Gesture constructor (so that a Gesture instance is fully-constructed immediately)

  // defeats expressed using strings (gesture names) instead of Gesture instances
  // because we need to refer to gestures which haven't been constructed yet
  public Gesture(string name, IEnumerable<string> defeats)
  {
      if (name == null) throw new ArgumentNullException("name");
      Name = name;
  
      foreach (var gesture in defeats)
      {
          _defeats.Add(gesture);
      }
  }