I'm new to Python, and my first serious project was a FreeCell game. The game works, with lots of options to combine moves, save games, load them, etc. But I skimped on some specifics of Python. In particular, reading in options.
import re
vertical = False
doubles = False
autoReshuf = False
savePosition = False
saveOnWin = False
annoyingNudge = False
def readOpts():
global vertical
global autoReshuf
global doubles
global saveOnWin
global savePosition
global annoyingNudge
infile = "fcopt.txt"
with open(infile) as f:
for line in f:
gotOne = 1
if line[0] == '#': #ignore comments
continue
q=re.sub(r'.*=', '', line.rstrip())
if "autoReshuf".lower() in line.lower():
autoReshuf = TOrF(q)
if "savePosition".lower() in line.lower():
savePosition = TOrF(q)
if "saveOnWin".lower() in line.lower():
saveOnWin = TOrF(q)
if "vertical".lower() in line.lower():
vertical = TOrF(q)
if "doubles".lower() in line.lower():
doubles = TOrF(q)
if "annoyingNudge".lower() in line.lower():
annoyingNudge = TOrF(q)
if gotOne:
print "Options file read."
f.close()
else:
print "Failed to read options file."
return
def TOrF(x):
if x == "False" or x == "0":
return False
return True
readOpts();
The options file I have features lines such as this, with #
s to comment lines out:
vertical=1
doubles=0
autoReshuf=1
savePosition=0
saveOnWin=0
annoyingNudge=True
I have a feeling I could simplify this code greatly with ConfigParser, but I'm totally busted at the moment, even with examples. And I don't like throwing all those globals around. How can I write this brute-force code more succinctly and efficiently?
1 Answer 1
You have a good idea on what to do. But rather than using global instead use local objects. But even better use a dictionary.
Currently you have something like:
my_global_variable = 'default'
def read_settings():
global my_global_variable
my_global_variable = 'something else'
def foo():
if my_global_variable == 'something else':
# Do something
However this means that you have to keep everything in your global scope. Rather than this, I'd change to use a dictionary. To do this you can set all the defaults in a single variable, and then over-write them in your read_settings
file. Take:
settings = {
'key': 'default'
}
def read_settings():
settings['key'] = 'something else'
def foo(SETTINGS):
if SETTINGS['key'] == 'something else':
# Do something
This allows you to later move these functions into their own modules, and they will still be usable. And so I'd go on to change the original code to use a dictionary to hold the settings.
I would also change:
Remove
re
and instead usestr.split
, it makes the code easier to understand.
If XKDC is your think they even suggest not using them:Check if the left hand side is actually correct,
autoReshufAn'Ting
shouldn't pass, and be allocated toautoReshuf
.- Change your settings to have better keywords, like
auto_reshuffle
. The use of_
allows you to use only lower-case letters and for your keys to still be readable. - Remove the
if
/else
after you read the file. Theelse
is wrong as you were able to read the options file, otherwise there would be an error. - Don't use semicolons, there's no need in Python.
- Follow PEP8.
Without using dictionarys, you can get something like:
vertical = False
doubles = False
auto_reshuffle = False
save_position = False
save_on_win = False
annoying_nudge = False
def read_settings(infile="fcopt.txt"):
global vertical
global doubles
global auto_reshuffle
global save_position
global save_on_win
global annoying_nudge
with open(infile) as f:
for line in f:
# ignore comments
if line[0] == '#':
continue
key, value = line.split("=", 1)
if key.lower() == "vertical":
vertical = true_or_false(value)
if key.lower() == "doubles":
doubles = true_or_false(value)
if key.lower() == "auto_reshuffle":
auto_reshuffle = true_or_false(value)
if key.lower() == "save_position":
save_position = true_or_false(value)
if key.lower() == "save_on_win":
save_on_win = true_or_false(value)
if key.lower() == "annoying_nudge":
annoying_nudge = true_or_false(value)
def true_or_false(x):
if x == "False" or x == "0":
return False
return True
read_settings()
After this you want to use the dictionary as I suggested. With this you can use in
to check if key.lower()
is in it, and allows you to remove all the duplicate code. After checking if the key is in the dictionary, you want to overwrite the default value in the dictionary, so you can just assign the true_or_false
index the dictionary, by indexing with the lowercase key.
This can get something like:
SETTINGS = {
'vertical': False,
'doubles': False,
'auto_reshuffle': False,
'save_position': False,
'save_on_win': False,
'annoying_nudge': False,
}
def read_settings(infile="fcopt.txt"):
with open(infile) as f:
for line in f:
# ignore comments
if line[0] == '#':
continue
key, value = line.split("=", 1)
key = key.lower()
if key in SETTINGS:
SETTINGS[key] = true_or_false(value)
def true_or_false(x):
if x == "False" or x == "0":
return False
return True
read_settings()
And so if you want to add a new value to your settings you can! Just by adding it to the dictionary.
You can also try using json
to make a config for you. I personally use it as my default data storage. And with by using sort_keys=True
and indent=4
, you can make a pretty configuration. Which doesn't support comments. Or alternately you can import a Python file with a dictionary, shown below, or use configparser
.
setting.py
:
settings = {
'VerTicaL': True,
}
main.py
:
import settings
settings = {k.lower(): v for k, v in settings.settings.iteritems()}
-
\$\begingroup\$ A very belated thanks for this. I didn't have comment privileges before due to low rep, so I couldn't post, but...I've been able to do pretty much all you've suggested, and I've come back to your response several times. \$\endgroup\$aschultz– aschultz2017年04月27日 17:11:45 +00:00Commented Apr 27, 2017 at 17:11
-
1\$\begingroup\$ @aschultz Late is better than never, :) Thank you for your comment, I'm happy I could help! \$\endgroup\$2017年04月27日 17:41:10 +00:00Commented Apr 27, 2017 at 17:41
Explore related questions
See similar questions with these tags.
savePosition=false
would actually set it toTrue
is unintuitive. \$\endgroup\$