5
\$\begingroup\$

I tried to apply for software engineer - intern with this task, and there is no reply since I sent it, so I guess I failed. Can you give me a direction in what way I should improve my code?

Task: The point of this task is to create a service, which will generate a mosaic for given images downloaded from provided URLs.

mosaic.py takes a list of images in cv2 format (for example jpg) and creates a mosaic from them. server.py allows to run a server on your computer from command line, so by entering localhost:8080 in your web browser you can provide a link with urls. The server downloads all images and passes it to the mosaic function, so the mosaic is displayed in the web browser.

Example with 3 images: When this URL is provided, one of possible outcomes: http://localhost:8080/mozaika?losowo=1&rozdzielczosc=512x512&zdjecia=https://www.humanesociety.org/sites/default/files/styles/768x326/public/2018/08/kitten-440379.jpg?h=f6a7b1af&itok=vU0J0uZR,https://cdn.britannica.com/67/197567-131-1645A26E.jpg,https://images.unsplash.com/photo-1518791841217-8f162f1e1131?ixlib=rb-1.2.1&ixid=eyJhcHBfaWQiOjEyMDd9&w=1000&q=80 enter image description here

To run:

mosaic.py:

import cv2
import numpy as np
import random
from math import ceil

class Mozaika:
    """Class Mozaika takes 1 required attribute: list of images in cv2 format,
    3 optional attributes: random image positioning, width of output image, height of output image.
    Output image is stored in variable 'output_image'.
    Class is looking for the least proportional image and returns it in (0,0) - top left corner if no random positioning"""
    def __init__(self, image_list, losowo, w=2048, h=2048):
        self.losowo = losowo # defines whether image position is random
        self.w = int(w) # width of output image
        self.h = int(h) # height of output image
        self.output_image = 0

        # variables are stored in 3 lists: image_names for sorted name strings, image_list for image in cv2 format,
        # image_dict for height and width for every image
        self.image_names = [] # Names of images

        self.image_list = image_list # list of files (images)
        if self.losowo == 1:
            random.shuffle(self.image_list)

        for i in range(len(self.image_list)):
            self.image_names.append(f"img{i}")

        self.image_dict = {}
        for image in range(len(self.image_list)):
            key = self.image_names[image]
            h, w = self.image_list[image].shape[:2] # height, width of each image
            self.image_dict[key] = [h, w]

        self.how_many_images()

    def how_many_images(self):
        number_of_images = len(self.image_dict) # checks how many images is given
        if number_of_images == 1:
            self.make_square()
            self.resize_huge_image()
        elif number_of_images == 2:
            self.rectangle_image(2)
        elif number_of_images == 3 or number_of_images == 4:
            self.grid2x2()
        elif number_of_images > 4:
            self.grid3x3()

    def rectangle_image(self, images=1): # the least proportional image will become a rectangle
        ratios = []
        self.check_ratio() 
        ratios = [e[2] for e in list(self.image_dict.values())] # getting image ratio(s)
        max_ratio = max(ratios)

        for name, value in self.image_dict.items(): # finding highest/longest image
            if value[2] == max_ratio:
                name_max = name
                list_index_max = self.image_names.index(name)

        if images == 1: # method is called for 1 image
            if self.image_dict[name_max][1] > self.image_dict[name_max][0]: # checks if width or height of the image is greater
                return self.make_horizontal_rectangle(name_max, list_index_max, max_ratio), 0, name_max # return image, horizontal/vertical, name of image 

            elif self.image_dict[name_max][1] < self.image_dict[name_max][0]:
                return self.make_vertical_rectangle(name_max, list_index_max, max_ratio), 1, name_max


        elif images == 2: #it will only work if there are 2 images, creates mosaic of 2 images
            i = 0
            if self.image_dict[name_max][1] > self.image_dict[name_max][0]:
                for name, value in self.image_dict.items(): # checks ratio the least proportional image and decides
                    self.make_horizontal_rectangle(name, i, value[2]) # whether images should be vertical or horizontal
                    i += 1
                self.merge_two_images_horizontally() # merge 2 images with minimum quality loss
            elif self.image_dict[name_max][1] < self.image_dict[name_max][0]:
                for name, value in self.image_dict.items():
                    self.make_vertical_rectangle(name, i, value[2])
                    i += 1
                self.merge_two_images_vertically()

    def check_ratio(self):
        # appends to dictionary height to width (or width to height) ratio
        i = 0
        for image in self.image_dict:
            if self.image_dict[image][0] > self.image_dict[image][1]:
                ratio = self.image_dict[image][0]/self.image_dict[image][1]
            else:
                ratio = self.image_dict[image][1]/self.image_dict[image][0]
            self.image_dict[image].append(ratio)


    def make_square(self):
        # centralizes picture and cuts it so it becomes a square
        i = 0
        for image in self.image_dict.values(): # check in dictionary for width/height
            if image[0] > image[1]:
                cut = int((image[0] - image[1])/2)
                self.image_list[i] = self.image_list[i][cut : -cut, :image[1]] # numpy operation on image
            elif image[0] < image[1]:
                cut = int((image[1] - image[0])/2)
                self.image_list[i] = self.image_list[i][:image[0], cut : -cut]
            i += 1

    def make_horizontal_rectangle(self, name, list_index, ratio):
        # if ratio == 2, it's perfect rectangle. Otherwise it is cut to this ratio
        if ratio < 2:
            cut = int(  (self.image_dict[name][0] - (self.image_dict[name][0] / (2/ratio)))/2  )
            return self.image_list[list_index][cut : -cut, : self.image_dict[name][1]]  
        elif ratio > 2:
            if self.image_dict[name][1] > self.image_dict[name][0]:
                cut = int(  (self.image_dict[name][0] - (self.image_dict[name][0] / (ratio/2)))/2  )
                return self.image_list[list_index][: self.image_dict[name][0], cut : -cut]

    def make_vertical_rectangle(self, name, list_index, ratio):
        if ratio < 2:
            cut = int(  (self.image_dict[name][1] - (self.image_dict[name][1] / (2/ratio)))/2  )
            return self.image_list[list_index][: self.image_dict[name][0], cut : -cut]
        elif ratio > 2:
            cut = int(  (self.image_dict[name][1] - (self.image_dict[name][1] / (ratio/2)))/2  )
            return self.image_list[list_index][cut : -cut, : self.image_dict[name][1]]

    def merge_two_images_horizontally(self):
        # method takes 2 horizontal images and merges them
        self.image_list[0] = cv2.resize(self.image_list[0], (self.w, int(self.h/2)))
        self.image_list[1] = cv2.resize(self.image_list[1], (self.w, int(self.h/2)))
        self.output_image = np.concatenate((self.image_list[0], self.image_list[1]), axis=0)        

    def merge_two_images_vertically(self):
        # method takes 2 vertical images and merges them
        self.image_list[0] = cv2.resize(self.image_list[0], (int(self.w/2), self.h))
        self.image_list[1] = cv2.resize(self.image_list[1], (int(self.w/2), self.h))
        self.output_image = np.concatenate((self.image_list[0], self.image_list[1]), axis=1)

    def resize_huge_image(self):
        # returns one image of the size of the output image
        self.output_image = cv2.resize(self.image_list[0], (self.w, self.h))

    def resize_big_image(self, index):
        # returns one image of 2/3 width/height of the output image
        name = self.image_names[index]
        return cv2.resize(self.image_list[index], (int(self.w/(3/2)), int(self.h/(3/2)))), name

    def resize_medium_image(self, index):
        # returns one image of 1/2 width/height of the output image
        return cv2.resize(self.image_list[index], (int(self.w/2), int(self.h/2)))

    def resize_small_image(self, index):
        # returns one image of 1/3 width/height of the output image
        return cv2.resize(self.image_list[index], (int(self.w/3), int(self.h/3)))

    def grid2x2(self):
        placement = self.put_image2x2() # defines where to put images
        decrease_h = ceil(2*(self.h/2 - int(self.h/2))) # decrease size of output image due to roundings, so there are no black spaces
        decrease_w = ceil(2*(self.w/2 - int(self.w/2)))
        vis = np.zeros((self.h - decrease_h, self.w - decrease_w, 3), np.uint8) # smaller image due to roundings
        num = 0
        for i in range(0,2): # grid 2x2, so 4 squares to fill
            for k in range(0,2):
                vis[i*int(self.h/2) : (i+1)*int(self.h/2), k*int(self.w/2) : (k+1)*int(self.w/2)] = placement[num]
                num += 1
        self.output_image = cv2.resize(vis, (self.w, self.h)) # optional, scales image to match requirements accurately

    def grid3x3(self):
        placement = self.put_image3x3() # defines where to put images
        decrease_h = ceil(3*(self.h/3 - int(self.h/3))) # decrease size of output image due to roundings, so there are no black spaces
        decrease_w = ceil(3*(self.w/3 - int(self.w/3)))
        vis = np.zeros((self.h - decrease_h, self.w - decrease_w, 3), np.uint8) # smaller image due to roundings
        num = 0
        for i in range(0,3): # grid 3x3, so nine squares to fill
            for k in range(0,3):
                vis[i*int(self.h/3) : (i+1)*int(self.h/3), k*int(self.w/3) : (k+1)*int(self.w/3)] = placement[num]
                num += 1
        self.output_image = cv2.resize(vis, (self.w, self.h)) # optional, scales image to match requirements accurately

    def put_image2x2(self):
        placement = [0]*4 # it'll store images
        if len(self.image_names) == 3: # to do if there are 3 images
            rect_image, vertical, name = self.rectangle_image()
            index = self.image_names.index(name)
            self.image_list.pop(index) # deleting rectangle image from image_list, so there will be no duplicates
            other_position = [e for e in range(4)] # 4 possibilities to put 1 image
            if vertical: # 1 vertical image
                rect_image = cv2.resize(rect_image, (int(self.w/2), self.h))
                if self.losowo == 1:
                    position = random.randrange(0,2) # choose random position for image
                else:
                    position = 0                    # or fixed position
                other_position.remove(position) # rectangle image takes 2 places
                other_position.remove(position + 2)
                placement[position] = rect_image[:int(self.h/2), :int(self.w/2)]
                placement[position + 2] = rect_image[int(self.h/2):self.h, :int(self.w/2)]
            else: # 1 horizontal image
                rect_image = cv2.resize(rect_image, (self.w, int(self.h/2)))
                if self.losowo == 1:
                    position = random.randrange(0,3,2) # possible positions are top left and bottom left
                else:
                    position = 0
                other_position.remove(position)
                other_position.remove(position + 1)
                placement[position] = rect_image[:int(self.h/2), :int(self.w/2)]
                placement[position + 1] = rect_image[:int(self.h/2), int(self.w/2):self.w]

            num = 0
            for i in other_position: # after puting bigger image fill other places with smalles images
                placement[i] = self.resize_medium_image(num)
                num += 1
        else: # 4 images
            for i in range(len(self.image_list)): 
                placement[i] = self.resize_medium_image(i) # fill 4 places with medium images

        return placement

    def put_image3x3(self):
        placement = [0]*9
        img2x = [] # list of rectangle images
        img4x = [] # list of big square images
        num_img = len(self.image_names)
        var = 0
        var1 = 0
        while num_img < 9:
            if 9 - num_img < 3: # big image can't fit, increase number of takes space by making rectangles
                img2x.append(self.rectangle_image())
                remove_image = img2x[var][2] # get image name
                self.image_dict.pop(remove_image) # delete image to avoid duplicates (there are 3 places where it is)
                index = self.image_names.index(remove_image)
                self.image_names.remove(remove_image)
                self.image_list.pop(index)
                num_img += 1
                var += 1
            else:
                img4x.append(self.resize_big_image(0))
                remove_image = img4x[var1][1] # get image name
                self.image_dict.pop(remove_image) # delete image to avoid duplicates
                index = self.image_names.index(remove_image)
                self.image_names.remove(remove_image)
                self.image_list.pop(index)
                var1 += 1
                num_img += 3

        biash = ceil(self.h*(2/3) - int(self.h*(2/3))) # image can be to big to fit in square, need to decrease it
        biasw = ceil(self.w*(2/3) - int(self.w*(2/3)))
        other_position = set([e for e in range(9)]) # 9 possible places for one image

        for img in img4x: # takes big image and tries to fit it
            square_img = img[0]
            other_position, position = self.find_big_position(other_position) # find possible position
            placement[position] = square_img[:int(self.h/3), :int(self.w/3)] # top left corner of the image
            placement[position + 1] = square_img[:int(self.h/3), int(self.w/3):int(self.w*(2/3)) - biasw] # top right corner
            placement[position + 3] = square_img[int(self.h/3):int(self.h*(2/3)) - biash, :int(self.w/3)] # bottom left corner
            placement[position + 4] = square_img[int(self.h/3):int(self.h*(2/3)) - biash, int(self.w/3):int(self.w*(2/3)) - biasw] # bottom right corner

        for img in img2x: # takes rectangles and tries to fit them
            rect_image, vertical = img[:2] # check if rectangle is vertical
            if vertical:
                rect_image = cv2.resize(rect_image, (int(self.w/3), int(self.h*(2/3))))
                other_position, position = self.find_vertical_position(other_position) # checks for vertical possibilities
                placement[position] = rect_image[:int(self.h/3), :int(self.w/3)]
                placement[position + 3] = rect_image[int(self.h/3):int(self.h*(2/3)) - biash, :int(self.w/3)]
            else:
                rect_image = cv2.resize(rect_image, (int(self.w*(2/3)), int(self.h/3)))
                other_position, position = self.find_horizontal_position(other_position) # checks for horizontal possibilities
                placement[position] = rect_image[:int(self.h/3), :int(self.w/3)]
                placement[position + 1] = rect_image[:int(self.h/3), int(self.w/3):int(self.w*(2/3)) - biasw]

        num = 0
        for i in other_position: # after puting bigger image fill other places with smaller images
            placement[i] = self.resize_small_image(num)
            num += 1

        return placement

    def find_big_position(self, avaiable_pos):
        # find position for 2/3 width/height image
        myList = avaiable_pos
        mylistshifted=[x-1 for x in myList]
        possible_position = [0,1,3,4] # only possible possisions for big image
        intersection_set = list(set(myList) & set(mylistshifted) & set(possible_position))
        if self.losowo == 1:
            position = random.choice(intersection_set)
        else:
            position = intersection_set[0]
        myList.remove(position) # removes places from other_position, so no other image can take these places
        myList.remove(position + 1)
        myList.remove(position + 3)
        myList.remove(position + 4)
        return myList, position

    def find_horizontal_position(self, avaiable_pos):
        # find position for horizontal rectangle image
        myList = avaiable_pos
        mylistshifted=[x-1 for x in myList]
        possible_position = [0,1,3,4,6,7] # positions where image is not cut in half
        intersection_set = list(set(myList) & set(mylistshifted) & set(possible_position))
        if self.losowo == 1:
            position = random.choice(intersection_set)
        else:
            position = intersection_set[0]
        myList.remove(position) # removes places from other_position, so no other image can take these places
        myList.remove(position + 1)
        return myList, position

    def find_vertical_position(self, avaiable_pos):
        # find position vertical rectangle image
        myList = avaiable_pos
        mylistshifted=[x-3 for x in myList]
        possible_position = [e for e in range(6)] # positions where image is not cut in half
        intersection_set = list(set(myList) & set(mylistshifted) & set(possible_position))
        if self.losowo == 1:
            position = random.choice(intersection_set)
        else:
            position = intersection_set[0]
        myList.remove(position) # removes places from other_position, so no other image can take these places
        myList.remove(position + 3)
        return myList, position

