I wrote this class to generate a random "image," similar to a QR code, using strings. How can I make it better (I am a beginner in programming)?

import random

""" N is the length of squared field to be generated"""
class genImg():
  def __init__(self, N):
    self.N = N
    self.field = []

  def genField(self):
    for i in range(self.N):
      row = []
      for j in range(self.N):

  def getField(self):
    return self.field

  def fillField(self):
    field = self.getField()
    result = ""
    for i in field:
      row = ""
      for j in i:
        randNum = random.randint(1,100)
        if randNum % 2 == 0 or randNum % 3 == 0:
          j = '|0|'
        row += str(j) + " "
      result += row[:-1] + "\n"
  • 1
    \$\begingroup\$ Can you add an example output? I think it will make the question more attractive. \$\endgroup\$ – 301_Moved_Permanently Jun 27 '18 at 16:48

You are doing unnecessary work by first looping over each row and each column to initialise the field to empty squares; and then looping over each row and each column to change the state based on a random condition. You can do it in one looping by assigning the "empty" or "filled" state based on said condition.

And while we're at it, you could simplify the condition as there is exactly 67 numbers that are either multiple of two or three between 1 and 100. So you could at least test if randNum <= 67 which would yield the same probability. But this probability is pretty close to \$\frac{2}{3}\$ so we can come up with an other approach: use a tupple composed of two " filled" patterns and an "empty" one and use random.choice instead.

You could also benefit a lot from the str.join method instead of concatenating the chosen pattern and a separator to a temporary string. It will help improve your memory usage. Just put the selected patterns in a list and use the join method of your separator to create the whole string at once. Better yet, join over a list-comprehension or a generator expression.

Lastly, you don't need a class to solve this problem as you don't have any state. The various attributes are only there to store temporary variables. A simple function will do:

import random

def generate_image(size, *, filled='|0|', empty='|-|'):
    patterns = filled, filled, empty

    return '\n'.join(
            ' '.join(random.choice(patterns) for _ in range(size))
            for _ in range(size)

if __name__ == '__main__':
| improve this answer | |
  • \$\begingroup\$ What is the advantage of taking more positional arguments and throwing them away with *? Does it guarantee that filled and empty can only ever be passed as keyword arguments? \$\endgroup\$ – Graipher Jun 27 '18 at 18:56
  • 2
    \$\begingroup\$ @Graipher This is keyword-only arguments syntax (aka PEP3102) \$\endgroup\$ – 301_Moved_Permanently Jun 27 '18 at 18:59
  1. genField() generates an empty field. My first assumption given it's name is that it generates a random field. Call it something like genEmptyField() to make this clearer.

  2. fillField() does not work unless genField() is called first. Call genField() at the begining of fillField(). This will also ensure that subsequent calls to fillField() are still random.

  3. fillField() does the work of both generating the code and printing it. Seperate these into two seperate functions.

  4. Make a method __str__(self) of genImg() that returns the string representation of the image, so that the codes that you generate can be printed, rather than them doing the printing.

  5. Consider calling fillField() in __init__() so that the user always has a useful image.

  6. If you want a uniform random image, randNum % 2 == 0 or randNum % 3 == 0 can be replaced by random.getrandbits(1)

  7. In order to seperate generation and representation, consider storing the array of points as bools and then convert them to the string of your choice in __str__(). This makes changing your output format much simpler if you want to do that later.

  8. self.field.append(row[:]) is slower and harder to understand than self.field.extend(row[:])

  9. The name genImg implies that this class generates images. In reality this class is an image and provides methods for generation. Consider naming it something like Image. It is generally a good idea to name your classes with nouns rather than verbs (i.e. ImgGenerator rather than genImg)

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy