Create mosaic task

Question

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:

• Required libraries: http.server, numpy, opencv-python
• Github: https://github.com/Krzysztof-Wojtczak/Allegro-Task
• Run server.py

• In your browser type: http://localhost:8080/mozaika?losowo=Z&rozdzielczosc=XxY&zdjecia=URL1,URL2,URL3...

  where: losowo - optional parameter, if Z = 1 images places are random. rozdzielczosc - optional parameter, defines width and height. Default is 2048x2048 URL1,URL2,URL3... image addresses, 1 to 9.(or copy the link above).

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()

Answer 1

• 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.

• Plajerity: 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?
• Peilonrayz: @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.
• Plajerity: 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".
• Peilonrayz: @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.
• Plajerity: 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.