Refactoring a Rock Paper Scissors Lizard Spock Game // Code Roast Part 1

[@ArjanCodes]

💡 Get my FREE 7-step guide to help you consistently design great software: arjancodes.com/designguide.

[@circularanemone]

Thanks for helping me get better at python by looking at the aspects that come after introductory courses. Examples are clear and your explanations add a lot to the content. Thank you!

[@ArjanCodes]

You’re welcome - I’m happy to hear the videos are helpful!

[@shaddwatson1833]

This is just amazing! Especially since I don't have anyone to critique my code. Thanks for sharing!!

[@johnhutchinson9445]

This is a great series, I’ve never seen someone do an in-depth refactor like this. Looking forward to checking out the rest of the videos!

[@ArjanCodes]

Thanks John, happy you’re enjoying the content!

[@julianvega6405]

Nice stuff!! I'm currently learning python from a udemy course, but I think the teacher is not really following best practices. Good to have this channel as a reference for that. Thank you!

[@lbb2rfarangkiinok]

I have yet to take a walkthrough tutorial that uses best practices. On the other hand, doing so would often include too much abstraction for new learners.

[@joshuawalsh6968]

This is an fantastic channel, you have really helped my coding. Who else does code roasts Ive never seen it so in depth, what a great idea! So awesome 😎😎

[@ArjanCodes]

You're very welcome!

[@fastrockstar1705]

Your channel is pure Gold, Arjan!

[@ArjanCodes]

Thank you so much, glad you like the content!

[@Thayme]

Great video as always! My nitpick would be that there is still duplication between the player and the CPU, which are both "players" and only differ in entity selection method (one has an AI, one takes user input). I would introduce a player protocol, then implement HumanPlayer and CPUPlayer classes. This would also move the AI logic inside the "pick_cpu_entity" method out of the UI, where it doesn't really fit (this logic would be the same regardless of the UI). Finally, it would also let you extend the code to allow 2-player hot-seat, where the 2nd player has a very slight advantage! Party like it's 1999!

[@nateholloway]

A Player class would also allow the player to have a score and displaying the scoreboard could take a list of Players

[@JakeDiToro]

Ya, moving the "AI" logic inside the UI bugged me as well. A seperation of player from the Game class would solve some of those issues.

[@henrikholland1730]

Thanks Arjan, best code roast video’s out there.

[@ArjanCodes]

Thanks Henrik!

[@cj5925]

Nicely orchestrated!

[@ArjanCodes]

Thanks!

[@asifiqbal21]

Thanks Arjan for the video.

[@appuser]

Would love to see a video from you on how to write a good main() function, once you're done refactoring to a family of classes, since you still need to initialize and run everything. E.g. do you create a few classes/functions to "compress" to a few lines, or is it okay to have a long declarative section?

[@whkoh7619]

learning so much from your videos. thanks!

[@ArjanCodes]

Glad to hear they’re helpful!

[@danielw8620]

@@ArjanCodes can i get access to the source code for this project so i can follow along easier? do you have this posted on github or anything, thanks for your work!

[@ArjanCodes]

I've added a link to the Git repository just now, see the description.

[@danielw8620]

@@ArjanCodes thanks your the best

[@LxAU]

