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 am doing my homework and i have done this? Is there any other better way to do this?

def write_log(name, total):
    # change user input with shape names
    name = name.replace('1', 'triangle')
    name = name.replace('2', 'square')
    name = name.replace('3', 'rectangle')
    name = name.replace('4', 'pentagon')
    name = name.replace('5', 'hexagon')
    name = name.replace('6', 'octagon')
    name = name.replace('7', 'circle')

    total = int(total)

    logfile = open("text.txt", "a")

    if (total == 1):
       towrite = ('%i %s \n'
                  % (total, name))

    elif (total >= 2 and total <= 5):
       name += 's' # add s to name to make it plural
       towrite = ('%i %s \n'
                  % (total, name))
    else:
       towrite = ''
    try:
       logfile.write(towrite)
    finally:
       logfile.close()
    return
share|improve this question

migrated from stackoverflow.com Jan 16 '12 at 6:47

This question came from our site for professional and enthusiast programmers.

4 Answers 4

up vote 7 down vote accepted

I'd find it a bit more elegant to use a list of tuples:

replacements = [('1', 'triangle'),
                ('2', 'square'),
                # etc.
               ]

for old, new in replacements:
    name = name.replace(old, new)

This way, you can more easily change the code later on; e.g., when you decide you want to use re.sub instead of .replace.

Also, I'd use the with statement for the logfile:

with open("text.txt", "a") as logfile:
    # write stuff

This saves you the trouble of closing the file explicitly, so you can remove the exception-handling logic.

share|improve this answer
    
Use enumerate to build the dictionary as I show in my answer. Then just access the dictionary using the number as the key. +1 for using with. –  aculich Jan 16 '12 at 21:45

-- One --

You can use a dictionary to hold your translation data:

mydict = {'1':'triangle', '2':'square', '3':'rectangle',
          '4':'pentagon','5':'hexagon', '6':'octagon','7':'circle'}

Now your list of replacement lines becomes simply:

for key, value in mydict.iteritems():
    name = name.replace(key, value)

another option would be:

for key in mydict:
    name = name.replace(key, mydict[key])

To make this part complete, consider also a tuple for holding your data as indicated in @larmans answer. See also comments in the answers. I think there are interesting hints about which could be the best alternative for different cases.

-- Two --

also remember you can do

2 <= total <= 5

instead of

total >= 2 and total <= 5

-- Three --

Note you dont need parenthesis in the if expressions. Just do this:

if total == 1:
share|improve this answer
2  
No need for a dict, since there's no lookup going on. A list of tuples would suffice. –  larsmans Jan 15 '12 at 17:32
    
But better create the dict like this: mydict = {'1':'triangle', '2':square ... } –  evotopid Jan 15 '12 at 17:32
    
And who said a "list of tuples" is better than a dict for anything? A dict is simpler to input and lighter on the system. No need for a listof tuples in there. –  jsbueno Jan 15 '12 at 17:36
    
This doesn't work as written. The mydict= line is a syntax error, and iterating over mydict (if it were a dict) gives the keys, not (key, value) pairs. –  DSM Jan 15 '12 at 17:42
    
@larsman, use of a dict for lookup is a very common strategy. I'm not sure if a tuple is a bit more light or a bit more faster, but I think a dict is a good thing to show for a homework. He want to translate numbers into figures, so lets provide him with a dictionary ! –  joaquin Jan 15 '12 at 17:43

Below I've answered the question about a better way to write the replacement, but I've also re-written your code in some other ways that I will explain below how and why I changed it the way I did.

Starting with def write_log(name, total): it would be better to use a more meaningful variable name, num, since you expect a number which you'll later be converting into a name.

It is good that you used total = int(total) to make sure that the input is, indeed, a number. We'll do that with the new variable num, as well. Note, that if this fails it will throw a ValueError exception.

Next, since you have a list of names, make them explicitly a list with ['triangle','square'...]. Here I also give it a meaningful variable name, shapes.

For the replacements, this is a good candidate for using a dictionary data structure. And since you want to number each of these names in order you can use the enumerate function; note that by default it will start the numbering from 0, but since you want to start from 1 in your example, I added the starting number as the second argument to enumerate.

