araten10 / EdgewarePlusPlus

Expansion to PetitTournesol's fetishware "Edgeware", adding more features and config options.
MIT License
57 stars 19 forks source link

issues with popup.pyw #68

Closed s-b-repo closed 2 months ago

s-b-repo commented 3 months ago

Unused Imports:

Several imported modules are not used in the code. These include hashlib, logging, subprocess, time, webbrowser, and pathlib. Unused imports can make the code less readable and slightly increase the memory footprint.

Potential Redundant Import:

The pathlib module is imported twice, once as import pathlib and once as from pathlib import Path. The first import is not used, making it redundant.

Mutable Default Arguments:

In the PrefixData class, the captions and images arguments default to None, which is good practice. However, if mutable types like lists or dictionaries were used as default values, it could lead to unexpected behavior.

Exception Handling:

The try...except blocks catch all exceptions with a generic Exception class. This is generally not recommended because it can hide unexpected errors. It’s better to catch specific exceptions.

Unnecessary Global Variable:

The HASHED_PATH variable is set to None globally and later reassigned within an if block. If it is only used in the context of that block, it would be better to define it locally within that block.

Possible Unchecked File Operations:

When opening and reading files, it's good practice to ensure the file exists before attempting to open it. While the code checks the existence of some files, other file operations, such as reading from Resource.WEB, are done without checking for the file's existence first.

Code Duplication:

There is a fair amount of duplicated logic, particularly in reading and processing JSON files. This could be refactored into helper functions to reduce repetition and improve maintainability.

Concurrency Issues:

The thread module is used for threading, but there is no thread synchronization mechanism like threading.Lock. Depending on how threads interact with shared resources (like the GUI), this could lead to race conditions.

Inefficient Random Selection in pick_resource:

The pick_resource function could potentially enter an infinite loop if it keeps selecting an invalid item. It may be more efficient to filter out invalid items before entering the loop.

Magic Numbers:

Several parts of the code use magic numbers (like 75, 100, 20, etc.) which make the code less readable. These should be replaced with named constants.

Potential Infinite Loops:

In functions like move_window, while True loops are used without any clear exit conditions, which could potentially lead to infinite loops if not handled carefully.

Redundant List to List Conversion:

In the GifLabel class, the list of frames is initially an empty list, and later converted to a cycle object. This conversion seems unnecessary and could be done in one step.
LewdDevelopment commented 2 months ago

The main code of Edgeware is currently being rewritten and many of these points have already been addressed. A few suggestions are still relevant though, and I'll be sure to implement fixes in the new version, thanks for your thoroughness!

s-b-repo commented 2 months ago

The main code of Edgeware is currently being rewritten and many of these points have already been addressed. A few suggestions are still relevant though, and I'll be sure to implement fixes in the new version, thanks for your thoroughness!

no problem brother