server.py

from http.server import HTTPServer, BaseHTTPRequestHandler
import re
from urllib.request import urlopen
import cv2
import numpy as np
from mozaika import Mozaika


class Serv(BaseHTTPRequestHandler):
    def do_GET(self):
        w = 2048 # default width
        h = 2048 # default height
        losowo = 1 # random image placement = true
        urls = [] # images URLs
        if self.path.startswith("/mozaika?"): # keyword for getting mosaic, URL should be put in format:
            parameters = self.path.split("&") # http://localhost:8080/mozaika?losowo=Z&rozdzielczosc=XxY&zdjecia=URL1,URL2,URL3..
            for par in parameters:
                if par.find("losowo") == -1:
                    pass
                else:
                    losowo_index = par.find("losowo")
                    try:
                        losowo = int(par[losowo_index + 7])
                    except:
                        pass

                if par.find("rozdzielczosc") == -1:
                    pass
                else:
                    try:
                        w, h = re.findall('\d+', par)
                    except:
                        pass

                if par.find("zdjecia=") == -1:
                    pass
                else:
                    urls = self.path[self.path.find("zdjecia=") + 8 :]
                    urls = urls.split(",")

            try:
                image_list = create_images_list(urls)   
                # call mosaic creator
                # 1 required attribute: list of images in cv2 format,
                # 3 optional attributes: random image positioning, width of output image, height of output image
                mozaika = Mozaika(image_list, losowo, w, h)
                img = mozaika.output_image # store output image

                f = cv2.imencode('.jpg', img)[1].tostring() # encode to binary format
                self.send_response(200)
                self.send_header('Content-type', 'image/jpg')
            except:
                self.send_response(404)
            self.end_headers()
            self.wfile.write(f) # send output image
                #return


