Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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()
share|improve this question
add comment

3 Answers

up vote 7 down vote accepted
+50

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.

share|improve this answer
    
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. –  BeetDemGuise Jun 30 at 13:14
1  
Thanks @DarinDouglass. I've made some small changes. –  hansn Jun 30 at 14:46
    
@hansn, your edit inverted the logic. –  otus Jul 2 at 11:33
    
@otus oh, thanks. Fixed it –  hansn Jul 2 at 11:35
    
@hansn that's why I didn't pick this as the answer at first. The edit that inverted the logic had me confused. –  techteej Jul 2 at 20:51
add comment

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.

share|improve this answer
add comment

DRY ("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)
share|improve this answer
    
Dear downvoter, please explain what is wrong here so I could try to fix it... –  janos Jul 7 at 8:41
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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