Rock, Paper, Scissors, Lizard, Spock (Without regex) by andresmoschini · Pull Request #5 · MakingSense/coding-dojo

andresmoschini · 2017-12-27T16:09:09Z

Well, because of a confusion, I have worked in a different kata than specified in the repo.

Anyway, I think that it could be useful, to enrich this repository. It is pending to add the specification, but in general, it is based on https://gist.github.com/trikitrok/63bfb5226f8cc2e8f6c3 and https://ajlopez.wordpress.com/2013/04/26/tdd-kata-1-piedra-papel-tijera-lagarto-spock/

I would like to some advice about how to include it in the repo, maybe it is enough adding a new readme.md, but I would like to hear Sensei's (@DanielAltamirano) opinion, or if he is not available, @AlphaGit 's one. I also would like to see your reviews.

Also, I still have to recover information about the time that I have spent on this.

I am not following the fine granular TDD approach, and now that I am doing this commit I am thinking that it would be equally easy to apply a granular approach as I did with Draw. Well, it is already done so it tests a lot of scenarios together.

Since all test have the same structure, I think that it could be a good idea to merge them. Thoughts?

…ting multiples implementations I am stopping working for the moment, the following steps could be: * Add new referee implementation following [nickmcdowall's Stage 2](https://github.com/nickmcdowall/Rock-Paper-Scissors-Kata/blob/master/README.md#stage-2) * Add support for Spock and Lizard as it is described in [nickmcdowall's Stage 3](https://github.com/nickmcdowall/Rock-Paper-Scissors-Kata/blob/master/README.md#stage-3) * Add richer result object with the action that winner choice does on the looser, for example _rocks **crushes** scissors_

…Referee **Attention!** something is wrong: All test passed when they should not. See the next commit for more information.

Since in commit 9975412 I made together the changes in the tests and the new implementation, I did not verify that test failed. **Lesson learned** Always verify that tests fails before fix them.

I have removed redundant rules, but I am still not happy with OOP approach, I still prefer rule based one.

…ions

andresmoschini · 2017-12-27T20:06:45Z

Added a new "explanation" feature. I wanted to check how refactoring painless is each approach.

AlphaGit

I like the solution! It took me a bit to figure out why the testing solution required so many classes, but when I understood it, it was clearly the best approach.

Regarding the game logic, I left some questions about naming and responsibility delegation. I think that the game itself is fairly simple but trying to abstract it may have made the game a little bit more difficult to follow.

However, this is an open-ended discussion. Let me know what you think!

AlphaGit · 2018-01-07T19:30:35Z

...aneous/rock-paper-scissors-no-regex/Solutions/amoschini/RpsKata/ExtensionRulesGameReferee.cs

+{
+    public class ExtensionRulesGameReferee : IGameReferee
+    {
+        private (GameChoice player1Choice, GameChoice player2Choice, GameResult result)[] _rules = new[]


Is this an array of tuples? It does look like them but I didn't know each tuple could have named values.

Or are these anonymous objects? In that case, I did not know they could be assigned to a class-level variable.

Please, enlight me!

Regardless, do you think that the complexity of the structure would have required a data structure? Something like a GamePrediction class?

Is this an array of tuples? It does look like them but I didn't know each tuple could have named values.

Or are these anonymous objects? In that case, I did not know they could be assigned to a class-level variable.

It is an array of ValueTuple, you can see a detailed explanation in Mark Zhou's Tech Blog, for example:

You can even give a name to every element in the value tuple, like this:

public (string name, int age) GetStudentInfo(string id) { // Search by ID and find the student. return (name: "Annie", age: 25); } public void Test() { (string name, int age) info = GetStudentInfo("100-000-1000"); Console.WriteLine($"Name: {info.name}, Age: {info.age}"); }

Regardless, do you think that the complexity of the structure would have required a data structure? Something like a GamePrediction class?

You agree with you, a class would be better, but since I was using it internally I did not pay attention to that detail. Maybe the name should be related to rule in place of prediction.

Oh, I knew about ValueTypes, I did not know you could declare an array of value tuples with names in them. That blew my mind.

I probably (wrongly) assumed that ValueTuples where just associated objects without structure (like pretty much other language treat tuples) but it seems not to be the case.

Thank you!

AlphaGit · 2018-01-07T19:35:40Z

Miscellaneous/rock-paper-scissors-no-regex/Solutions/amoschini/RpsKata/GameRefereeTest.cs

+    public abstract class GameRefereeTest<TSut> : GameRefereeTest
+        where TSut : IGameReferee, new()
+    {
+        protected override IGameReferee GetSut() => new TSut();


What does Sut stand for? "System under test"?

Yes, it is System Under Test, It is common to see this name in Mark Seemann's posts

AlphaGit · 2018-01-07T19:42:13Z

Miscellaneous/rock-paper-scissors-no-regex/Solutions/amoschini/RpsKata/GameResult.cs

+        {
+            TheWinnerIsPlayer1 = player1Won;
+            TheWinnerIsPlayer2 = player2Won;
+            ItIsADraw = player1Won == player2Won;


The game, conceptually, has 3 possible result states: Player1 wins, Player2 wins, the game is a Draw. Each of these 3 possible states is mutually exclusive from all the others.

The class definition has independent Player1Won (boolean: 2 states), Player2Won (boolean: 2 states), ItIsADraw (boolean: 2 states). As such, the class allows for 8 distinct states.

I'm thinking maybe the GameResult class was not a good idea as a class with state, but rather it should have been into a wrapper of an enum (GameResult) and an explanation string.

I understand your distrust about boolean properties, but in my opinion, they represent the result in a simpler way. To avoid illegal states and inconsistencies, I make these properties read-only and used named constructors (Player1Won(), Player2Won() and Draw()) to generate the result, then the class does not allow for 8 distinct states.

Also, GameResult does not have state because all properties are read-only.

AlphaGit · 2018-01-07T19:43:02Z

...neous/rock-paper-scissors-no-regex/Solutions/amoschini/RpsKata/OopAdapterRulesGameReferee.cs

+        }
+
+        // This method is only necessary to adapt this new implementation to previous 
+        // interface. If we decided to use this implementation definitivaly, it should


Minor typo: definitely.

AlphaGit · 2018-01-07T19:46:12Z

...neous/rock-paper-scissors-no-regex/Solutions/amoschini/RpsKata/OopAdapterRulesGameReferee.cs

+            }
+
+            protected GameResult Draw() => GameResult.Draw(ToGameChoice());
+            public GameResult IWin(string verb, Choice looser) => _resultFactoryIfItWins(ToGameChoice(), verb, looser.ToGameChoice());


Minor typo: loser

AlphaGit · 2018-01-07T19:48:18Z

Miscellaneous/rock-paper-scissors-no-regex/Solutions/amoschini/RpsKata/SimpleGameReferee.cs

+                ?? GameResult.Draw(gameSetup.Player1Choice);
+        }
+
+        private GameResult Evaluate(Func<GameChoice, string, GameChoice, GameResult> playerWon, GameChoice playerAChoice, GameChoice playerBChoice)


Minor: throughout most of the classes, players had been named player and player2, but in here, they're named playerA and playerB. It is generally better to keep a same denomination across the code for consistency.

AlphaGit · 2018-01-07T19:51:54Z

Miscellaneous/rock-paper-scissors-no-regex/Solutions/amoschini/RpsKata/SimpleGameReferee.cs

+        public GameResult Evaluate(GameSetup gameSetup)
+        {
+            return Evaluate(GameResult.Player1Won, gameSetup.Player1Choice, gameSetup.Player2Choice)
+                ?? Evaluate(GameResult.Player2Won, gameSetup.Player2Choice, gameSetup.Player1Choice)


Why does Evaluate need to be called twice, otherwise, the result is a Draw?

My first instinct tells me (because of the inverted parameters) that each evaluates if player1 won or player2 won, so I'd imagine that a better signature could have been PlayerWins(Player.Player1, gameSetup).

But then, the signature of the private Evaluate confuses me again: why does it receive a Func as the first parameter? Since this class is the referee -- what behavior is it delegating? Maybe renaming the arguments could help clear it up -- or maybe even a named interface would clear it up.

You are right. Since this Evaluate() overload is private, I did not pay enough attention to the name and semantics, but I will review it. Thanks!

AlphaGit · 2018-01-07T19:55:09Z

I would like to some advice about how to include it in the repo, maybe it is enough adding a new readme.md

I'd say that's absolutely right. As long as you credit the source and there are no legal issues with it, feel free to do so!

andresmoschini · 2018-01-22T17:37:53Z

Thanks for the review @AlphaGit ! I really appreciate it.

I just come back from vacations so I will be checking your comments during some time, maybe a comment per day.

andresmoschini added 30 commits December 23, 2017 11:30

Add .gitignore and .gitattributes.

63113a0

Add project files.

d2cecd2

Add system structure

0487f1f

Support Draw (RED)

26f2405

Support Draw (GREEN)

57a4a8b

Avoid wrong Draw results (RED)

6c088d6

Avoid wrong Draw results (GREEN)

3a0b451

Support right result for when not drawing (GREEN)

12ecfac

Refactor tests

12de524

Since all test have the same structure, I think that it could be a good idea to merge them. Thoughts?

Add new GameReferee with simple rules definition

9975412

Take advantage of HashSet O(1) search time complexity

4025be9

Add support for Spock and Lizard (RED)

6380c00

Add support for Spock and Lizard (RED/GREEN) - Fix SimpleGameReferee

c4034e8

Add support for Spock and Lizard (RED/GREEN) - Fix ExtensionRulesGame…

579c565

…Referee **Attention!** something is wrong: All test passed when they should not. See the next commit for more information.

fixup! Add new GameReferee with simple rules definition

de7a91f

Since in commit 9975412 I made together the changes in the tests and the new implementation, I did not verify that test failed. **Lesson learned** Always verify that tests fails before fix them.

Add support for Spock and Lizard (GREEN) - Fix WinnerRulesGameReferee

836a82d

Move to solution folder based on repo standard

a6106c3

Add new OOP based solution (RED)

405afbd

Add new OOP based solution (GREEN / UGLY)

fd71e87

Add new OOP based solution (GREEN / REFACTOR / STILL UGLY)

d32f487

Add new OOP based solution (GREEN)

e8c6dd6

I have removed redundant rules, but I am still not happy with OOP approach, I still prefer rule based one.

Merge remote-tracking branch 'upstream/master' into rpsls

0e593e1

Refactor GameResult as a class in place of an enum

bd41891

Add explanation to result (RED)

81a4424

Add explanation to result (RED) - Fix draw results in all implementat…

75249f8

…ions

Add explanation to result (RED) - Fix ExtensionRulesGameReferee results

a292bd7

Add explanation to result (RED) - Fix SimpleGameReferee

b187a74

Add explanation to result (RED) - Fix WinnerRulesGameReferee

6aceebb

Add explanation to result (GREEN) - Fix OopAdapterRulesGameReferee

da2ba9f

AlphaGit approved these changes Jan 7, 2018

View reviewed changes

andresmoschini mentioned this pull request Feb 23, 2018

Morse decoder with Elixir #3

Merged

Conversation

andresmoschini commented Dec 27, 2017

Uh oh!

andresmoschini commented Dec 27, 2017

Uh oh!

AlphaGit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlphaGit Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlphaGit commented Jan 7, 2018

Uh oh!

andresmoschini commented Jan 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlphaGit Jan 22, 2018 •

edited

Loading