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.

This isn't really much of an actual project, just something I threw together in about 5-10 minutes. I just want to know if it goes with the current Python standards, and if anything should be changed or fixed.

print(''.join([__import__('random').choice(['\033[7;1;32m  \033[0m','\033[7;32m  \033[0m','\033[7;1;33m  \033[0m','\033[7;34m  \033[0m']) for world in range(40)])); __import__('time').sleep(0.5)

Can anything be improved? What needs to be fixed?

share|improve this question
3  
Is it a requirement that it is a one-liner (code-wise, not output-wise), and if so, why? –  Jonas Wielicki Jun 23 at 15:09
    
@JonasWielicki I wrote this as a challenge for myself, so my general preference is that it stays at one line. If it goes against standards and has to be on more than one line, I'm okay with that too. –  Vladimir Putin Jun 23 at 15:11
2  
    
@Doorknob Nice! Unutbu? –  Vladimir Putin Jun 24 at 2:05
1  
Yes, I'm running Ubuntu. (I just added a for __ in range(99): to the beginning. :-P) –  Doorknob Jun 24 at 3:39
show 1 more comment

2 Answers 2

up vote 8 down vote accepted

What does THE standard say about imports :

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

What else could easily be improved in your code ?

  • _ is a usual name for variable whose value is not going to be used.
  • you do not need to create a list : you can use generator expressions by getting rid of brackets.
  • you can get rid of the duplicated prefix and suffix in the different choices.
  • in Python 2.*, you do not need parenthesis to call print.

Your code would become :

from random import choice
from time import sleep
print ''.join('\033[7;' + choice(['1;32','32','1;33','34']) + 'm  \033[0m' for _ in range(40))
sleep(0.5)
share|improve this answer
    
+1 for mentioning _ and condensing the list. –  Jonas Wielicki Jun 23 at 15:25
    
Usually when I do complex printing like this I prefer to use print() instead of print. I don't know why, I just do. –  Vladimir Putin Jun 23 at 15:34
    
It’s also more forward-compatible with python3 to do that. –  Jonas Wielicki Jun 23 at 15:46
1  
I'm not quite sure to understand, what is offensive? –  Josay Jun 24 at 6:30
1  
Well I guess, this is just a matter of preference : some might want to keep superfluous parenthesis (being ready for Python 3 is the only valid reason for this IMO), some might want to remove them (pylint will warn you and it corresponds to the zen of Python : simple is better, one obvious way to do it). I don't really care about what you do but AFAIK the code I suggested is valid Python 2. Also, given what OP's code looks code, I assumed that making things more concise would be appreciated. It seems like I am wrong but he's free to decide whatever he prefers. –  Josay Jun 24 at 14:48
show 4 more comments

Of course, you’re violating the 79 characters-per-line limit as per PEP 8, but I guess you know that and thats hard to avoid when you want to stay on one line.

  • You can save some characters and some memory by using generator expressions:

    ''.join([random.choice(…) for world in range(40)]) 
    

    then becomes

    ''.join(random.choice(…) for world in range(40)) 
    

    This prevents that the whole list is created in memory before being joined, and is also more readable, as it saves brackets.

  • Using __import__ is not exactly every-day standard. Unfortunately, there is no other way for handling this in one line. Preferably, you would be using at least three lines:

    import random
    import time  # only if you need the delay
    print(''.join([random.choice(['\033[7;1;32m  \033[0m','\033[7;32m  \033[0m','\033[7;1;33m  \033[0m','\033[7;34m  \033[0m']) for world in range(40)])); time.sleep(0.5)
    

    One issue with __import__ is that it returns the top-level package module instead of the actual module. So __import__("os.path") returns the os module instead of the os.path module, which can be surprising to some and which is why one should avoid using it.

    The suggested alternative is importlib.import_module, but you cannot use this of course unless you first import importlib, defeating the purpose.

There are several other readability issues, but these all relate to the fact that you want to stay on one line, so they’re out of scope for this question.

share|improve this answer
    
I thought it was 80 chars per line. –  Vladimir Putin Jun 23 at 15:37
    
@SomeGuy As said in the document, it should be 79 characters (quoting) “to avoid wrapping in editors with the window width set to 80, even if the tool places a marker glyph in the final column when wrapping lines. –  Jonas Wielicki Jun 23 at 15:47
    
@SomeGuy Meh, you're lucky, limit is 72 characters on punch-card. –  rolfl Jun 23 at 23:09
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.