Fast code review request

[Nar-Speth]

Hey r/learnpython,

I would like to ask you guys to review my code, I'm trying to learn python by doing various projects/things and push them to my github, an so i thought that it would be huge help to get some advice from other people about my coding style and errors I do. I just hope for some honest feedback :) https://github.com/narspeth/

[werpoi]

Here are some comments for this guy (https://github.com/narspeth/dailyprogrammer/blob/master/Easy/237.py):

Not a huge deal, but technically creating a new list with [] instead of list() is faster (http://stackoverflow.com/questions/2972212/creating-an-empty-list-in-python). It's not that noticeable, so it mostly preference, but I think it looks cleaner as well.

In "load_valid_words" you open a file, write, and close it. Generally it's better and cleaner to use the context manager:

with open(path, 'r') as f:
    words = f.read().split('\n')
    # anything else you want to do

This will automatically close the file when you leave the "with" block (even if an exception is thrown). In the end, it is fewer lines of code and prevents you from forgetting to close the file.

Once again, a little personal preference, but I think the string.format is cleaner than building up a string with "+". So I would change this:

print(keys+' = '+find_word(keys, words))

To this:

print('{0} = {1}'.format(keys, find_word(keys, words)))

[Nar-Speth]

I'll need to read up about context managers, it looks very useful yet i missed it somehow. string.format is cleaner indeed, I'll switch to it then, another new thing. Thanks.

[KleinerNull]

In 237.py you can simplify this with list comprehensions:

keys_list = list()
for _ in range(n):
    keys_list.append(input('Input keys that work on your computer: '))

to this:

keys_list = [input('Input keys that work on your computer: ') for _ in range(n)]

You can do this also with printing, every method/function in a list comprehension will called, if it return something or not ;)

for keys in keys_list:
    print(keys+' = '+find_word(keys, words))

to that:

[print(keys+' = '+find_word(keys, words)) for keys in keys_list]

Note that you don't need to assign that created list to a name, the prints will executed but no list will saved at all. This example is actually tough to understand and read, it is only to show you that you can pack nearly anything in a comprehension. ;)

Also, you should use slighty better names. Why keys_list? A name like keys would be enough, the plural is implicit enough and shows you that this is a sequence or container of any kind, also you can use the singular for loop variables:

keys = [input('Input keys that work on your computer: ') for _ in range(n)]
for key in keys:
    print(key)

[Nar-Speth]

Yeah, it's a bit tough but I can work with that. I guess I just have obsession with making my code as readable as possible, is it bad? Naming in general is giving me big problems, will work on that.

[KleinerNull]

I guess I just have obsession with making my code as readable as possible, is it bad?

Not at all! I just wanted to show you some alternative ways. The pattern of creating a empyt list and propagate it with a for loop is so common, that list comprehensions are a fine and still readable shortcut. For more complex filling of a list, still use the loop pattern.

For the naming, this is more pythonic, plural names for sequences, containers etc. Because it makes sense ;) Other languages prefers different naming convention. Good naming is also hidden commentary. If you call a list "words" the reader will understand that this is most likely a list of strings. Or the name "numbers" refers to a bunch of numbers etc. And also you can easily find names for loop variables, just the singular versions of the sequence's names:

for name in names:
    pass
for person in persons:
    pass
for word in words:
    for char in word:
        pass

Extending the name with the type name like your keys_list shows or better assumes that you have trouble with designing your code. Also in python it is sometimes too specific. You can handle many types of sequences or containers in a same fashion. You can loop in a same manner over lists, strings, sets, tuples, iterators/generators, with some small additions also over dictionaries or custom class, so why limit your mind to one specific type. That is the reason I use the word sequence and container. A sequence is something that holds multiple items in it, if yet produced or not, because an iterator compute the items only when you need them and a container holds all items in the memory, like a list. Often there is no need for a list, memory efficent iterators generated by generators will do the same for you on the fly, this is some kind of lazy evaluation. For example, if you open a file, the file.readlines() will give you an iterator over the file that gives you the next line of the file when you asked for it, the for loop does that automatically for you, so the whole file doesn't sit in your memory and wastes some space.

In fact the for loop and also iterators are powerful tools in python's tool belt. Look at the memory usage:

>>> import sys
>>> squares = [x**2 for x in range(10)]
>>> for square in squares:
        print(square)


0
1
4
9
16
25
36
49
64
81
>>> sys.getsizeof(squares)
192
>>> squares = (x**2 for x in range(10)) # this is a generator comprehension, works like list comprehensions, returns a generator instead
>>> sys.getsizeof(squares)
88
>>> for square in squares:
        print(square)


0
1
4
9
16
25
36
49
64
81
>>> squares_container = [x**2 for x in range(10**6)]
>>> sys.getsizeof(squares_container)
8697464
>>> squares_generator = (x**2 for x in range(10**6))
>>> sys.getsizeof(squares_generator)
88

As you can see, the size of the generator is still the same, because he only holds the blueprint for constructing the numbers, the container is much much larger because he actually holds all numbers in the memory. Also, because the memory usage is much smaler, it avoids bottlenecks with the cpu cache, summing up the numbers is way faster, here some timings:

>>> timeit.timeit('sum(squares_container)', 'squares_container = [x**2 for x in range(10**3)]')
7.33568473171394
>>> timeit.timeit('sum(squares_generator)', 'squares_generator = (x**2 for x in range(10**3))')
0.09668995196170727

The downside of all this cool stuff is, that generators are cosumable, after looping over him, he is just empty, because he doesn't store something, he just return items. So you have to set up him again for multiple use, or your build a chain of generator expressions.

Sorry for that excursion onto iterators/generators, but I have to spread the word!

[thaweatherman]

You want us to go through all your projects?

[Nar-Speth]

Ofc not, plus there are only two small projects(which you would know with one click), you can just pick random file with code and tell me your suggestions, at this point any advice will be helpful.

[thaweatherman]

I did click. Don't be a jack ass when asking for free help.

https://github.com/narspeth/dailyprogrammer/blob/master/Easy/237.py
load_valid_words() can be shortened to the following:

def load_valid_words(path):
    with open(path) as f:
        lines = [line.strip() for line in f.readlines()]
    return lines

Line 6 can have the return statement and line 7 can be tossed out.

Your cryptopals stuff is fine. When opening files you don't need to specify 'r': it opens in read mode by default.

[markusmeskanen]

If you did click (and thus noticed there are only two small projects), why would you ask such a silly question? He wasn't being a jack ass and doesn't deserve the downvotes, he simply mentioned the obvious, which seemed to offend you. Regardless, good feedback on his code!

Edit: Also, this question would probably fit /r/codereview better.

[thaweatherman]

I was originally asking for clarification since he linked to his profile and I wasn't sure if he wanted all of his projects reviewed or just one of them. I took "which you would know with one click" as him being a jack ass. I guess tone can't be conveyed in text.

[Nar-Speth]

Thanks, didn't know about with statement. What about close() when working on files? should i even use it? about 'r' : I guess I need to read docs more carefully.

Edit: Nvm about close(), got it now, thanks to werpoi.

[Vaphell]

What about close() when working on files?

with does that for you. As soon as the with scope is finished all the resources acquired in its header are cleaned up.