Conversation
Unsafe practices:
|
Abstraction for testingLines 63 to 80 in 286da95 It is difficult for us to test this game in an automated fashion, because everything that happens in a single cycle of the game loop happens in one method:
To test this in a script, we need a way to pass choices to Game which doesn't involve input(). Let's refactor the code to enable this. |
Abstraction: avoiding redundancyWe see our first chance to eliminate redundancy in Lines 9 to 65 in 286da95 53 lines of very similar code: do we really need all that? Spot the repeating pattern: print(message)
[
print(f"{num+1}: {value}")
for num, value in enumerate(options)
]
choice = int(input(prompt))
return options[choice - 1]Let's make a function for this: [33d83af] Lines 8 to 14 in 33d83af Each method is now reduced to a single function call: Lines 17 to 54 in 33d83af |
|
Now that feels a little silly; can we use abstraction to further simplify this? Sure! First we start by collapsing the disparate pieces of data into a consolidated data structure, a nested dict: [0b941b5] Lines 5 to 13 in 0b941b5 I trust a few lines suffice to demonstrate the idea. Then the *menu methods are reduced to: Lines 22 to 42 in 0b941b5 |
|
A little cleanup before we proceed: [1edb5a6] Input validationWith unused code removed, the Interface class seems a little excessive: it doesn't store any attributes, and essentially bundles methods that don't even use Finally, let's make things a little more robust. We'll validate the player's choice before returning the chosen option: [abeeee9] Lines 8 to 17 in abeeee9 Thanks to the abstraction we did earlier, we only need to write the code once, in one place, and all the menus benefit from it. |
Separating code and dataWe still have some room data in data.py. Let's move that all to gamedata.json: [d8f6a03] Now data.py has only factory functions for creating objects. If we ever need to modify the game data, we know we only need to do so in gamedata.json. |
Separation of concernsA common idea in programming is that of pure functions vs side effects. Take Game.isover(): Lines 22 to 32 in abeeee9 This method returns a When we write functions or methods, we typically want them to do one thing but not the other: return a value, or cause an effect (change a global variable, display a message, write data to a file, ...), but seldom both together. Let's refactor isover() to split up these concerns: [8b1f770] Lines 22 to 38 in 8b1f770 |
Breaking up the game loopFinally, let's break up the game loop to separate concerns and also create smaller methods that make it easier to test our game in automated fashion: [17d0fd5] Lines 66 to 70 in 17d0fd5 Lines 72 to 80 in 17d0fd5 We have a method that displays text, takes player input, validates it, and returns it. And another method that takes a valid choice and carries it out. If we want to test our game in a script, we can call do_choice() with our own script of actions without calling get_choice(): concerns nicely separated! |
Refactoring combatLastly let's see how to simplify combat. Spot any similarities? Lines 26 to 48 in 17d0fd5 Applying encapsulation: [579cfd4] Let's collapse them: [6196649] Lines 32 to 41 in 6196649 This is the heart of the attack() method: an attacker strikes the defender, and a status message is displayed. Lines 12 to 32 in ebf00ae Here we run the battle cycle, using the same method for attacker and defender to trade blows, and swapping their roles each iteration. Does it look messy? Yeah, all those if-elses sure make things hard to read ... this is what polymorphism is for. Let's see how we can simply this first before we apply some polymorphism: [37d000b] |
Separation of concerns, revisitedOne little problem: it's not quite clear how the attacker deals damage to the defender? The logic of this has been hidden in the Character class, when it's actually part of the battle logic. If we add multipliers and other information later, this complexity should be handled by Battle and not Character. Let's move that logic to its rightful place: [b04ebdd] |
the `report` variable in the interface module can be changed to modify how output is handled
|
And again, let's apply this to the battle report: [3dec349] Lines 33 to 41 in 3dec349 It's a minor point, but now we've separated the logic from what happens in an attack from the logic of how to report an attack. It's a small thing, but the small things add up to make it easier to see what's going on quickly. Separation of concerns, yet againOkay, okay, we've already split up the methods, what next? Remember that we want to automate testing, and we don't always want the battle report messages displaying alongside test reports. Is there a way to "disable" printing in the Battle class and just have the battle proceed without the messages? Let's do it: [3b1c2b4] Since display responsibility is handled by the interface module, let's set a variable Now let's update all the places where we used During testing, we don't want game text to be displayed; we want to see test results. So we log all game output to a Now we can write our test script: [f6ad3db] Lines 12 to 17 in f6ad3db Eliminating the last bit of input from Game: [213b000] Because of the way we applied abstraction, we can write our test script as a series of commands and run them in a fixed-iteration loop. |
Wrapping upAnd that wraps up the review for this project. There is of course much more than can be done for abstraction, especially if more features are going to be added for this game; as you notice more repetition and repeated patterns in your code, think about how you can reduce it once it starts to get tedious. |
1 participant
[This is a placeholder pull review: do not approve this PR! It will subsequently be updated with more commits.]
In this review, I'll show you how your project can be further refactored to make it easier for team members and future contributors to understand, and contribute more easily.
We start with some readability fixes, following PEP8 rules for Python development. Replit's editor provides a "Format" button to do this easily, saving you the manual effort.