The following code will generate a number plate and then generate a random speed and see if it's speeding

import random, string, time

class Car:
    def __init__(self):
        self.plate = self.genPlate()
        self.limit = 60
        self.checkSpeeding()

    def checkSpeeding(self):
        self.speeding = random.randint(0, 1)

        if self.speeding:
            self.speed = random.randint(self.limit+3, self.limit+20)
        else:
            self.speed = random.randint(self.limit-20, self.limit+2)

    def genPlate(self):
        plateFormat = ['L', 'L', 'N', 'N', 'L', 'L', 'L']
        genPlate = []

        for i in plateFormat:
            if i == 'L':
                genPlate.append(random.choice(string.ascii_letters[26:]))

            elif i == 'N':
                genPlate.append(str(random.randint(0, 9)))

        genPlate.insert(4, " ")

        return  "".join(genPlate)


allCars = []
x = 0

while True:
    allCars.append(Car())

    print(allCars[x].plate + " was going " + str(allCars[x].speed) + "mph in a " + str(allCars[x].limit) + "mph zone")
    if allCars[x].speeding:
        print("speeding fine issued")

    print("\n")

    time.sleep(5)
    x += 1

Here is a link to the original source

  • 4
    Welcome to Code Review! I have fixed the formatting (select the code, press ctrl-k to indent all lines by 4 spaces). You should edit your question though, and add a better description of what your code does, and what you would like reviewed. – rolfl Jan 3 '15 at 14:18
  • 2
    In my opinion, the check_speeding method should not mutate anything. You could have another method like change_speed and only then call check_speeding that returns a boolean. But then you would need to store the previous speed or keep the acceleration history. – Eliseu Monar dos Santos Jan 3 '15 at 19:45
  • @EliseuMonar with a little more effort, you could post that as an answer. – RubberDuck Jan 4 '15 at 13:31
  • Please do not remove the code as it will make your question off topic. – Hosch250 Aug 20 '16 at 1:41
up vote 11 down vote accepted

I am going to improve the following:

allCars = []
x = 0

while True:
    allCars.append(Car())

    print(allCars[x].plate + " was going " + str(allCars[x].speed) + "mph in a " + str(allCars[x].limit) + "mph zone")
    if allCars[x].speeding:
        print("speeding fine issued")

    print("\n")

    time.sleep(5)
    x += 1

Your x variable should be eliminated:

allCars = []

while True:
    allCars.append(Car())

    print(allCars[-1].plate + " was going " + str(allCars[-1].speed) + "mph in a " + str(allCars[-1].limit) + "mph zone")
    if allCars[-1].speeding:
        print("speeding fine issued")

    print("\n")

    time.sleep(5)

[-1] is the last element and append puts the element in last position.


Actually you don't even need a list:

while True:
    new_car = Car()

    print(new_car.plate + " was going " + str(new_car.speed) + "mph in a " + str(new_car.limit) + "mph zone")
    if new_car.speeding:
        print("speeding fine issued")

    print("\n")

    time.sleep(5)

The print message is very long and not nice, instead use .format():

while True:
    new_car = Car()

    print("{} was going {} mph in a {} mph zone".format(
        new_car.plate,new_car.speed,new_car.limit))
    if new_car.speeding:
        print("speeding fine issued")

    print("\n")

    time.sleep(5)

Adding dots to the end of phrases, Capitalizing the first letter, removing unecessary statement and sleeping less:

while True:
    new_car = Car()

    print("{} was going {} mph in a {} mph zone.".format(
        new_car.plate,new_car.speed,new_car.limit))
    if new_car.speeding:
        print("Speeding fine issued.\n")

    time.sleep(1)

You should use a main function and if __name__ == "__main__": so that you will be able to import your module without running it:

def main():
    while True:
        new_car = Car()

        print("{} was going {} mph in a {} mph zone.".format(
            new_car.plate,new_car.speed,new_car.limit))
        if new_car.speeding:
            print("Speeding fine issued.\n")

        time.sleep(1)

if __name__ == "__main__":
    main()

You should always strive to make the code the simpler possible.


About the rest of the code, minor remarks:

  • You should use names_with_underscores and not camelCase because of PEP8
  • 60 is hardcoded, it would be nicer to pass in a limit parameter when generating a new car.

Consider the real world scenario a little bit. Does a car know what the speed limit is? Does it issue itself a fine if it's speeding? Or do the police do this? Assuming we're talking about the current state of things and not some near future world where cars do these things, police do.

A more proper OOP approach would be to create a Policeman class with a IsSpeeding method that takes a Car in as an argument. The Policeman would then be responsible for determining whether or not the car was speeding and issuing tickets when necessary.

  • 1
    +1, Although the OOP approach isn't always appropriate. In this case an is_speeding function would suffice. If you want to keep the current behaviour, an argument to the constructor would be more appropriate. – Veedrac Jan 3 '15 at 16:00
  • 2
    You're right @Veedrac, OOP isn't always best and I don't actually know if OOP or FP is more common in Python. I'm assuming that this is an educational exercise though. So, I feel it's an important concept to think about how a class models its real world equivalent. – RubberDuck Jan 3 '15 at 16:05

I am going to get pedantic about formatting (see PEP-0008). I have left comments about the formatting in the code below.

# Each import gets its own line.
import random
import string
import time

# Always inherit classes from object when you are not inheriting
# from another class.
class Car(object):
    def __init__(self):
        # Naming: camelCase is generally frowned upon, classes
        # get PascalCase and vars and functions have_underscores.
        self.plate = self.gen_plate()
        self.limit = 60
        self.check_speeding()

    def check_speeding(self):
        self.speeding = random.randint(0, 1)

        if self.speeding:
            # Operators always have spaces before and after them.
            self.speed = random.randint(self.limit + 3, self.limit + 20)
        else:
            self.speed = random.randint(self.limit - 20, self.limit + 2)

    # Does not really need any access to Car's members.
    @staticmethod
    def gen_plate():
        # You can iterate over a string just fine.
        plate_format = "LLNN LLL"
        # Avoid confusion between Car.gen_plate and the list
        plate = []

        for i in plate_format:
            if i == 'L':
                plate.append(random.choice(string.ascii_letters[26:]))
            elif i == 'N':
                plate.append(str(random.randint(0, 9)))
            else:
                # No need for inserts, it is better to do everything
                # in one place.
                plate.append(i)

        return "".join(plate)

# So that it does not start executing if someone loads your code
# as a library.
if __name__ == "__main__":
    # Removed x and all_cars because you never access any
    # data from previously created cars.
    while True:
        car = Car()    
        # String concatenation is not very readable.
        print("%s was going %i mph in a %i mph zone" %
              (car.plate, car.speed, car.limit))
        if car.speeding:
            print("speeding fine issued")
        print("\n")
        time.sleep(5)

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.