10
\$\begingroup\$

I would like to use this in a program to save users' names even after they relaunch the script. I was just wondering if there were some ways I could clean up this code.

Note: this currently was made to not support name changing, but could possibly be implemented in the future.

if os.path.isfile('names.json'): # if there is a file with names...
    text_file = open("names.json") # only open it in normal mode
else: # if not...
    text_file = open("names.json", "w+") # make one

if os.stat("names.json").st_size > 0:
    for line in text_file:
        if line.istitle():
            player_name = line
        else:
            player_name = console.input_alert('What is your name? ').title()
            text_file = open("names.json", "w")
            text_file.write(player_name)
            text_file.close()
else:
    player_name = console.input_alert('What is your name? ').title()
    text_file = open("names.json", "w")
    text_file.write(player_name)
    text_file.close()
\$\endgroup\$
7
+50
\$\begingroup\$

I'm not sure why you are reading all of the file and checking for istitle(). Here I make the assumption that if the file exists, it will always include the player name on the first line.

if os.path.isfile('names.json'):
  with open('names.json') as f:
    player_name = f.readline()
else:
  player_name = console.input_alert('What is your name? ').title()

with open('names.json', 'w') as f:      
    f.write(player_name)

If you want to read the entire file and check for istitle():

player_name = None
if os.path.isfile('names.json'):
  with open('names.json') as f:
      for line in f.readlines():
          if line.istitle():
              player_name = line

with open('names.json', 'w') as f:  
    if player_name is None:
      player_name = console.input_alert('What is your name? ').title()
    f.write(player_name)

You may notice that I do not close the files in either case. That is because the with statement takes care of that for us. See Understanding Python's "with" statement.

\$\endgroup\$
  • \$\begingroup\$ Nice answer. However, I would be careful checking for equality to None. Also, I would add a quick section describing why you used with instead of the OP's method of open/close. \$\endgroup\$ – BeetDemGuise Jun 30 '14 at 13:14
  • \$\begingroup\$ @hansn that's why I didn't pick this as the answer at first. The edit that inverted the logic had me confused. \$\endgroup\$ – tda Jul 2 '14 at 20:51
4
\$\begingroup\$

Don't Repeat Yourself

The first thing that jumps to the eye is the duplication in your code:

  • 'names.json' appears 4 times: you should not duplicate string literals like this, put it in a variable instead
  • console.input_alert('What is your name? ').title() appears twice: you should not duplicate logic, put it in a function instead

Leaked resources

You open names.json at multiple points and don't always close the filehandle. This is a bad practice and could lead to strange bugs. As others have pointed out, use the with ... idiom, and try to organize your code to do it at one common place.

Naming

names.json is not a good name, for several reasons:

  • The .json extension is misleading, because you're using plain text format, there's no JSON anywhere in your code
  • The plural "names" suggests there will be more than one names, but this doesn't seem to be the case for you

Perhaps a better, more future-proof name would be user.config.

Encapsulation

Configuration management sounds like a good subject to capture in a class.

Putting it all together...

I would rewrite like this:

CONFIGFILE = 'user.config'


class ConfigManager(object):
    def __init__(self, path):
        self.path = path

    def load_name(self):
        try:
            with open(self.path) as fh:
                line = fh.readline()
                if line.istitle():
                    return line
        except IOError:
            return

    def save_name(self, name):
        with open(self.path, 'w') as fh:
            fh.write(name)


def ask_name():
    return raw_input('What is your name? ').title()


config = ConfigManager(CONFIGFILE)
name = config.load_name()
if not name:
    name = ask_name()
    config.save_name(name)

print 'So your name is {} ...'.format(name)
\$\endgroup\$
1
\$\begingroup\$

Since you have named the file 'names.json', it would seem more natural to use JSON. Leaving that aside for now, I'll first review your current code, then suggest new:

if os.path.isfile('names.json'): # if there is a file with names...
    text_file = open("names.json") # only open it in normal mode
else: # if not...
    text_file = open("names.json", "w+") # make one

Two observations. First, like hansn's answer notes, a with block is the cleanest way to manipulate files. Second, it is often more Pythonic to "try and fail" than test and do (or as the docs put it, EAFP).

So, I would do something like:

filename = 'names.json'
try:
    with open(filename) as f:
        name = parse_name(f.read())
except OSError: # also any parse error parse_name may raise
    with open(filename, 'w') as f:
        name = ask_name()
        f.write(format_name(name))

(Of course, if you just want write the name as a line, you can do away with parse/format and just read/writeline.)


Now, if you wanted to use JSON, parse/format would have natural implementations:

try:
    with open(filename) as f:
        name = json.load(f)
except OSError, ValueError:
    with open(filename, 'w') as f:
        name = ask_name()
        json.dump(name, f)

(Note that the above doesn't check that the json really contains a string, but why shouldn't someone be named [42];)

Now the only thing left would be ask_name(), which could just be console.input_alert('What is your name? ') if you don't care about name format or length.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.