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 is a Python script that generates a grid of random HTML colors. It works by reading random data from /dev/urandom and then encoding it as a hexadecimal string. This string is spit into individual values that will be represented as a color.

I am wanting to know if this is the best way to generate random values for this task. Would it be better to use the random module? Or is my method more beneficial for this task?

WIDTH = 64
HIGHT = 5

def chunks(l, n): 
   return [l[i:i+n] for i in range(0, len(l), n)]
data = file("/dev/urandom").read(HIGHT*WIDTH*3).encode("hex")
colors = ""
count = 0
for byte in chunks(data, 6): 
   if count == WIDTH: 
       colors+="<br>"
       count=0
   count+=1
   colors+="""<p style="color:%s; display:inline;">&#x25a0;</p>""" % (byte)
file("colors.html", "w").write(colors)

This is the output:

Screenshot

share|improve this question
add comment

2 Answers

An implementation that uses the random module has the benefit of being platform independent. This code isn't going to run so well if you try it on Windows.

There is also another alternative. os.urandom() will use the underlying os to provide the random values.

On a UNIX-like system this will query /dev/urandom, and on Windows it will use CryptGenRandom().

You can also use random.SystemRandom() if you still want the OS source, but want to use the interface provided by random.


This answer provides an implementation of chunks() that iterates instead of constructing a list of all the chunks.


I would also lean towards an implementation that opens the output file first, then writes to it as it iterates over the chunks. This way, you won't need to build a huge string in memory before writing it to disk. Similarly, I would generate the random values as needed.

share|improve this answer
add comment
  1. By using /dev/urandom you make it hard for someone to run your code on Windows. Instead, use the built-in function os.urandom.

  2. But what are these colours going to be used for? Do you really care whether they are generated using cryptographically secure random numbers? It looks like an application for which ordinary pseudo-random numbers from the random module will be perfectly adequate.

  3. The word "height" has an "e" in it.

  4. str.encode('hex') isn't portable to Python 3. Instead, you should use binascii.hexlify.

  5. The name byte seems like a bad name for a variable that contains a 6-character-long string.

  6. You read a big block of random data and then use the chunks function to split it up. But it would be simpler to read one chunk of random data at a time—then you wouldn't need the chunks function.

  7. Similarly, you build a big string and then write it to the output all in one go. But it would be simpler to open the output file and then write one small string at a time to it.

  8. The entity reference &#x25a0; is unclear. I would use the unicodedata module and write unicodedata.lookup('BLACK SQUARE') so that the meaning is clear

  9. Why do you use <p> if you are going to specify display:inline? Why not use <span> instead?

Putting all this together, I'd write:

from random import randrange
from unicodedata import lookup

WIDTH = 64
HEIGHT = 5
SQUARE = ord(lookup('BLACK SQUARE'))
FORMAT = '<span style="color:{{:06x}}">&#x{:x};</span>'.format(SQUARE)

with open('colors.html', 'w') as f:
    for _ in range(HEIGHT):
        for _ in range(WIDTH):
            f.write(FORMAT.format(randrange(0x1000000)))
        f.write('<br>')
share|improve this answer
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.