Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This question is a follow up to these questions:

  1. RPSLS Game in C#

  2. Is my coding technique progressing in terms of C# loops?

  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 
/// http://codereview.stackexchange.com/users/38054/benvlodgi
/// http://codereview.stackexchange.com/users/4318/eric-lippert
/// http://codereview.stackexchange.com/users/23788/mats-mug
/// http://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 
/// http://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.

share|improve this question
    
I did fail to mention that I gave in to peer pressure and used a do while loop for my main loop. Any feed back on that is also welcome, please also let me know what you think I am doing right as well. –  Malachi Mar 16 at 21:17

3 Answers 3

up vote 11 down vote accepted

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 [...]

http://programmers.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.

share|improve this answer
    
I like this answer, 1. I was using someone else's code there and should have changed that exception. 2. I have created exceptions before, but I need the practice. 3. not sure why I would use an exception there. I need to look into it more this coming week probably before(or while) I work on the KillMessages –  Malachi Mar 16 at 4:58
1  
@Malachi FYI, I've moved this answer here as a canonical answer to "Throwing System.Exception". –  Mat's Mug Mar 20 at 18:26
    
sounds good to me. –  Malachi Mar 20 at 18:44
  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

share|improve this answer
1  
@Malachi #4 is correct, you should use the curly braces to define a scope - the case blocks of a switch statement do not define a scope, by wrapping case blocks in curly braces you are introducing a scope there, which means each case can then define its own local variables, which smells. As for #5, it's also correct, you should spare "egyptian braces" for Java... but using them won't cause any harm (other than a future maintainer would probably CSharpify them - I know I would!) –  Mat's Mug Mar 17 at 17:37
1  
@Malachi indeed, if (true) DoSomething(); is legal (same for a 1-liner loop), but for maintainability and readability purposes, it's recommended to always correctly scope these code blocks, i.e. use them curly braces and go if (true) { DoSomething(); }. –  Mat's Mug Mar 17 at 17:44
1  
@Malachi in a case statement you should use use the break keyword to end your code for that specific case. –  BenVlodgi Mar 17 at 18:14
1  
@Malachi maintain a list of rules, it is sooo much simpler than making a gigantic clunky Gesture class. Then just use an extension method as I did above. –  BenVlodgi Mar 17 at 18:31
1  
FWIW an enum is also an object :) –  Mat's Mug Mar 17 at 19:02

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);
        }
    }
    
share|improve this answer
    
would you still feel this way if I made it possible for an admin user to create new Gestures from the console interface of the application? I mean for the Static Gesture Constructor? little confused about the static constructor part as well. –  Malachi Mar 17 at 17:34
    
I edited the answer to show an example of a static constructor. –  ChrisW Mar 17 at 17:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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