Code review - #1
Conversation
| import requests | ||
| from bs4 import BeautifulSoup | ||
| import pandas as pd | ||
| import numpy as np |
| from PIL import Image | ||
| import signal | ||
| import platform | ||
| import threading |
There was a problem hiding this comment.
Using threading.timer to provide timeout mechanism for windows
| if self.os_is_windows: | ||
| # For better portability a timeout class for windows is needed | ||
| self.timer = threading.Timer(self.seconds, self._raise_timeout) | ||
| self.timer.start |
There was a problem hiding this comment.
threading.timer won't interrupt any main thread in case it's in blocking operation (i,e I/O or sleep) it waits for it to complete and raises an exception
| # soup = BeautifulSoup(htmldata, 'html.parser') | ||
| # for item in soup.find_all('img'): | ||
| # print(item['src']) | ||
| number_of_images) No newline at end of file |
There was a problem hiding this comment.
Removing commented code to provide readability
|
|
||
| def connect(): | ||
| @contextmanager | ||
| def get_db_connection(): |
There was a problem hiding this comment.
establishing DB connection manually using connect function is error-prone, instead opt for a context manager to ensure safely opening and closing connection
| @@ -1,166 +1,131 @@ | |||
| import psycopg2 | |||
| from configparser import ConfigParser | |||
| from contextlib import contextmanager | |||
There was a problem hiding this comment.
context manager in python provides a better way to allocate and release resources
|
|
||
| result = cursor.fetchall() | ||
| for row in result: | ||
| print(f"id {row[0]} | username : {row[1]} | password : {row[2]}") |
There was a problem hiding this comment.
avoid displaying password
| number_cam += 1 | ||
|
|
||
| print("Number cameras = ", number_cam) | ||
| try: |
There was a problem hiding this comment.
adding exception handling to database functions is mandatory to avoid any crash in case of an incorrect SQL query plus the privilege of providing meaningful logging information
| for row in fall_alerts: | ||
| print(f"ID : {row[0]} | alert time : {row[1]} | video link : {row[2]} | class : {row[3]}") | ||
|
|
||
| def retrieve_robbery_alerts(): |
There was a problem hiding this comment.
leveraging code_reusebility is important here as a centralized function with an additional parameter to decide which table to handle is a better approach
|
|
||
| def storeFallAlertData(alertTime, videoLink, AlertClass): | ||
|
|
||
| # Establishing Connection to DB |
There was a problem hiding this comment.
here it's clear there is huge similarity between store alerts functions in a way that it should be centralized into a single function
| with get_db_connection() as (cursor, conn): | ||
| query = "insert into fall_alerts (alert_time, video_link, class) values (%s, %s, %s)" | ||
| cursor.execute(query, (alertTime, videoLink, AlertClass)) | ||
| conn.commit() | ||
|
|
||
| def storeRobberyAlertData(alertTime, videoLink, AlertClass): | ||
|
|
||
| # Establishing Connection to DB | ||
| with get_db_connection() as (cursor, conn): | ||
| query = "insert into robbery_alerts (alert_time, video_link, class) values ( %s, %s, %s)" | ||
| cursor.execute(query, (alertTime, videoLink, AlertClass)) | ||
| conn.commit() | ||
| try: | ||
| cursor.execute(query, (alert_time, video_link, alert_class)) | ||
| conn.commit() | ||
| except Exception as e: | ||
| print(f"Error inserting alert into {alert_type}_alerts: {e}") | ||
|
|
There was a problem hiding this comment.
always ensure handling exception in case of an error in the given SQL query
|
No newline at end of file |
||
|
|
||
| def remove_camera(camera_id): | ||
|
|
||
| with get_db_connection() as (cursor, conn): | ||
|
|
||
| try: | ||
| query = "DELETE FROM cameras WHERE id = %s" | ||
| cursor.execute(query, (camera_id)) | ||
| conn.commit() | ||
| print(f"Camera with ID {camera_id} removed successfully.") | ||
| except Exception as e: | ||
| print(f"Error removing camera: {e}") |
There was a problem hiding this comment.
A new function to remove a camera from database can be very useful
| driver_path = '/home/iheb/chromedriver' | ||
| output_path = 'data/images/robbery_images' | ||
| number_of_images = 1000 | ||
| GET_IMAGE_TIMEOUT = 2 | ||
| SLEEP_BETWEEN_INTERACTIONS = 0.1 | ||
| SLEEP_BEFORE_MORE = 5 | ||
| IMAGE_QUALITY = 1024 | ||
| search_terms = ["armed robbery", | ||
| "shop robbery", | ||
| "man wearing robber mask", | ||
| "man wearing robber mask and knife", | ||
| "shop armed looting", | ||
|
|
||
| "persons", | ||
| "store customers", | ||
| "faces", | ||
| "covid mask", | ||
| "person portrait", | ||
| "full body person portrait", | ||
| "person smiling"] | ||
| # search_terms = ["armed masked thief"] | ||
|
|
There was a problem hiding this comment.
removing global variables will improve flexibility and maintainability
| with webdriver.Chrome(executable_path=driver_path) as wd: | ||
| res = fetch_image_urls(search_term, number_images, wd= wd, sleep_between_interactions=SLEEP_BETWEEN_INTERACTIONS) | ||
| with webdriver.Chrome(executable_path=config.driver_path) as wd: | ||
| res = fetch_image_urls(search_term, number_images, wd= wd, sleep_between_interactions=config.SLEEP_BETWEEN_INTERACTIONS) |
There was a problem hiding this comment.
after removing global variables we can directly use config instance attributes
| if number_cam < 4: | ||
| query = "insert into cameras (address, nom) values (%s, %s)" | ||
| cursor.execute(query, (address, nom)) | ||
| conn.commit() | ||
| else: | ||
| print("Maximum number of cameras added already") | ||
| if number_cam < 4: | ||
| query = "INERT INTO cameras (address, nom) VALUES (%s, %s)" | ||
| cursor.execute(query, (address, nom)) | ||
| conn.commit() | ||
| else: | ||
| print("Maximun number of cameras added already") | ||
| except Exception as e: | ||
| print(f"Error adding camera: {e}") |
There was a problem hiding this comment.
ensuring the usage of parameterized querries (i,e placeholders like %s) is important to avoid SQL injection
eced453 to
f7893a4
Compare
| TODO: remove cloud connection string | ||
| Initialize the blob service from Azure storage connection string. | ||
|
|
||
| Ensures connection string is retrieved securely from env variables. | ||
| """ | ||
|
|
||
| connect_string = os.getenv('AZURE_STORAGE_CONNECTION_STRING') | ||
|
|
||
| if not connect_string: | ||
| raise EnvironmentError("AZURE_STORAGE_CONNECTION_STRING env variable not set.") | ||
|
|
There was a problem hiding this comment.
raising an exception here in mandatory
| return blob_service_client | ||
|
|
||
| def create_azure_container(): | ||
| def create_azure_container(container_name = "alerts"): |
There was a problem hiding this comment.
avoid hardcoding the container name for better flexibility
|
|
||
| container_name = "alerts" | ||
|
|
||
| container_client = blob_service_client.create_container(container_name) | ||
|
|
||
| try: | ||
| container_client = blob_service_client.create_container(container_name) | ||
| except Exception as e: | ||
| print(f"Error creating container {container_name}: {e}") |
There was a problem hiding this comment.
add exception handling
| current_time = datetime.now() | ||
| current_day = datetime.today() | ||
| current_time = current_time.strftime("%H:%M:%S") | ||
| videoName = videoName + " " + str(current_day) + " "+ current_time + ".mp4" | ||
| blob_client = blob_service_client.get_blob_client(container="alerts", blob=videoName) | ||
| current_time = datetime.now().strftime("%H:%M:%S") | ||
| current_day = datetime.today().strftime("%Y-%m-%d") | ||
|
|
||
| video_name = f"{video_name} {current_day} {current_time}.mp4" | ||
| blob_client = blob_service_client.get_blob_client(container="alerts", blob=video_name) |
| # Converting videoArray ( numpy array ) into video | ||
| # fps = 30 # 25 frames per second | ||
|
|
||
|
|
||
| # print(videoName) | ||
| output = cv2.VideoWriter(videoName, cv2.VideoWriter_fourcc(*'mp4v'), fps, (width, height), True) | ||
| for i in videoArray: | ||
| output.write(i) | ||
| output.release() | ||
|
|
||
| path = "./" + videoName | ||
| # print("path : ", path) | ||
| with open(path, "rb") as data : | ||
| # Uploading video to the cloud | ||
| blob_client.upload_blob(data) | ||
| # Delete file after upload | ||
|
|
||
| os.remove(path) | ||
| try: | ||
| with tempfile.NamedTemporaryFile(suffix=".mp4", delete=False) as temp_video_file: | ||
| output = cv2.VideoWriter(temp_video_file.name, cv2.VideoWriter_fourcc(*'mp4v'), fps, (width, height), True) | ||
| for i in video_array: | ||
| output.write(i) | ||
| output.release() | ||
|
|
||
| with open(temp_video_file.name, "rb") as data: | ||
| # Uploading video to the cloud | ||
| blob_client.upload_blob(data) | ||
|
|
||
| os.remove(temp_video_file.name) # removing temporary file after upload | ||
| return blob_client.url | ||
|
|
||
| except Exception as e: | ||
| print(f"Error uploading video {video_name}: {e}") | ||
| return None |
There was a problem hiding this comment.
using tempfile here is better than hardcoding paths to avoid cross-platform or directory not writable issues
| print('Robbery Robbery') | ||
|
|
||
| logging.debug(f"Robbery detected") |
There was a problem hiding this comment.
instead of using print statements for debugging use logging
| """ | ||
| Cycle function will be automatically called within QValka main process and runs the expected Machine vision analyses | ||
| on the passed frames. | ||
|
|
||
|
|
||
| If robbery detected, The frames of the incident will be stored in the cloud And the admin will be alerted through an SMS using twilio. | ||
|
|
||
| """ |
There was a problem hiding this comment.
it is necessary to add docstrings to describe what each function does to ensure it's maintainability
1 participant
This is a self-done code review to locate areas where improvements can be made