def url_to_image(url):
    # gets image from URL and converts it to cv2 color image format
    resp = urlopen(url)
    image = np.asarray(bytearray(resp.read()), dtype="uint8")
    image = cv2.imdecode(image, cv2.IMREAD_COLOR)
    return image


def create_images_list(urls):
    # takes URLs list and creates list of images
    image_list = []
    for url in urls:
        image = url_to_image(url)
        if image is not None:
            image_list.append(image)
    return image_list



httpd = HTTPServer(("localhost", 8080), Serv)
httpd.serve_forever()
\$\endgroup\$
4
\$\begingroup\$
  • You have a god-class Mozaika, you should define image mutations on another class Image.
  • You have three mutating containers that hold the information you need. This is really really really bad. If I were an interviewer the second I see that I'd know I wouldn't want you.

    This is because it makes your code hard to read, and really fragile.

Below is what, a segment of, your code would look like without these two massive problems:

import cv2
import numpy as np
import random
from math import ceil


class Image:
    def __init__(self, image):
        self._image = image
        self.height, self.width = image.shape[:2]

    @property
    def ratio(self):
        return max(self.height, self.width) / min(self.height, self.width)

    def square(self):
        if self.height > self.width:
            cut = int((self.height - self.width) / 2)
            return Image(self._image[cut : -cut, :self.width])
        else:
            cut = int((self.width - self.height) / 2)
            return Image(self._image[:self.height, cut : -cut])

    def make_horizontal_rectangle(self):
        ratio = self.ratio
        if ratio < 2:
            cut = int((self.height - ratio * self.height / 2) / 2)
            return Image(self._image[cut : -cut, : self.width])
        elif ratio > 2:
            if self.width > self.height:
                cut = int((self.height - 2 * self.height / ratio) / 2)
                return Image(self._image[: self.height, cut : -cut])
        return self

    def make_vertical_rectangle(self):
        ratio = self.ratio
        if ratio < 2:
            cut = int((self.width - ratio * self.width / 2) / 2)
            return Image(self._image[: self.height, cut : -cut])
        elif ratio > 2:
            cut = int((self.width - 2 * self.width / ratio) / 2)
            return Image(self._image[cut : -cut, : self.width])
        return self

    def resize(self, width, height):
        return cv2.resize(self._image, (width, height))

    def merge(self, other, horizontally=True):
        axis = 0 if horizontally else 1
        return Image((self._image, other._image), axis=axis)


