First of all, I like the idea of the project and although I think it is pretty simple it can also be a good beginners exercise to learn handling with API calls and GUI interfaces. That being said, I've found a lot of errors in the application denoting that it has not been tested sufficiently and I want to share my findings here to help the author with this project and future ones:
Table of Contents
Requirements
The Readme requirements section doesn't gather all the external dependencies necessary for the project to work properly. As you may see in search.py:3 it is required to have installed the requests package, but that is not mentioned in the Readme. For your help, it is always recommended to run
in your project's virtual environment to have the full list of dependencies and, most important, it is a good practice to store all of them inside a text file normally called requirements.txt so the building systems and the rest of the users can install all the dependencies just in one command:
pip install -r requirements.txt
Therefore, in your case you could generate the requirements.txt file as follows:
pip freeze > requirements.txt
Content of requirements.txt:
certifi==2023.7.22
charset-normalizer==3.3.0
darkdetect==0.7.1
idna==3.4
PyQt6==6.5.3
PyQt6-Qt6==6.5.3
PyQt6-sip==13.6.0
pyqtdarktheme==2.1.0
requests==2.31.0
urllib3==2.0.6
I highly recommend you to add the requirements.txt or, if you want to go a step further, consider packaging your project and uploading it to PyPI so it can be installed with pip directly from the command line.
For that task you will need to define a setup.py file or a pyproject.toml file. You can find more information about that in the official documentation. I personally recommend you using the pyproject.toml file because it is easier to define and it is the future of Python packaging and it supports PEP 517 which is the standard for building Python packages.
To have it working with a bit more of excellence, I also recommend you using Poetry to manage your project's dependencies and packaging. It is a great build back-end tool that will help you a lot with the packaging and distribution of your projects. You can find more information about it in the official documentation and it supports pyproject.toml out of the box!.
Errors / Crashes
The app crashes abruptly in many different use cases:
-
If you don't select any image nor write any text in the search bar and click on the search button the app crashes with the following error:
Traceback (most recent call last):
File "AnimeSnap\src\main.py", line 151, in onClickSearch
search.search_img(self, anilist_info=anilist_status, rbb=rbb_status)
File "AnimeSnap\src\search.py", line 15, in search_img
files={"image": open(f"{self.img_path}", "rb")}
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: ''
This happens because there is no error-handling in the code and the self.img_path variable is empty.
-
If you search an image successfully and then you click on the Search Another Image button the app crashes with the following error:
Traceback (most recent call last):
File "AnimeSnap\src\main.py", line 102, in returnToMenu
self.stacked_widget.setCurrentWidget(self.main_menu_widget)
^^^^^^^^^^^^^^^^^^^
AttributeError: 'App' object has no attribute 'stacked_widget'
This happens because the self.stacked_widget variable is not defined in the App class but you are trying to access it modifying the current widget.
-
If you search an image successfully and then you click on the Write to JSON button the app crashes with the following error:
Traceback (most recent call last):
File "AnimeSnap\src\search.py", line 36, in <lambda>
json_button.clicked.connect(lambda: json_operations.write_to_json(self))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "AnimeSnap\src\json_operations.py", line 49, in write_to_json
options = QFileDialog.Options()
^^^^^^^^^^^
NameError: name 'QFileDialog' is not defined
This happens because the QFileDialog class is not imported in the json_operations.py file.
-
If you only write an image URL in the search bar and click on the search button the app crashes with the following error:
Traceback (most recent call last):
File "AnimeSnap\src\main.py", line 153, in onClickSearch
search.search_url(self)
File "AnimeSnap\src\search.py", line 52, in search_url
files={"image": open(f"{self.img_url}", "rb")}
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: [Errno 22] Invalid argument: 'https://example.com'
This happens because the app does not support searching images by URL if you don't provide any Anilist Anime ID. That is a problem because that ID is supposed to be optional, just to retrieve more information.
-
If you select a local image and also provide an Anilist Anime ID the app crashes with the following error:
Traceback (most recent call last):
File "AnimeSnap\src\main.py", line 160, in onClickSearch
json_operations.json_to_tabular(self, iad=False)
File "AnimeSnap\src\json_operations.py", line 7, in json_to_tabular
result = self.a['result']
~~~~~~^^^^^^^^^^
KeyError: 'result'
This happens because the search fails and the self.a variable contains the following dictionary:
{'error': 'Failed to fetch image /path/to/image.jpg'}
and you are trying to access the result key that does not exist.
Code
The code design and structure is far from perfect and there are a lot of thing to mention:
Overall
There are some things all over the code that I want to point out some programming principles and good practices that are not followed:
- Standards and conventions. The code is not following the PEP 8 style guide. I recommend you to use a linter like flake8 or pylint to check your code and make it more readable and maintainable.
- Single Responsibility Principle. The
App class is doing too much work which makes the main.py take almost all the responsibility of the app.
- Model–view–controller (MVC) pattern. The
App class is the controller and it is also doing the work of the model and the view.
- Don't Repeat Yourself (DRY). There is a lot of duplicated code.
- Law of Demeter (LoD) principle. There are a lot of calls to the
self variable in the App class and in the search.py file.
- Tell, Don't Ask principle
search.py
-
Unused Imports: You import os.path and json, but it's not used in the code. Remove unnecessary imports to keep the code clean.
-
Bugs with variables: The variable stranilist_info in line 60 is not defined. I think you meant anilist_info and it's just a typo.
-
Inconsistent Naming: The variable names in the code are inconsistent. For example, you use anilist_info and rbb in some places and stranilist_info in others. Make sure variable names are consistent throughout the code.
-
Magic Numbers: The hardcoded numbers such as 100, 100, 750, 800 for window geometry should be replaced with named constants or variables to improve code readability. The same applies to the hardcoded URLs used for the requests.
-
Code Duplication: There's code duplication in the search_img and search_url functions. You can refactor these into a single function by passing the image path or URL as a parameter, rather than having two separate functions.
-
Function Parameters: The self parameter is present in both functions, which suggests that these functions should be part of a class. However, you haven't provided the class definition or other methods. You should either remove the self parameter or refactor the code to use a class.
-
Useless logic: The conditional flow for the rbb variable value is completely pointless! From lines 18 and 55:
if rbb is False:
url = f"https://api.trace.moe/search?anilistID={anilist_info}&url=" + "{}"
self.a = requests.get(url
.format(urllib.parse.quote_plus(f"{self.img_path}"))).json()
else:
url = f"https://api.trace.moe/search?anilistID={anilist_info}&url="+"{}"
self.a = requests.get(url
.format(urllib.parse.quote_plus(f"{self.img_path}"))).json()You are doing the exact same thing in both cases, so you can remove the if statement and just keep the else block. Therefore, the rbb argument does not do anything at all.
-
Use of Lambda Function: Using a lambda function in the json_button click event is not recommended. It's better to define a separate method for this purpose.
-
Inconsistent URL Formatting: In both search_img and search_url, you are formatting the URL with different methods. Stick to one consistent way for better maintainability. My suggestion is to use f-strings for more readability.
-
Error Handling: The code doesn't include error handling for potential network issues, request failures, or invalid input. You should add appropriate error handling to make the application more robust.
-
Lack of Comments and Documentation: The code lacks comments and documentation. Adding comments to explain the purpose of functions, variables, and complex logic will make the code more understandable for others and your future self.
-
Responsibility of the Functions: As mentioned in the Single Responsibility Principle section, functions inside the search.py file should only be meant to interact with the API and display the results. They should not handle UI elements directly, as it is not a good practice. Consider refactoring to separate concerns better.
-
Possible Resource Leaks: Ensure that resources like opened files and network connections are properly closed and managed. The current code doesn't close the file opened with open(). You should always open files using the with statement to ensure that they are closed automatically.
json_operations.py
-
Import Statements: You import os.path and requests, but it's not used in the code. Remove unnecessary imports to keep the code clean.
-
Inconsistent Naming: The variable iad could have a more descriptive name for better readability. Using abbreviations can make the code harder to understand for others and your future self. Consider renaming the variable to something like include_all_details.
-
Magic Numbers: The code multiplies similarity by 100, which is somewhat of a magic number. Consider using a named constant or variable to make the intent clear.
-
Logic not simple enough: The following lines
if iad is False:
formatted_data += (
f"Filename: {filename}\n"
f"Episode: {episode}\n"
f"From: {from_time}\n"
f"To: {to_time}\n"
f"Similarity: {similarity:.2f}%\n\n"
)
else:
formatted_data += (
f"Filename: {filename}\n"
f"Episode: {episode}\n"
f"From: {from_time}\n"
f"To: {to_time}\n"
f"Similarity: {similarity:.2f}%\n\n"
f"From: {from_time}\n"
f"To: {to_time}\n"
f"Anilist: {anilist}\n"
f"Video URL: {video_url}\n"
f"Image URL: {image_url}\n"
)can be simplified by moving the common code outside the if-else block as this:
formatted_data += (
f"Filename: {filename}\n"
f"Episode: {episode}\n"
f"From: {from_time}\n"
f"To: {to_time}\n"
f"Similarity: {similarity:.2f}%\n\n"
)
if iad:
formatted_data += (
f"Anilist: {anilist}\n"
f"Video URL: {video_url}\n"
f"Image URL: {image_url}\n"
)This is shorter and easier to understand and maintain.
-
String Formatting: In the formatted_data strings, you use both f-strings and manual concatenation. Consistently use f-strings for better code readability.
-
Lack of Comments and Documentation: The code lacks comments and documentation. Adding comments to explain the purpose of functions, variables, and complex logic will make the code more understandable for others and your future self.
-
Error Handling: The write_to_json function handles file I/O and should include proper error handling. Make sure to handle exceptions that may occur during file operations.
-
Inconsistent Error Handling: In write_to_json, you use try and catch exceptions. It would be more appropriate to handle exceptions related to file operations specifically (e.g., FileNotFoundError, PermissionError, etc.).
-
Resource Management: In the write_to_json function, the file should be properly closed after writing. You can use a context manager to ensure the file is closed automatically.
-
Responsibility of the Functions: The functions uses self.a which is a property from the app class. As with search.py, they should not access UI elements directly. Consider refactoring to separate concerns better. You can pass the self.a variable as a parameter to the function so they receive the result dictionary from the API call and not the whole App class.
main.py
There are a lot of things to mention in this file but I want to focus on 2 main points:
-
The handling of different views is not done properly. You are using a QStackedWidget to handle the different views but you are not using it correctly. You are not adding the widgets to the QStackedWidget and you are not using the setCurrentWidget() method to change the current view.
What you are doing is creating a group of buttons, layouts and widgets and setting all of them inside the central_widget inside the init. When the search function is called on onClickSearch you clear all the layout (which is non-sense) and you create a new layout with the results.
That is not the correct way to handle the views. You should create a different widget for each view and add them to the QStackedWidget and then use the setCurrentWidget() method to change the current view. That way you don't need to create the widgets and remove the existing ones every time you want to change the view.
-
The onClickSearch method has a lot of bugs and unnecessary code:
if self.checkbox_iad.isChecked():
iad_status = True
else:
iad_status = False
if self.checkbox_rbb.isChecked():
rbb_status = True
else:
rbb_status = False
if anilist_id == "":
anilist_status = None
else:
anilist_status = anilist_id
...
if iad_status is True:
json_operations.json_to_tabular(self, iad=True)
else:
json_operations.json_to_tabular(self, iad=False)This handling of the values is absolutely redundant and unnecessary. You are checking if the checkboxes are checked and if they are you are assigning the value True to the variables iad_status and rbb_status and if they are not you are assigning the value False. That is not necessary at all, you can just assign the value of the isChecked() method to the variables directly:
iad_status = self.checkbox_iad.isChecked()
rbb_status = self.checkbox_rbb.isChecked()
anilist_status = anilist_id if anilist_id else None
...
json_operations.json_to_tabular(self, iad=iad_status)
And on the other hand, the app does not support searching images by URL at all, as you can see in main.py:153 when the search_url() function is called it is not passing any argument to the function, so the argument anilist_info will have the default value None and it will never search by URL. It should be:
if self.img_url == "":
search.search_img(self, anilist_info=anilist_status, rbb=rbb_status)
else:
search.search_url(self, anilist_info=anilist_status, rbb=rbb_status)If you take your time to simplify the design of those functions you can have only one generic function that handles both cases and you don't need to have 2 different functions for that.
search.search_anime_image(self, anilist_info=anilist_status, rbb=rbb_status)
In this module there are again problems like magic numbers, lack of comments, documentation and error-handling, inconsistent naming, etc. but I think the 2 points mentioned above are the most important ones.
Final Thoughts
As I mentioned at the beginning, I think the project is a good idea and it can be a good exercise for beginners to learn how to handle API calls and GUI interfaces but it needs a lot of work to be done. I hope my feedback helps you to improve the project and your skills.
Note: I created a fork of the project with all the changes mentioned in this review, some other improvements and changes that I think are suitable for the project. You can check it out here.
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent