For a start, I would recommend swapping out any if/elif/else block with more than 3 clauses to a switch block, but that's more of a personal preference thing than an actual necessity. I find that maintaining large switch blocks tends to create fewer errors than maintaining a huge if/elif/else block. Ideally, if you're comfortable with it, I would recommend a more functionally oriented solution by creating a dict of lambda expressions to navigate, and reduce your giant if/elif/else blocks to something like this (example taken from attributes.py, Stats.mod_stat [line 25]):
def mod_stat(self, stat, value, mode):
stats = {
'max_hp': lambda value: self.max_hp += value,
'hp': lambda value: self.hp += value,
'atk': lambda value: self.atk += value,
'acc': lambda value: self.acc += value,
'arm': lambda value: self.arm += value
}
if mode == 0:
value = -value
if stat in stats:
stats[stat](value)
else:
print(stat, "is not a stat")
Forgive me if my python syntax is a bit off, it's been a while since I was last able to use it
EDIT: Gaaah, I accidentally typed stat[stats] and didn't notice it. That most definitely would not work.
EDIT2: While typing up my reply to this, I noticed that I missed an if statement in this function
Just thought of this - If you don't mind a more serious refactoring towards a more object-oriented solution, here's an example of how I would rewrite attributes.py (you are free to either follow this example or ignore it as you wish):
class Stat:
def __init__(self, initValue=0, increaseAmount=0, statName="", max=initValue, min=0): # Yay default values
self.value = initValue
self.maxValue = max
self.minValue = min
self.increaseAmount = increaseAmount
self.statName = statName
def increase(self):
self.maxValue += self.increaseAmount
self.value += self.increaseAmount # Not sure if this should increase with the max or not, I'm not the one writing the game
print("Your " + self.statName + " is now", self.value
def mod(self,value=0):
self.value += value
if self.value > self.maxValue:
self.value = self.maxValue
elif self.value < self.minValue:
self.value = self.minValue
class Stats:
def __init__(self, max_hp, atk, acc, arm, moveset):
self.stats = {
'hp': new Stat(max_hp),
'atk': new Stat(atk),
'acc': new Stat(acc),
'arm': new Stat(arm)
}
self.moveset = moveset
def mod_stat(self, stat, value, mode):
if mode == 0:
value = -value
if stat in self.stats:
self.stats[stat].mod(value)
else:
print(stat, "is not a stat")
class PlayerStats(Stats): # mod_stat inherited from Stats, so we don't need to define it here
def __init__(self):
self.stats = {
'hp': new Stat(100, 10, "HP"),
'atk': new Stat(10, 3, "attack"),
'acc': new Stat(85, 5, "accuracy"),
'arm': new Stat(0, 5, "armor"),
'mp': new Stat(100, 10, "mp"),
'nut': new Stat(3, 0, "nut")
}
self.moveset = None
def increase(self, stat):
if stat in self.stats:
self.stats[stat].increase()
else:
print("WHAT ARE YOU DOING")
I'm guessing the lambdas somehow make it so I'm not just increasing the value in the dict? I tried it with a dict first, it only increased the dict value.
Yeah, the lambda makes the entry a function that you can invoke at any time. Sorry, I really should have included a link to some documentation in my original comment.
In short, lambdas are functions that are bound to variables, and can be called anywhere within the function that they're defined in, or they can be a return value from a function.
Self-explanatory, really. In other words, it would be easier to understand if you stored this logic in a data structure instead of using "print" statements. But's that's really a very minor point: it's easy to understand as it is.
With global variables like these, one thing you can do to make the code easier to understand is to comment extensively about (a) where the variable is set, and (b) where the variable is read. This way, when you see a function accessing it, you will have a better idea what code set the value of the variable.
Seems a bit strange to use exception handling to know whether the command is recognized or not.
One question is how to read the code. So I see there is an exception here called KeyError. What throws this exception? Is it caught elsewhere? I thought I could look through this file for this KeyError but it only appears twice in the file and neither place gives me a clue. This shows a basic principle of what makes code easy to understand: put stuff near each other in the source code so you don't have to look across many locations in the source code to understand the logic, and when this isn't the case, comments help.
So you have the all_spells variable which you access in a few places in this file. I looked at the spells.py file where the all_spells variable is initialized. But I wasn't sure if spells were added elsewhere as well. I think it would be hard to tell without looking through the rest of the source code.
If you can't create a scene that isn't in the global list, then one wonders what the point of the parameter is.
The point of the parameter is that pretty much everyone has told me "global vars are bad, mmkay?"
Seems a bit strange to use exception handling to know whether the command is recognized or not.
Thanks for finding that, I've been trying to get rid of them, I was mostly using them when I didn't really know any other way to fix the problem I was having at the time.
But I wasn't sure if spells were added elsewhere as well.
Unless I've misunderstood you, every single spell in the game (currently one) should be in spells.py
16 comments
2 u/RevanProdigalKnight 25 Apr 2016 13:51
For a start, I would recommend swapping out any
if/elif/elseblock with more than 3 clauses to aswitchblock, but that's more of a personal preference thing than an actual necessity. I find that maintaining largeswitchblocks tends to create fewer errors than maintaining a hugeif/elif/elseblock. Ideally, if you're comfortable with it, I would recommend a more functionally oriented solution by creating adictoflambdaexpressions to navigate, and reduce your giantif/elif/elseblocks to something like this (example taken fromattributes.py,Stats.mod_stat[line 25]):Forgive me if my python syntax is a bit off, it's been a while since I was last able to use it
EDIT: Gaaah, I accidentally typed
stat[stats]and didn't notice it. That most definitely would not work.EDIT2: While typing up my reply to this, I noticed that I missed an if statement in this function
1 u/RevanProdigalKnight 25 Apr 2016 14:46
Just thought of this - If you don't mind a more serious refactoring towards a more object-oriented solution, here's an example of how I would rewrite
attributes.py(you are free to either follow this example or ignore it as you wish):0 u/draco_nite [OP] 26 Apr 2016 05:26
Just a small nitpick:
newdoesn't work in Python 3.1 u/draco_nite [OP] 25 Apr 2016 21:16
I'm guessing the lambdas somehow make it so I'm not just increasing the value in the dict? I tried it with a dict first, it only increased the dict value.
0 u/RevanProdigalKnight 25 Apr 2016 22:36
Yeah, the lambda makes the entry a function that you can invoke at any time. Sorry, I really should have included a link to some documentation in my original comment.
In short, lambdas are functions that are bound to variables, and can be called anywhere within the function that they're defined in, or they can be a return value from a function.
1 u/svipbo 25 Apr 2016 20:33
Here's one thing:
https://bitbucket.org/weanoob/whorehouse/src/8a9fba4083af2e1397563db509cd7077889b6902/scenes.py?at=master#scenes.py-123
Self-explanatory, really. In other words, it would be easier to understand if you stored this logic in a data structure instead of using "print" statements. But's that's really a very minor point: it's easy to understand as it is.
https://bitbucket.org/weanoob/whorehouse/src/8a9fba4083af2e1397563db509cd7077889b6902/scenes.py?at=master#scenes.py-39
A bit confusing here. You use a global variable to store a list of the scenes, but then you also have a parameter to say which list to use:
https://bitbucket.org/weanoob/whorehouse/src/8a9fba4083af2e1397563db509cd7077889b6902/scenes.py?at=master#scenes.py-205
If you can't create a scene that isn't in the global list, then one wonders what the point of the parameter is.
Generally I don't see global variables as that bad. You'll know that they are bad when you start to get confused by the code.
https://bitbucket.org/weanoob/whorehouse/src/8a9fba4083af2e1397563db509cd7077889b6902/combat.py?at=master#combat.py-13
With global variables like these, one thing you can do to make the code easier to understand is to comment extensively about (a) where the variable is set, and (b) where the variable is read. This way, when you see a function accessing it, you will have a better idea what code set the value of the variable.
https://bitbucket.org/weanoob/whorehouse/src/8a9fba4083af2e1397563db509cd7077889b6902/combat.py?at=master#combat.py-73
Seems a bit strange to use exception handling to know whether the command is recognized or not.
One question is how to read the code. So I see there is an exception here called
KeyError. What throws this exception? Is it caught elsewhere? I thought I could look through this file for thisKeyErrorbut it only appears twice in the file and neither place gives me a clue. This shows a basic principle of what makes code easy to understand: put stuff near each other in the source code so you don't have to look across many locations in the source code to understand the logic, and when this isn't the case, comments help.https://bitbucket.org/weanoob/whorehouse/src/8a9fba4083af2e1397563db509cd7077889b6902/combat.py?at=master#combat.py-70
stuffandstuff_splitare bad variable names. Maybeinput_lineandinput_argumentsor something?https://bitbucket.org/weanoob/whorehouse/src/8a9fba4083af2e1397563db509cd7077889b6902/combat.py?at=master#combat.py-8
So you have the
all_spellsvariable which you access in a few places in this file. I looked at thespells.pyfile where theall_spellsvariable is initialized. But I wasn't sure if spells were added elsewhere as well. I think it would be hard to tell without looking through the rest of the source code.0 u/draco_nite [OP] 26 Apr 2016 05:54
The point of the parameter is that pretty much everyone has told me "global vars are bad, mmkay?"
Thanks for finding that, I've been trying to get rid of them, I was mostly using them when I didn't really know any other way to fix the problem I was having at the time.
Unless I've misunderstood you, every single spell in the game (currently one) should be in spells.py
0 u/Tommstein 26 Apr 2016 04:33
I like the theme of this game.
-1 u/SketchyFishSandwich 25 Apr 2016 13:22
Will there be a console port?
0 u/draco_nite [OP] 25 Apr 2016 21:15
If someone buys me a devkit after I finish this, sure.
-2 u/J_Darnley 25 Apr 2016 20:04
It's python; need I say more?
/me runs
0 u/Tommstein 26 Apr 2016 04:39
lolwut