class Mozaika:
    def __init__(self, image_list, losowo, w=2048, h=2048):
        self.losowo = losowo # defines whether image position is random
        self.w = int(w) # width of output image
        self.h = int(h) # height of output image
        self.output_image = 0

        self.images = [Image(i) for i in image_list]
        if self.losowo == 1:
            random.shuffle(self.images)
        self.how_many_images()

    def how_many_images(self):
        number_of_images = len(self.image_dict) # checks how many images is given
        if number_of_images == 1:
            self.output_image = self.images[0].square().resize(self.w, self.h)
        elif number_of_images == 2:
            self.output_image = self.rectangle_image(2)[0]
        elif number_of_images == 3 or number_of_images == 4:
            self.grid2x2()
        elif number_of_images > 4:
            self.grid3x3()

    def rectangle_image(self, images=1):
        largest = max(self.images, key=lambda i: i.ratio)
        maxratio = largest.ratio

        if images == 1:
            if largest.width > largest.height:
                return largest.make_horizontal_rectangle(), 0
            elif self.width < self.height:
                return largest.make_vertical_rectangle(), 1
        elif images == 2:
            # ...

To get a better review you should change the rest of the code to follow the same style the above is. To help you out I'll give you some 'rules':

  • You're only allowed to overwrite self.images.

    This means:

    # Not allowed
    self.images[0] = ...
    images = self.images
    images[0] = ...
    self.images = images
    
    # Allowed
    self.images = [...]
    
    import copy
    images = copy.copy(self.images)
    images[0] = ...
    self.images = images
    

    Mutating data can lead to unpredictable things to happen. Overwriting data allows people to understand everything that's happening. Even if it's more verbose.

    If you post another question someone will probably say my recommendations are bad. And they are in their own way, but doing by following them you'll have gotten rid of some larger problems, that almost makes your code un-reviewable.

  • You're only allowed to overwrite Mozakia.images once per function call.

  • Only Mozaika.images is allowed to contain Images.

    You are allowed local variables that hold Images too. (Like images in the above code snippet.)

  • You're not allowed to touch Image._image outside of Image.

  • Only Image.merge is allowed to be passed another Image.
  • You're not allowed to change Image.merge.