Once you have enumerated your shapes list into a dictionary you can now use num as a key to get the value (the shape name). I wrapped it in a try ... except block in case someone called write_log with an invalid value. If an exception happens, then we print an error to stderr and then return from the function. I used this exception handling as an example to show when you would want to use return; in this case we are returning from the middle of the function. Note that I removed the return at the end of your program because you don't have to explicitly write return at the end unless you are actually returning some value, but in your example your function returns nothing at the end, so there is no need to write it out explicitly there at the end.

For pluralization we can simplify further by separating out the pluralization from the log message itself. We only append an 's' if total is greater than 1; I'm not sure when you had total <= 5, but if you really only wanted pluralization for >=2 and <=5, then you could write it that way, but >1 seems to make more sense here. Then building the string for the log message is the same whether or not name is plural.

To open the log file it is better to use the with statement. That will put a try ... finally block around the code so that the file is automatically closed once the logfile variable is no longer in scope (including if an exception is thrown). What you wrote here is definitely correct (and good practice), but since it is such a common pattern the with statement was created for exactly this purpose (see PEP 343).

#!/usr/bin/env python

import sys

def write_log(num, total):
    # ensure that inputs are numbers
    num = int(num)
    total = int(total)

    shapes = ['triangle', 'square', 'rectangle', 'pentagon', 'hexagon', 'octagon', 'circle']
    replacements = dict(enumerate(shapes, 1))
    try:
        name = replacements[num]
    except KeyError:
        sys.stderr.write('Error: %i is not a valid input.\n' % (num))
        return

    if total > 1:
        name += 's' # add s to name to make it plural

    with open('text.txt', 'a') as logfile:
        msg = '%i %s \n' % (total, name)
        logfile.write(msg)

def main():
    write_log(1, 2)
    write_log(23, 5) # this will generate an error
    write_log(3, 4)

if __name__ == "__main__":
    main()
share|improve this answer
    
+1, I didn't know enumerate took a second argument. –  larsmans Jan 16 '12 at 23:02

If you want something a bit fancy you can use re.sub:

import re
SHAPES = {
    '1': 'triangle',
    '2': 'square',
    '3': 'rectangle',
    '4': 'pentagon',
    '5': 'hexagon',
    '6': 'octagon',
    '7': 'circle',
}

name = re.sub('[1-7]', lambda m: SHAPES[m.group(0)], name)

Also for the output probably use a single test and format:

if 1 <= total <= 5:
    towrite = ('%i %s%s \n' % (total, name, "s" if name > 1 else ""))

Note that Python lets you run similar conditions together so you don't need to use and for this sort of condition.

share|improve this answer
    
+1 for re.sub. –  larsmans Jan 15 '12 at 18:08
    
@Duncan Use enumerate to build the dictionary as I show in my answer. Also, once you have the dictionary, there is no need to use re when you can just access the dictionary using the number as the key. –  aculich Jan 16 '12 at 21:44
    
@aculich, using re.sub means a single pass through the string so it scales much better than lots of repeated replace calls. That isn't important here, but it can be significant in other similar situations. –  Duncan Jan 16 '12 at 22:02
    
@Duncan I would consider that to be premature optimization and harder to maintain in a way that will lead to bugs. For example, if they added three more shapes, so the total was 10, then your code would produce the incorrect result: triangle0. And even if you correct the regular expression in re.sub(), it is still worth using dict(enumerate()) on the list to make the code easier to maintain, for example if they wanted to start the numbering from 0 or to re-order items in the list. –  aculich Jan 16 '12 at 23:11
    
@aculich hence the 'If you want something a bit fancy'. I wasn't proposing it as better than the accepted solution for this case but as an alternative technique that is worth knowing. I'm agnostic about enumerate here: you have to either convert the matching groups to int or str the keys so it adds complexity. Also say it was mapping error numbers to messages then quite likely the numbers would soon cease being consecutive and you have to go back to the literal dictionary. As for the regex going wrong: \d+ and using SHAPES.get(m.group(0), m.group(0)) sorts that one. –  Duncan Jan 17 '12 at 8:43

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.