2
\$\begingroup\$

I recently build a simple image editor in Python. There is nothing specific that I am dissatisfied about regarding this code. I am just asking for general criticism to help me improve.

import pygame
import random
import time

x = input("Do you want to edit an image?")

if  x == "yes":
    image_url = input("please enter image url")
    image = pygame.image.load(image_url) 
    i_edit = True

else:
    i_edit = False



pygame.init()


red = (255,0,0)
orange = (255,165,0)
yellow = (255,255,0)
green = (0,255,0)
blue = (0,0,215)
indigo = (75,0,130)
violet = (128,0,128)
black = (0, 0, 0)
white = (255,255,255)

gameDisplay = pygame.display.set_mode((900, 900))
clock = pygame.time.Clock()

color = white

crashed = False
buttpressed = 0
rightkeypressed = False
radius = 10

background_colour = (0,0,0)
gameDisplay.fill(background_colour)

if i_edit:
    gameDisplay.blit(image, (0, 0))

while not crashed:

    if radius < 5:
        radius = 5
 
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            crashed = True

        pygame.display.set_caption("Your Pen Size is {0}".format(radius))

        if event.type == pygame.KEYDOWN and rightkeypressed is True:
            if event.unicode == 'n':
                background_colour = black
            if event.unicode == 'w':
                background_colour = white
            if event.unicode == 'r':
                background_colour = red
            if event.unicode == 'o':
                background_colour = orange
            if event.unicode == 'y':
                background_colour = yellow
            if event.unicode == 'g':
                background_colour = green
            if event.unicode == 'b':
                background_colour = blue
            if event.unicode == 'i':
                background_colour = indigo
            if event.unicode == 'p':
                background_colour = violet
            if event.unicode == 'e':
                color = background_colour
            gameDisplay.fill(background_colour)
            if event.unicode == 'q':
                gameDisplay.blit(image, (0, 0))


        if event.type == pygame.MOUSEBUTTONDOWN:
            if event.button == 3:
                rightkeypressed = True

        if event.type == pygame.MOUSEBUTTONUP:
            if event.button == 3:
                rightkeypressed = False
        
        if event.type == pygame.KEYDOWN:
            if event.unicode == 'n':
                color = black
            if event.unicode == 'w':
                color = white
            if event.unicode == 'r':
                color = red
            if event.unicode == 'o':
                color = orange
            if event.unicode == 'y':
                color = yellow
            if event.unicode == 'g':
                color = green
            if event.unicode == 'b':
                color = blue
            if event.unicode == 'i':
                color = indigo
            if event.unicode == 'p':
                color = violet
            if event.unicode == 'e':
                color = background_colour
            if event.unicode == '=':
                radius += 5
            if event.unicode == '-':
                radius -= 5

        
        if event.type == pygame.MOUSEBUTTONDOWN:
            buttpressed += 1


        if event.type == pygame.MOUSEMOTION and (buttpressed%2) != 0:
            pygame.draw.circle(gameDisplay, color, pygame.mouse.get_pos(), radius)

    pygame.display.update()
    clock.tick(60)
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Please, anyone that is the first to down vote please leave a comment as to why you are down voting. This is a first time contributor and they may not understand all of our guidelines on how to ask a good question. \$\endgroup\$ – pacmaninbw Aug 1 '20 at 10:29
1
\$\begingroup\$

All in all, the code looks good and the image editor is a lot of fun, I tried it out.

A few little suggestions:

Variables with telling names

The name x is not really a good name, is does not tell you anything about the purpose or the value of the variable:

x = input("Do you want to edit an image?")

I would use a name like user_input or better edit_image_user_input. This makes the code easier to understand. However, I must admit that in this specific case the name is not that important because the variable is already used in the following line, and it is only used once. Still, I think choosing meaningful names is a good habit.

When asking for input, tell the user which options are available and how they should be entered

As a user, when I see a prompt like

"Do you want to edit an image?"

I do not know how I should enter my decision. Shall I type "yes" or is "y" also okay? Is the input case-sensitive? Also, the question could be more specific: "Do you want to edit an image?" -> Of course I want to edit an image, I just opened an image editor. You should tell the user that he can open an image from an URL.

Avoid hardcoding

To make changes to your code easier, replace the magic number 5 in the following piece of code with a variable

if radius < 5:
    radius = 5

I would suggest

minimum_pen_radius = 5 # put this at the beginning of your script or in a separate config file
# [...]
if radius < minimum_pen_radius:
    radius = minimum_pen_radius

You assign the background_colour with a tuple holding the RGB values for black, even though you already defined black a few lines before:

black = (0, 0, 0)
# [...]
background_colour = (0,0,0)

Better do it like this:

black = (0, 0, 0)
# [...]
background_colour = black 

Avoid repetitions

Look at this large block of code:

if event.unicode == 'n':
    background_colour = black
if event.unicode == 'w':
    background_colour = white
if event.unicode == 'r':
    background_colour = red
if event.unicode == 'o':
    background_colour = orange
if event.unicode == 'y':
    background_colour = yellow
if event.unicode == 'g':
    background_colour = green
if event.unicode == 'b':
    background_colour = blue
if event.unicode == 'i':
    background_colour = indigo
if event.unicode == 'p':
    background_colour = violet
if event.unicode == 'e':
    color = background_colour
gameDisplay.fill(background_colour)
if event.unicode == 'q':
    gameDisplay.blit(image, (0, 0))

and this one:

if event.unicode == 'n':
    color = black
if event.unicode == 'w':
    color = white
if event.unicode == 'r':
    color = red
if event.unicode == 'o':
    color = orange
if event.unicode == 'y':
    color = yellow
if event.unicode == 'g':
    color = green
if event.unicode == 'b':
    color = blue
if event.unicode == 'i':
    color = indigo
if event.unicode == 'p':
    color = violet
if event.unicode == 'e':
    color = background_colour
if event.unicode == '=':
    radius += 5
if event.unicode == '-':
    radius -= 5

These are lots of repetitions, within each code block and between them. Something like this is not very flexible and it is a lot to read. For example, if you want to change the key for the color black, you have to do it twice: for background and pen color. You could instead try another approach. First, create a mapping from key to color in form of a dictionary:

key_to_color_map = {'n': black,
                    'w': white,
                    'r': red} # do this for all colors

Then, replace the large block of if statements with

try:
    background_colour = key_to_color_map[event.unicode]
    gameDisplay.fill(background_colour)   
except KeyError:
    if event.unicode == 'q':
        gameDisplay.blit(image, (0, 0))     

For all keys that you defined beforehand you can simply look up the corresponding color in the dictionary. In case the key is not in the dictionary (throws a KeyError), you will jump to the except block. You can do this for the block where you set the background color as well as for the block where you set the pen color, and you can use the same dictionary key_to_color_map for both.

\$\endgroup\$

Your Answer

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

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