This will mean that your code doesn't abuse mutations, and your code will be split up correctly into different segments. Meaning that it'll be far easier to review.

I highly recommend you follow the above rules and come back and post another question.


Additional notes:

  • All the functions in Image return a new Image, in your code, sometimes the code wouldn't mutate Mozaika.image_list, and so in these cases they return self.
  • Your code looks like it has some bugs, you always do if a > b: elif a < b: never with an else. This means that your code can fail if a == b.
  • make_horizontal_rectangle has an additional if that make_vertical_rectangle. That looks like a bug.
\$\endgroup\$
  • \$\begingroup\$ Thank you! I can see now one more massive problem, where I thought it'll be easier to understand, but no... There are 3 functions merging images, you didn't mention the 2 more important - I should have really deleted function merging 2 images. put_image2x2 and 3x3 are using grids of zeros to put images there. I think they should be in Mozaika class, and Image class will be for operations on single images, right? \$\endgroup\$ – Plajerity Jun 11 at 9:50
  • \$\begingroup\$ @Plajerity As your code because too complex for me to reasonably review, I didn't look at the code after where I put the # .... Whilst I havn't looked at the code, you should be able to convert put_image2x2 and put_image3x3 to use Image.merge. Image is for a single image. Image.merge is to merge two images. Mozaika should only position the images, and defer to Image when it needs to merge or resize an image. Try following the above rules, and post another answer, it will surely benefit you. \$\endgroup\$ – Peilonrayz Jun 11 at 11:18
  • \$\begingroup\$ I feel I should mention it... It's not possible to use Image.merge (with concatenate) for more than 2 images. Concatenate requires to merge images of the same width/height. We would need to write in which order it merges, and it depends on where is rectangle image - random position. When I was writing it, first I created function to merge 2 images, and then realised that it's not possible with more images. Therefore there are more merging functions, and I decided to leave "the simplest one". \$\endgroup\$ – Plajerity Jun 20 at 20:57
  • \$\begingroup\$ @Plajerity I'm not sure this is the case. However I may be wrong. Can you show me an example image that would need these other merge functions, as the one in the OP doesn't. \$\endgroup\$ – Peilonrayz Jun 20 at 21:02
  • \$\begingroup\$ Take a look at given image above. Firstly we need to merge vertically, then horizontally. Otherwise it won't work. If image with the highest ratio was horizontal, we would need to merge 2 smaller images horizontally and then merge them all vertically. There are to many combinations. \$\endgroup\$ – Plajerity Jun 20 at 21:04

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.