Hi Arjan. I notice that when you implement a Protocol, you don’t extend from that Protocol (e.g. “class CLI” instead of “class CLI(UI)”. I find I get much better IDE functionality if I do the latter. What are your thoughts on this?

[@mukeshsinhrajput2205]

kzbin.info/www/bejne/rqfFZpt9gdR-ZqM

[@thibautdecombe1975]

Protocol is supposed to be more of a typing thing. In this video, adding `ui: UI` to the init method is enough to say `hey, we need any ui object to implement the UI Protocol ie having all these custom methods available`. Because CLI implement these methods, the type checking is passing. However, you can explicitly declare that CLI is implementing the UI Protocol (using `CLI(UI)`) as specified here: peps.python.org/pep-0544/#explicitly-declaring-implementation You get a few little differences (quoted from the PEP): - You get some protocol methods “for free”. (For example if the UI class was implementing a few methods not raising notImplementedError) - Type checkers can statically verify that the class actually implements the protocol correctly. So like you say, You probably get more IDE functionality by explicitly declaring it. But I also feel it's pretty much and ABC now so idk, I'm probably missing something. Would love to have Arjan thoughts on that too.

[@quasistellar_]

Pretty sure he just forgot, and since python doesn't have the override keyword or annotation, it was left unnoticed.

[@pacersgo]

Protocol class should not be inherited by the concrete classes. The concrete class implements the protocol, but it is not the subclass of the protocol class.

[@LxAU]

@@quasistellar_ Possibly, although I noticed that this wasn’t done in a couple other Protocol-related videos either-so it could be a conscious decision, but I’m hoping it’s not, because it sure does make things easier after modifying the protocol if everything extends from it.

[@rolandgarceau865]

I have. not been on a code roast for many many years. For me refactoring is good to start on a whiteboard and tackle one section at a time. There is a lot going on here, and all the other videos I have seen you do thus far have been absolutely amazing at taking the time to vocalize exactly what is happening. I've purposefully waited to watch these code review videos as I am just getting back up to speed with a few year hiatus from coding while trying to learn other aspects of technology. If I had the money I would pay for your services. I need to get a couple jobs under my belt. I will not forget how effective your tutorials are. Thank you very much for your time. I think I would be willing to watch a many part roast with your before and after code samples. I have been experimenting with using .ipynb to linearly dissect some of the coding refreshers I have been going through. Maybe that would help in situations like these. I'd love to talk more with you about teaching if you would like to have a conversation about that. [e-mail]. Thanks again for your highly effective videos.

[@tbpotn]

Why does Pick_Cpu_Entity go to CLI/UI? It doesn't have anything to do with commandline?

[@Jason-jb1tf]

It seems you've moved game state logic into the ui at 22:08 when you moved the cpu entity selection into cli.py instead of game.py, which then would require duplication for the cpu entity selection in a gui.py. Having pick_player_entity in cli.py / gui.py makes sense because you're getting user input to then feed back into the game, but having a function in your interface that isn't getting data from the user and isn't showing data to the user seems like a strange choice. If you were to roast your own refactor, would you leave the pick_cpu_entity() function in the game class? Or maybe move it out into an ai.py class that allows you to implement different ai players you can play against? (In this type of game all I can think of is differently weighted entity preferences for certain players, to mimic human behavior)

[@aflous]

Solid point here, I totally agree

[@sagiziv927]

Great video as always, really enjoyed it. I have a (bit unrelated) question about the UI class. If in the future we do choose to use GUI as the UI, how would the functions be implemented¿ Because in GUI, I assume, you don't read the input from the user when we call one of the functions in the Protocol, you wait for a button click (or any event that indicates the user entered input). This is something I have always struggled with, because the input mechanisms are so different between these 2 UIs. For CLI you just call ``input``, but for GUI you need to wait for some kind of an event to be triggered. Anyway, thanks again for the great video 😃

[@LxAU]

I thought about the same thing. I suppose in this instance, Arjan was picturing using a “modal” input box-one that interrupts the existing GUI flow until dismissed. Or perhaps the other parts of the GUI would be greyed out when it is called. Maybe Arjan would consider a Tk (built-in GUI toolkit) video sometime-although it would probably be quite a long one even for something simple.

[@cryptosimon9529]

When does Part 2 come out?!

[@ArjanCodes]

Next week on Friday!

[@tymoteuszsikora8812]

Hey, what color theme do u use?

[@maleldil1]

It's weird that you're using type hints for everything but don't use a type checker in the editor or mypy in the CLI. All the runtime errors you ran into would've been detected by mypy if you were using it.

[@cadoozles2]

Hi Arjan, question from a newer developer: I don't understand why you created the ui.py Protocol class. Should it be inherited by CLI so CLI becomes a subclass? Or is that ui.py just there as a reference for developers? Edit: Looks like I need to do some reading on 'Protocol' in python, there is some implicit subtyping with it's use? Your channel always introduces me to more concepts that I enjoy reading further on Love your channel!

[@aflous]

Hey, it not subtyping, but duck typing. In short, protocols are a kind of interfaces that allow you to define a contract for how objects should interact between each other. The dynamic nature of python plays well with this and you also get some benefits when using a static type checker as mypy for example.

[@FernandoCordeiroDr]

I think that's the first time I've seen Arjan raise a NotImplementedError for a Protocol. He used to do this only for ABCs... I wonder what changed.

[@ArjanCodes]

I’m struggling with whether to use ‘…’, ‘pass’ or raise a not implemented error like I did here. I believe the recommended way is to use ‘…’, but it leads to type errors. I’m investigating it.

[@FernandoCordeiroDr]

@@ArjanCodes Always found "..." weird. It's not very common in Python and my coworkers have to look them up whenever I use them, so I gave up on them. Raising an exception for a protocol, which is essentially a type hint, also doesn't feel right. I mean, no other user-defined type would raise an exception. I like `pass`, but what I like even more is just leaving a docstring. It looks good plus it allows me to remove the `pass` command. At the same time, it provides more context to whoever uses this Protocol after I move on. :)

[@quasistellar_]

@@FernandoCordeiroDr raising an error notifies you when you accidentally try to use methods of the abstract class (why abstract, static, etc. are not keywords in python is a mystery for me) instead of its actual implementations. Just ignoring this with a *pass* will leave the mistake unnoticed. P.S. I just googled 'abstract keyword in python' and turns out that some people actually use it in this exact context. Just writing *abstract* instead of *raise NotImplementedError* will hang the program with the NameError since python has no clue what this word means. Lol.

[@cychoboy]

pick_cpu_entity has nothing to do with the UI.

[@william10121972]

Hi Arjan great analysis. But just one question you don't like the _name in functions or classes but the first thing you do is create a UI (user interface).

[@ArjanCodes]

Haha, that’s actually a good point. I guess I should call it a ‘player interface’ to be consistent, but I think it would be really confusing.

[@FlorianHolzner]

Hi Arjan, every time you do those methods, I wonder if you've heard of micro committing and the mikado method for refactoring legacy code. Have you? Those two saved my sanity more than once this year and micro committing has changed how I do commits and MRs radically … :D

[@ArjanCodes]

Thanks for mentioning this. I haven’t heard of the mikado method, I’ll check it out. It looks like a nice topic for a video!

[@FlorianHolzner]

@@ArjanCodes look at The Legacy Code First Aid Kit. It's got a great intro to those two techniques and it's been a sanity saver for me. Enjoy, and keep up the good work! :)

[@soylentpink7845]

I often have the case that multiple things have to be done in sequence like a pipeline or just things that have to be executed in order. I implement it using methods that change the object state, but i find that to be very dirty because you never know what the status is explicitly. I see this problem very often also in other context. It would be very welcome if you could present an example on how to solve such a problem.

[@TheWorpler]

Let's go!!

[@xnick_uy]

I was thinking that the method to display the rules should accept a Rules object (either class instance or dictionary) as an argument, so it can generate the appropriate message on runtime -- or assign the responsibility of displaying the rules to the Rules object. Another thing I was wondering if the Entity class could already have the rules embedded on itself, perhaps tinkering with the Enums. But I'm not sure if that's a good idea. I guess I would have tried to arrange the rules as some form of 2D data such as rules = { Entity.PAPER: { Entity.ROCK: {'wins':true, 'message': 'Paper covers Rock'}, Entity.SPOCK: {'wins':true, 'message':'Paper disapproves Spok'}, .... }, Entity.ROCK: {Entity.LIZARD:{...},...}, .......} or even tried to get fancier with one 2D boolean array to determine the winner and another one for the messages. But one of the downsides of this approach is that any change made on the Entity class leads to having to manually update the Rules accordingly. Would it make more sense to have the rules as part of each Entity item? That way, each time an Entity item is added to the class, we only have to "teach" the rules to that specific item (first item has empty rules, second item knows the rules against the first, third item knows the rules against the previous two, and so on).

[@natehermawan9176]

Great vid! I learn a lot about class programming etiquette watching these decomposition videos. I’m curious though, is there anything wrong with introducing some game logic directly into the entity classes? For instance, Rock has a loses_to attribute listing Spock and Paper, and a wins_against attribute listing Lizard and Scissors.

[@samarbid13]

Thank you for the refactoring contants, Can you do more refactores for MVP frameworks like django?

[@ImOnUToob]

I’m a newbie making a CLI game as a first project. Question: For each of the methods in the UI class that don’t reference “self”, should these be static methods or does it really just not matter? I know it works either way, but I’m trying to learn best practices in these situations. What if the class ends up being 100% static methods…. Should it even be a class at that point? I suppose the class/object created is still required for the dependency injection… right?

[@СергейПищулов-ы4ч]

The decision about storing UI instance in field of Game class is debatable. If you want to test game class you will need to create a UI instance and pass it to Game constructor. Thus every time you will run some tests for Game class, there will be some strings printed by this UI instance

[@ArjanCodes]

That’s easy to solve by creating a UI subclass that doesn’t print/do anything, and then pass that to Game in your testing code. If I didn’t separate the UI from Game, this wouldn’t have been possible and testing Game would have been a lot harder.

[@СергейПищулов-ы4ч]

@@ArjanCodescreating an instance of class that does nothing is a meaningless infrastructure code, using only to correspond the choosen architecture. Wouldn't MVC pattern be the better decision? Thank you for answering my question.

[@philippwestphal5178]

Hi Arjan, thanks for the video. Assuming you always play against the cpu, would it not make more sense to keep the "cpu" player in the game engine and not put it in the UI?

[@Cookie-mv2hg]

So...Do senior developers change like a HELL LOF OF code until they try to run if it breaks? holy...

[@ArjanCodes]

Nope. I do this to keep the length of the video under control.

[@skyeadamson8257]

First time I've come across protocols - is there a difference in when they'd be used compared to abstract classes?

[@VulturARG]

There is a video on this channel about that.

[@Gummibandet1]

Sorry if this is dumb, but isnt a file where every function is "NotImplemented" confusing for someone reading the code. As in, it is interpreted as actually not being implemented. Isn't there a better way to leave empty functions for subclasses to use?

[@agentdarkboote]

This is a pattern I've seen a lot, although a lot of the time there will also be an error message saying something like "subclasses must override this method", which is a bit more user friendly I suppose. Python doesn't have the same way to make interfaces that other languages do.

[@susmitvengurlekar]

Suggestion: I really feel that you should use ABC instead of Protocol as well as make sure that implementations inherit from the abstract classes. All current codes are unlinked to the interface class. Others do you agree ?

[@pacersgo]

There are pros and cons for using abc or protocols. In this case I don’t see any reason why protocol would not work better than abc.

[@susmitvengurlekar]

@@pacersgo I need to study indepth about there differences. But if the classes are linked via inheritance even if when using protocol (if that can be done), then that would surely be helpful during development

[@aflous]

​@@susmitvengurlekarthis is relying on duck typing. In a way, Protocol classes can be seen as a way to formalize duck typing, as they allow you to explicitly describe the expected behavior of an object while still being flexible in terms of the actual class or interface used Note that Protocol classes don't enforce the implementation of the methods at runtime like ABCs do. However, if you use a type checker like mypy, it will verify that the classes implementing the Protocol have the correct method signatures.

[@pacersgo]

I think you can also try to apply mvc architecture for this application.

[@jpconstantineau]

New Rules? Trying to get an advantage over the CPU winning all the time?

[@ArjanCodes]

Haha, exactly!

[@akenvito]

Okay, cpu won, cpu always wins for some reason, let's nerf it!

[@제인-y9u]

First

[@jombonidracuz6176]

I am here to pronounce you are first.