diff --git a/src/controller/controller.py b/src/controller/controller.py index b9ef996..87d46fd 100644 --- a/src/controller/controller.py +++ b/src/controller/controller.py @@ -20,8 +20,11 @@ class Controller(BaseController): self.signal_widgets.emit(widgets) def update_settings(self, settings: list[dict]) -> None: + # TODO: Почему контроллеру просто не сообщить о медиаторе, и ему сообщать что-то через notify? Тем более, что + # медиатор уже знает про контроллер... self.signal_settings.emit(settings) + # TODO: Объедини переключение режимов в один метод, и по входному аргументу (например 1 и 2) переключай их. def raport_mode(self) -> None: self.signal_raport_mode.emit() @@ -29,12 +32,18 @@ class Controller(BaseController): self.signal_seeking_mode.emit() def open_file(self, filepath: str) -> None: + # TODO: Где тут открытие файла? self.signal_open_file.emit(filepath) def update_plots(self) -> None: self.signal_update_plots.emit() def update_status(self, msg: Union[str, float, int]) -> None: + # TODO: Довольно странный набор возможных типов входных аргументов. + # TODO: Для чего сюда принимается float, если он все равно приводится к int? + # TODO: Из медиатора сюда приходит статус 0.5, который становится нулем. + # Это точно ожидаемое и корректное поведение? Сомнительно... + # TODO: if isinstance(msg, float): ... if type(msg) == float or type(msg) == int: self.signal_statusBar.emit(int(msg)) else: diff --git a/src/controller/converter.py b/src/controller/converter.py index 5136841..fc16ed0 100644 --- a/src/controller/converter.py +++ b/src/controller/converter.py @@ -18,6 +18,8 @@ class DataConverter(BaseDataConverter): dataframe[bool_columns] = dataframe[bool_columns].replace({True: 1, False: 0}) return dataframe except: + # TODO: Обработка исключений! + # TODO: Осмысленное сообщение в лог. Без traceback и прочего подобного. return None def convert_data(self, files: list[str]) -> None: @@ -25,10 +27,12 @@ class DataConverter(BaseDataConverter): dataframes = [pd.read_csv(file) if file != '' else None for file in files] converted_dataframes = list(map(self._replace_bool, dataframes)) except: + # TODO: Обработка исключений! # Get the traceback object tb = sys.exc_info()[2] tbinfo = traceback.format_tb(tb)[0] pymsg = "Traceback info:\n" + tbinfo + "\nError Info:\n" + str(sys.exc_info()[1]) + # TODO: Осмысленное сообщение в лог. Без traceback и прочего подобного. logger.error(pymsg) converted_dataframes = [None] finally: diff --git a/src/controller/mediator.py b/src/controller/mediator.py index 51477eb..dac746a 100644 --- a/src/controller/mediator.py +++ b/src/controller/mediator.py @@ -16,6 +16,7 @@ class Mediator(BaseMediator): data: Union[list[str], list[pd.DataFrame], list[list], list[QWidget]]): if issubclass(source.__class__, BaseDirectoryMonitor): + # TODO: self._controller.update_status("CSV found! Calculating...") А вот так нельзя написать? self.update_status("CSV found! Calculating...") self._converter.convert_data(data) @@ -30,12 +31,17 @@ class Mediator(BaseMediator): if issubclass(source.__class__, BasePlotWidget): self.update_status(100) self._controller.send_widgets(data) - + def update_settings(self, settings: list[dict]): + # TODO: А дописать self.notify не? + # if issubclass(source.__class__, BaseController): + # self._monitor.update_settings(data) + # self._passportFormer.update_settings(data) self._monitor.update_settings(settings) self._passportFormer.update_settings(settings) def update_status(self, msg: Union[str, float]) -> None: + # TODO: Бессмысленный метод. self._controller.update_status(msg) diff --git a/src/controller/monitor.py b/src/controller/monitor.py index b976344..66bbfc1 100644 --- a/src/controller/monitor.py +++ b/src/controller/monitor.py @@ -7,10 +7,11 @@ from utils.base.base import BaseDirectoryMonitor class DirectoryMonitor(BaseDirectoryMonitor): def _init_state(self): - self._files = [ os.path.join(self._directory_path, file) for file in os.listdir(self._directory_path) + # TODO: FileNotFoundError: [Errno 2] No such file or directory: 'D:/downloads/a22' + # при выборе real time mode. if file.lower().endswith('.csv') ] @@ -31,6 +32,9 @@ class DirectoryMonitor(BaseDirectoryMonitor): if not current_files: self._files = [] + # TODO: Почему монитор директории обновляет какие-то настройки, графики, стартует что-то? + # Это не должно быть в его зоне ответственности. + # TODO: Может стоит сделать какой-то класс, который будет заниматься обновлениями? def update_settings(self, data: list[dict]) -> None: if self.isActive: self.stop() diff --git a/src/controller/passportFormer.py b/src/controller/passportFormer.py index e7d4c95..81bb77c 100644 --- a/src/controller/passportFormer.py +++ b/src/controller/passportFormer.py @@ -7,6 +7,7 @@ from utils.base.base import BasePointPassportFormer, BaseIdealDataBuilder class idealDataBuilder(BaseIdealDataBuilder): + # TODO: Имя класса с большой буквы. def get_closingDF(self) -> pd.DataFrame: return self._get_data(self.Ts['tclose'], self.calcPhaseClose) @@ -35,10 +36,13 @@ class idealDataBuilder(BaseIdealDataBuilder): class PassportFormer(BasePointPassportFormer): def form_passports(self, data: list[pd.DataFrame]) -> list[list[pd.DataFrame, dict, int]]: + # TODO: сигнатура метода не соответствует сигнатуре метода класса родителя try: return_data = [self._build_passports_pocket(dataframe) for dataframe in data] except: + # TODO: обработка исключений!!! tb = sys.exc_info()[2] + # TODO: Нормальные сообщения в лог! tbinfo = traceback.format_tb(tb)[0] pymsg = "Traceback info:\n" + tbinfo + "\nError Info:\n" + str(sys.exc_info()[1]) logger.error(pymsg) @@ -48,7 +52,8 @@ class PassportFormer(BasePointPassportFormer): def _build_passports_pocket(self, dataframe: pd.DataFrame) -> list[pd.DataFrame, dict, int]: - + # TODO: Еще раз проверь, что написано в аннотации, и что метод возвращает. + # TODO: Практически нечитаемо. Надо оптимизировать и декомпозировать. if dataframe is not None: events, point_quantity = self._filter_events(dataframe["time"], dataframe) if point_quantity == 0: @@ -74,16 +79,21 @@ class PassportFormer(BasePointPassportFormer): } params_list = [operator_settings, system_settings] cache_key = self._generate_cache_key(params_list) + # TODO: if-else заменить на: + # ideal_data = self._ideal_data_cashe.get(cache_key, self._build_ideal_data(idealDataBuilder=idealDataBuilder, params=params_list)) + # self._ideal_data_cashe[cache_key] = ideal_data if cache_key in self._ideal_data_cashe : ideal_data = self._ideal_data_cashe[cache_key] else: ideal_data = self._build_ideal_data(idealDataBuilder=idealDataBuilder, params=params_list) self._ideal_data_cashe[cache_key] = ideal_data + # TODO: point_timeframe, point_events = None, None if events is not None: idx = i+1 if idx_shift else i point_timeframe = [events[self._stages[0]][0][i], events[self._stages[-1]][1][idx]] point_events = {key: [value[0][i], value[1][i]] for key, value in events.items()} else: + # TODO: убрать else point_timeframe, point_events = None, None useful_p_data = {"thickness": operator_settings["object_thickness"], "L2": operator_settings["distance_l_2"], diff --git a/src/gui/mainGui.py b/src/gui/mainGui.py index 48b294e..d21f429 100644 --- a/src/gui/mainGui.py +++ b/src/gui/mainGui.py @@ -14,8 +14,18 @@ from gui.reportGui import ReportSettings class MainWindow(BaseMainWindow): def __init__(self, controller: Optional[BaseController] = None) -> None: + # TODO: Почему контроллер Optional? Нигде нет обработки случаев, если он будет None: при вызове его методов + # будет ошибка. + + # TODO: Касательно передачи экземпляра контроллера в конструктор. Не лучше ли реализовать взаимодействие с ним + # через сигналы? Как минимум это позволит исключить странную зависимость интерфейса от контроллера. + # Логичнее было бы использовать данный класс в качестве фасада над виджетами, о чем сказано ниже. super().__init__() self._controller = controller + # TODO: касательно ВСЕХ методов данного класса, СОЗДАЮЩИХ ВИДЖЕТЫ. Исходя из названия данного класса - это + # финальное главное окно приложения: оно не должно создавать виджеты, а уже использовать готовые, поэтому + # правильнее будет каждый создаваемый тут виджет вынести в отдельный модуль в отдельный класс, там выполнять + # его создание и настройку, а тут уже получать готовый экземпляр виджета и его отображать. self._init_startUI() self._init_dock_widgets() self._init_menu() @@ -194,6 +204,11 @@ class MainWindow(BaseMainWindow): if child.widget() is not None: child.widget().deleteLater() + # TODO: По методам self._init_seekingUI и self._init_raportUI: + # 1. Дублирование кода (блок из первых трех строк) - вынести в отдельный метод + # 2. Оба метода призваны проинициализировать UI, но в то же время занимаются созданием виджетов и еще что-то + # в контроллере дергают: создание виджетов вынести отдельно, контроллеру сообщить о чем-то при помощи + # pyqtSignal при нажатии кнопки или выборе пункта меню. def _init_seekingUI(self) -> None: self._clear() self._init_TabWidget() @@ -292,6 +307,7 @@ class MainWindow(BaseMainWindow): def _close_tab(self, index:int) -> None: self.tabWidget.removeTab(index) + # TODO: Зачем разделять эти 2 метода? В self._select_csv путь к файлу получили и контроллеру сигналом отправили. def _open_file(self) -> None: path = self._select_csv() if path is not None: @@ -307,7 +323,8 @@ class MainWindow(BaseMainWindow): def _on_tab_changed(self, index): try: tab = self.tabWidget.currentWidget() - except: + except: + # TODO: Очень плохая практика - не указывать конкретный тип исключений. tab = None if tab is not None and self.report_dock.isVisible(): reg_items = tab.property("reg_items") @@ -316,6 +333,9 @@ class MainWindow(BaseMainWindow): self.repSettings.build(index, reg_items, curve_items, qt_items) def _save_plots(self) -> None: + # TODO: Нарушение принципа единичной ответственности. Почему ИНТЕРФЕЙС занимается сохранением чего-то? + # От него должен только исходить призыв к действию (кнопка или что-то подобное), сама логика же должна + # быть в другом месте. filepath, _ = QtWidgets.QFileDialog.getSaveFileName(self, "Save file", "", "Image Files (*.png *.jpeg)") tab = self.tabWidget.currentWidget() if tab is not None: diff --git a/src/gui/plotter.py b/src/gui/plotter.py index 1085218..38b4ad5 100644 --- a/src/gui/plotter.py +++ b/src/gui/plotter.py @@ -13,8 +13,12 @@ import pandas as pd from typing import Optional, Any from utils.base.base import BasePlotWidget +# TODO: Навести порядок в импортах. class ProcessStage(): + # TODO: Для чего тут наследование от ничего? + # TODO: Если я правильно понял сценарий использования этого класса, то правильно это сделать либо отнаследовавшись + # от NamedTuple, либо использовать @dataclass. mean_value:int start_index:int finish_index:int @@ -157,6 +161,7 @@ class PlotWidget(BasePlotWidget): """ Добавляет QLabel с информацией о производительности. """ + # TODO: Почему PlotWidget создает Label? Вынести в другое место. tesla_TWC = round((1 - TWC_time/tesla_time)*100, 2) if tesla_time else 0.0 tesla_ideal = round((1 - ideal_time/tesla_time)*100, 2) if tesla_time else 0.0 TWC_ideal = round((ideal_time/TWC_time)*100, 2) if TWC_time else 0.0 @@ -187,6 +192,12 @@ class PlotWidget(BasePlotWidget): def _build_widget(self, data: list[Any]) -> QWidget: + # TODO: Либо передавать в метод 3 аргумента с аннотацией типа каждого, либо передавать + # NamedTuple / dataclass. + # TODO: Исходя из сигнатуры метода, он создает и возвращает виджет с графиками. + # Простыня "result_widget, reg_items, curve_items, qt_items" не похожа на виджет с графиком. + # TODO: Данный метод должен содержать только ту логику, которая относится непосредственно к графикам. + # Все остальное - вынести. Оставшийся метод декомпозировать - очень трудно вообще понять, что тут происходит. """ Собирает графический виджет для одного набора данных. Параметр `data` предполагается списком: [dataframe, points_pocket, useful_data]. @@ -353,19 +364,26 @@ class PlotWidget(BasePlotWidget): ... ] """ + # TODO: Про входные аргументы см. выше. try: + # TODO: Про инициализацию атрибутов класса где-то уже писал. На всякий случай: она делается в конструкторе. self._datalen = len(data) widgets_datapack = [self._build_widget(data_sample) for self._datastep, data_sample in enumerate(data)] except: + # TODO: Добавить конкретные исключения. tb = sys.exc_info()[2] tbinfo = traceback.format_tb(tb)[0] pymsg = "Traceback info:\n" + tbinfo + "\nError Info:\n" + str(sys.exc_info()[1]) + # TODO: Логи должны быть понятны обычному пользователю. Traceback что-то там - это не информативно и может + # быть непонятно, поэтому пишем осознанные сообщения. Если надо, то создаем собственные + # классы исключений. (Касается всего проекта - уже не первый раз натыкаюсь на это.) logger.error(pymsg) widgets_datapack = [QLabel(pymsg)] finally: self._mediator.notify(self, widgets_datapack) def _update_status(self, widgsteps:int, pointsteps:int, cur_widg:int, cur_point:int): + # TODO: if self._datalen: ... if self._datalen != 0: sycle_start = self._datastep/self._datalen*100 + 1 period1 = 100/self._datalen @@ -377,6 +395,7 @@ class PlotWidget(BasePlotWidget): period3 = period2/pointsteps if pointsteps != 0 else period2 progress = sycle_start + period2*cur_widg + period3*cur_point + # TODO: см. модуль mediator.py self._mediator.update_status(progress) diff --git a/src/gui/settings_window.py b/src/gui/settings_window.py index f068e91..6437db1 100644 --- a/src/gui/settings_window.py +++ b/src/gui/settings_window.py @@ -103,7 +103,7 @@ class settingsWindow(QWidget): for i, (_, items) in enumerate(self._data.items()): for j, item in enumerate(items): self._param_table.setItem(i, j, QTableWidgetItem(str(item))) - + # TODO: if isinstance()... if type(item) == int: self._param_table.setItemDelegateForRow(i, int_delegate) elif type(item) == float: diff --git a/src/main.py b/src/main.py index 9172ba4..b79db8e 100644 --- a/src/main.py +++ b/src/main.py @@ -1,6 +1,8 @@ import sys import pyqtgraph as pg from PyQt5 import QtWidgets +# TODO: Правило хорошего тона при оформлении импортов: сначала встроенные библиотеки, +# через одну пустую строку - дополнительно установленные, и еще через одну пустую строку - собственные импорты. from gui.mainGui import MainWindow from controller.monitor import DirectoryMonitor @@ -10,6 +12,8 @@ from gui.plotter import PlotWidget from controller.controller import Controller from controller.passportFormer import PassportFormer +# TODO: Актуализировать requirements.txt!!! Делать это регулярно после добавления новых библиотек! +# TODO: Именование модулей: lowercase / snake_case. def main(): pg.setConfigOptions(useOpenGL=False, antialias=False) diff --git a/src/performance/roboter.py b/src/performance/roboter.py index 7119c40..f82fbd0 100644 --- a/src/performance/roboter.py +++ b/src/performance/roboter.py @@ -4,15 +4,21 @@ from typing import Optional import os import numpy as np import pandas as pd - +# TODO: Оформление импортов class BasePerformanceFactory(ABC): + # TODO: Абстракция делается с целью определения интерфейса и только: внутри никаких реализаций быть не должно. + # Сначала делаешь абстракцию, затем от нее наследуешься и делаешь нечто базовое, затем конкретную реализацию. + # Есть вариант не делать базовое нечто, а сразу переходить к реализации. + + # TODO: Убрать из этого класса все реализации и имплементировать соответствующие методы уже в наследниках. + # TODO: Возможно стоит отдельно вынести все касающееся парсинга в отдельный класс (на подумать). @abstractmethod def factory_method(self): ... - + # TODO: @abstractmethod def job(self): ... @@ -30,17 +36,25 @@ class BasePerformanceFactory(ABC): inside_channel = False channels = 0 for line in file: - line = line.strip() + line = line.strip() + # TODO: if line in ("#BEGINCHANNELHEADER", "#BEGINGLOBALHEADER")... if line == '#BEGINCHANNELHEADER' or line == "#BEGINGLOBALHEADER": inside_channel = True - + # TODO: if line in ("#ENDCHANNELHEADER", "#ENDGLOBALHEADER")... elif line == '#ENDCHANNELHEADER' or line == "#ENDGLOBALHEADER": inside_channel = False + # TODO: для чего тут pass? pass + # TODO: inside_channel = line in ("#BEGINCHANNELHEADER", "#BEGINGLOBALHEADER") # Формирование словаря + # TODO: Предыдущие условия связаны были с line, а теперь под elif уже абсолютно другая логика. + # Не надо так делать. elif inside_channel: + # TODO: нижним подчеркиванием обозначается переменная, которая как бы есть, но не нужна. + # Ты же на что-то проверяешь, т.е. используешь. Назови нормально. _, data = line.split(',') match _: + # TODO: тут default case не нужен? case '102': head['rob_id'] = data case '200': @@ -58,9 +72,11 @@ class BasePerformanceFactory(ABC): case '241': head[ch_name]['multiplyer'] = float(data) head['channels'] = int(channels) + # TODO: Метод декомпозировать и переписать. return head, file def _r64_parser(self, path: str, head: dict) -> Optional[list[pd.Series, pd.DataFrame]]: + # TODO: Метод декомпозировать и переписать. ch = head['channels'] keys = list(head.keys())[-ch:] len_timestamps = head['Zeit']['len'] @@ -80,6 +96,8 @@ class BasePerformanceFactory(ABC): class BaseProduct(ABC): + # TODO: Аналогично BasePerformanceFactory: снова в абстракции куча реализаций. + # TODO: Может стоит дать классу название осмысленное, а не из статьи по паттернам? =) def __init__(self): self._signals = [] @@ -117,10 +135,16 @@ class BaseProduct(ABC): @abstractmethod def operation(self): + # TODO: Аналогично: что за операция? Что она делает? Не понятно. ... class Performance(BasePerformanceFactory): + # TODO: где реализация factory_method? Если не нужен, то надо его убрать из интерфейса. + # TODO: данный класс, простите, лебедь раком щуку. На все руки мастер. И то создает, и это создает... И принцип + # единичной ответственности нарушает, и принцип подстановки... + # Если реализуешь паттерн "Фабричный метод", то реализуй его правильно: для каждого порождаемого объекта свой + # класс - создатель, классы наследники в полной мере имплементируют методы класса - родителя... def robot_method(self) -> BaseProduct: return RobotData() @@ -132,10 +156,12 @@ class Performance(BasePerformanceFactory): return CommData() def job(self, path:str, TWC_raw:pd.DataFrame) -> list[pd.DataFrame, list[pd.DataFrame], list[pd.DataFrame]]: + # TODO: сигнатура метода не соответствует той, которая определена в родительском классе. robot = self.robot_method() TWC = self.TWC_method() comm=self.comm_method() dataframe = self._get_file_data(path) + # TODO: operation в родителе определен без входных параметров, используется с ними. Не надо так делать. rob_comm, rob_df = robot.operation(dataframe) TWC_comm, TWC_df = TWC.operation(TWC_raw) comm_df = comm.operation(rob_comm, TWC_comm) @@ -156,6 +182,8 @@ class RobotData(BaseProduct): communication_sig = {'sent':events["$OUT3244"]["fall"], 'received':events[""][""]} point_interval = self._form_intervals(start=events["$OUT3012"]["rise"], end=events["$OUT3012"]["fall"]) movement_interval = self._form_intervals(start=events["$OUT3244"]["rise"], end=events["$OUT3244"]["fall"]) + # TODO: возвращаемый результат не соответствует аннотированному. + # TODO: зачем возвращать DataFrame? В этом есть какой-то философский смысл? return communication_sig, pd.DataFrame({'in_point':point_interval, 'in_move':movement_interval}) @@ -177,6 +205,8 @@ class TWC_Data(BaseProduct): squeeze_interval = self._form_intervals(start=events["Squeeze"]["rise"], end=events["Squeeze"]["fall"]) relief_interval = self._form_intervals(start=events["Relief"]["rise"], end=events["Relief"]["fall"]) oncoming_interval = self._form_intervals(start=events["Oncoming"]["rise"], end=events["Oncoming"]["fall"]) + # TODO: возвращаемый результат не соответствует аннотированному. + # TODO: зачем возвращать DataFrame? В этом есть какой-то философский смысл? return communication_sig, pd.DataFrame({'in_closing':closing_interval, 'in_squeeze':squeeze_interval, 'in_relief':relief_interval, diff --git a/src/testAlgo.py b/src/testAlgo.py index f0b05a7..4444861 100644 --- a/src/testAlgo.py +++ b/src/testAlgo.py @@ -1,29 +1,52 @@ +# TODO: Тесты должны лежать в директории tests. + +# TODO: Правило хорошего тона при оформлении импортов: сначала встроенные библиотеки, +# через одну пустую строку - дополнительно установленные, и еще через одну пустую строку - собственные импорты. from src.OptAlgorithm.OptAlgorithm import OptAlgorithm from src.utils import read_json from matplotlib import pyplot as plt, use from numpy import cos, sin, sqrt, cbrt, arcsin, linspace, array - +# TODO: Отступ перед блоком if __name__ == "__main__" - 2 пустых строки. if __name__ == "__main__": - + # TODO: Что означают переменные ниже? Имена переменных должны быть осмысленными, чтобы их смысл был понятен не + # не только автору кода. tq = 1 ts = linspace(0, tq, 200000) - operator_params = read_json("params/operator_params.json") - system_params = read_json("params/system_params.json") - + operator_params = read_json("../params/operator_params.json") + system_params = read_json("../params/system_params.json") + + # TODO: 2 блока кода ниже (с циклами) нарушают принцип DRY (Don't Repeat Yourself): + # повторяющийся блок кода следует вынести в отдельную функцию. + # Например, в такую: + # def foo(params_dict: dict) -> dict: + # result_dict = {} + # i = 1 + # for key, value in params_dict.items(): + # if isinstance(value, list): + # result_dict[key] = value[i] if len(value) > i else value[0] + # else: + # result_dict[key] = value + # return result_dict + # И потом вызвать ее как-то так: + # non_array_operator_params, non_array_system_params = foo(operator_params), foo(system_params) + non_array_operator_params = {} i = 1 for key, value in operator_params.items(): + # TODO: Не совсем понятен смысл такой проверки. Если хотим проверить принадлежность value к list, + # то лучше использовать для этого isinstance. if hasattr(value, "__len__"): + # TODO: Можно заменить однострочным выражением: result_dict[key] = value[i] if len(value) > i else value[0] if len(value) > i: non_array_operator_params[key] = value[i] else: non_array_operator_params[key] = value[0] else: non_array_operator_params[key] = value - + non_array_system_params = {} for key, value in system_params.items(): if hasattr(value, "__len__"): @@ -33,7 +56,7 @@ if __name__ == "__main__": non_array_system_params[key] = value[0] else: non_array_system_params[key] = value - + opt = OptAlgorithm(non_array_operator_params, non_array_system_params) Xs = array([opt.getVar("X1", t) for t in ts]) diff --git a/src/uml/creator.py b/src/uml/creator.py index b176e62..51822da 100644 --- a/src/uml/creator.py +++ b/src/uml/creator.py @@ -11,7 +11,7 @@ class UMLCreator: real_data = [] ideal_data = [] if not self.theor_mode: - + # TODO: Писать так условия - моветон. Как вариант, можно применить конструкцию match - case. for key, items in self.timings_dict.items(): if key == 'closure': ideal_time = self._ideal_time[0] elif key == 'compression': ideal_time = self._ideal_time[1] @@ -98,7 +98,7 @@ class UMLCreator: timings_dict: dict, mode: bool, name:str): - + # TODO: Это надо в конструкторе класса делать! self._ideal_time = ideal_time self.bool_dict = bool_dict self.float_dict = float_dict diff --git a/src/uml/request_generator.py b/src/uml/request_generator.py index 8d2351c..3bc25b3 100644 --- a/src/uml/request_generator.py +++ b/src/uml/request_generator.py @@ -1,8 +1,9 @@ from plantuml import PlantUML from os.path import abspath - +# TODO: Навести порядок в импортах. class Request: + # TODO: Имя класса не соответствует тому, что он делает. def __init__(self, server_url: str): self._server_url = server_url @@ -13,12 +14,15 @@ class Request: self.clear() def _startUML(self): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.reqArr.append('@startuml') def _endUML(self): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.reqArr.append('@enduml') def clear(self): + # TODO: Атрибуты класса инициализируются в конструкторе класса. self.timestamps = {} self.reqArr = [] self.variables = [] @@ -27,28 +31,36 @@ class Request: self._startUML() def addAnalog(self, name, string): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.variables.append(f'analog "{string}" as {name}') def addBinary(self, name, string, style = ''): + # TODO: Единый стиль именования методов! (snake_case / lowercase). if style: name = name + '<<' + style + '>>' self.variables.append(f'binary "{string}" as {name}') def addClock(self, name, string): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.variables.append(f'clock "{string}" as {name}') def addConcise(self, name, string): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.variables.append(f'concise "{string}" as {name}') def addRobust(self, name, string): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.variables.append(f'robust "{string}" as {name}') def appendStr(self, string = ''): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.variables.append(string) def addLineStyle(self, name, color = 'green', thicknes = 1): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.lineStyles[name] = [f'LineColor {color}', f'LineThickness {thicknes}'] def generateSVG(self, name): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self._compileUML(name) filename = abspath(f'{name}.txt') self.server.processes_file(filename, outfile=f'{name}.svg') @@ -56,6 +68,7 @@ class Request: #return result def setTimestamps(self, name, input): + # TODO: Единый стиль именования методов! (snake_case / lowercase). for time, state in input: try: self.timestamps[f'@{time}'].append(f'{name} is {state}') @@ -63,11 +76,13 @@ class Request: self.timestamps[f'@{time}'] = [f'{name} is {state}'] def _addTimestamp(self, timecode, vars): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.reqArr.append(timecode) for var in vars: self.reqArr.append(var) def _addHeader(self): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self.reqArr.append('') def _addVariables(self): + # TODO: Единый стиль именования методов! (snake_case / lowercase). for var in self.variables: self.reqArr.append(str(var)) def _compileUML(self, name): + # TODO: Единый стиль именования методов! (snake_case / lowercase). self._addHeader() self._addVariables() diff --git a/src/utils/base/base.py b/src/utils/base/base.py index d9bc10d..72a834b 100644 --- a/src/utils/base/base.py +++ b/src/utils/base/base.py @@ -1,5 +1,9 @@ -from __future__ import annotations +# TODO: Почему каталог с базовыми классами лежит внутри каталога с утилитами? Это странно. + +from __future__ import annotations +# TODO: Правило хорошего тона при оформлении импортов: сначала встроенные библиотеки, +# через одну пустую строку - дополнительно установленные, и еще через одну пустую строку - собственные импорты. import os from typing import Optional, Union, Any from cachetools import LRUCache @@ -13,12 +17,15 @@ from OptAlgorithm import OptAlgorithm from utils.qt_settings import dark_style +# TODO: Глобальное замечание: Базовые классы создаются для обозначения неких базовых свойств объектов. +# Конкретная же реализация возлагается на классы - наследники. class BaseMediator: def __init__(self, monitor: BaseDirectoryMonitor, converter: BaseDataConverter, + # TODO: passport_former passportFormer: BasePointPassportFormer, plot: BasePlotWidget, controller: BaseController): @@ -37,12 +44,13 @@ class BaseMediator: source: Union[BaseDirectoryMonitor, BaseDataConverter, BasePointPassportFormer, BasePlotWidget], data: Union[list[str], list[pd.DataFrame], list[list], list[QWidget]]): ... + # TODO: Отступы между методами класса - одна пустая строка. def update_settings (self, data: list[dict]): ... def update_status(self, msg: Union[str, float]) -> None: ... - + # TODO: Отступы между классами - две пустых строки. class BaseDirectoryMonitor: update_timer = QTimer() @@ -88,7 +96,8 @@ class BaseDirectoryMonitor: def stop(self): self.isActive = False self.update_timer.stop() - + # TODO: Почему монитор директорий обновляет какие-то настройки, графики и т.д.? Помним про + # принцип единичной ответственности. def update_settings(self, data: list[dict]) -> None: ... @@ -96,6 +105,7 @@ class BaseDirectoryMonitor: ... def force_all_dir(self): + # TODO: По сигнатуре метода вообще не понятно, что он должен делать. ... class BaseDataConverter: @@ -230,6 +240,9 @@ class BasePlotWidget: }, } def set_style(self, object: Union[QTabWidget, QWidget]) -> None: + # TODO: Данный метод статичный. Для обозначения подобных методов используется декоратор @staticmethod. + # TODO: object - зарезервированное слово. Использовать эти слова для именования переменных не правильно. + # Если хочется использовать именно такое слово, можно написать "object_". object.setStyleSheet( """QLabel { color: #ffffff; @@ -239,6 +252,8 @@ class BasePlotWidget: }""") def _downsample_data(self, x, y, max_points=5000): + # TODO: Данный метод статичный. Для обозначения подобных методов используется декоратор @staticmethod. + # TODO: Какой тип данных у 'x'? Какой тип данных у 'y'? Надо добавить аннотации типов. """ Понижает разрешение данных до заданного количества точек для улучшения производительности навигатора. """ @@ -273,12 +288,13 @@ class BasePlotWidget: # Связываем изменение региона навигатора с обновлением области просмотра основного графика ROI_region.sigRegionChanged.connect(lambda: self._sync_main_plot_with_navigator(main_plot, ROI_region)) - + # TODO: Возвращаемый результат не соответствует аннотированному в сигнатуре метода. return navigator, ROI_region def _sync_main_plot_with_navigator(self, main_plot: pg.PlotItem, region: pg.LinearRegionItem): + # TODO: Данный метод статичный. Для обозначения подобных методов используется декоратор @staticmethod. """ Синхронизирует область просмотра основного графика с регионом навигатора. """ @@ -288,11 +304,14 @@ class BasePlotWidget: main_plot.setXRange(x_min, x_max, padding=0) main_plot.blockSignals(False) + # TODO: Методы _mirror_shift_data и _shift_data нарушают принцип DRY: дублирование кода. + # Сделать ОДИН метод, одним из входных параметров которого будет lambda-функция, которая применяется к dataframe. def _mirror_shift_data(self, valid_str: str, signals: list[dict], dataframe: pd.DataFrame, shift: float) -> pd.DataFrame: + # TODO: Данный метод статичный. Для обозначения подобных методов используется декоратор @staticmethod. keys = dataframe.keys() for signal in signals: if valid_str in signal["name"] and signal["name"] in keys: @@ -304,6 +323,7 @@ class BasePlotWidget: signals: list[dict], dataframe: pd.DataFrame, shift: float) -> pd.DataFrame: + # TODO: Данный метод статичный. Для обозначения подобных методов используется декоратор @staticmethod. keys = dataframe.keys() for signal in signals: if valid_str in signal["name"] and signal["name"] in keys: @@ -311,6 +331,7 @@ class BasePlotWidget: return dataframe def _sync_navigator_with_main(self, main_plot: pg.PlotItem, region:pg.LinearRegionItem): + # TODO: Данный метод статичный. Для обозначения подобных методов используется декоратор @staticmethod. """ Синхронизирует регион навигатора с областью просмотра основного графика. """ @@ -334,6 +355,8 @@ class BasePlotWidget: @opt.setter def opt(self, opt: BaseIdealDataBuilder): + # TODO: Атрибуты класса следует сначала определять в конструкторе класса. + # TODO: Что такое opt? Optical? Option? Optimal? Optimus Prime? self._opt = opt def build(self, data: list[pd.DataFrame]) -> list[QWidget]: @@ -349,9 +372,11 @@ class BaseController(QObject): ... def raport_mode (self) -> None: + # TODO: обычно в названиях методов должен присутствовать глагол. ... def seeking_mode(self) -> None: + # TODO: обычно в названиях методов должен присутствовать глагол. ... def open_file(self, filepath: str) -> None: @@ -429,6 +454,7 @@ class BaseMainWindow(QMainWindow): @controller.setter def controller(self, controller: BaseController) -> None: + # TODO: зачем делать setter для контроллера, если ты его экземпляр передаешь в конструктор? self._controller = controller def set_style(self, object: Union[QTabWidget, QWidget, QMainWindow]) -> None: diff --git a/src/utils/diagram_parser.py b/src/utils/diagram_parser.py index aceb9ce..06c6253 100644 --- a/src/utils/diagram_parser.py +++ b/src/utils/diagram_parser.py @@ -9,27 +9,34 @@ class DiagramParser: system_config["Welding_signal"], system_config["Release_signal"], system_config["Oncomming_signal"]] - + # TODO: Переменные в Python именуются в lowercase либо snake_case. self.boolDict = {} self.floatDict = {} self.timingsDict = {} self.theor_mode = False def setData(self, path): + # TODO: Методы в Python именуются в lowercase либо snake_case. + # TODO: Данный метод устанавливает какой-то режим, читает CSV, наполняет словари некими данными. + # Декомпозировать на разные методы, каждый из которых будет решать какую-то свою задачу. if not path: self.theor_mode = True else: self.data = pd.read_csv(path) - + + # TODO: 2 цикла ниже: дублирование кода. Устранить. for signalName in self.data.columns: + # TODO: if isinstance... if type (self.data[signalName].iloc[0]) == np.bool: self.boolDict[signalName] = self._getBoolChanges(signalName) for signalName in self.data.columns: + # TODO: if isinstance... if type (self.data[signalName].iloc[0]) == np.float64: self.floatDict[signalName] = self._getFloatChanges(signalName) for key, items in self.boolDict.items(): + # TODO: Писать так условия - моветон. Как вариант, можно применить конструкцию match - case. if key == self.signals[0]: name = "closure" elif key == self.signals[1]: name = "compression" elif key == self.signals[2]: name = "welding" @@ -46,22 +53,34 @@ class DiagramParser: self.timingsDict[name].append([items[i][0], items[i][0]+0.01]) def getBoolDict (self) -> dict: + # TODO: сделать через декоратор @property: + # @property + # def bool_dict(self) -> dict: + # return self._bool_dict + # TODO: Методы в Python именуются в lowercase либо snake_case. return self.boolDict def getFloatDict (self) -> dict: + # TODO: сделать через декоратор @property: + # TODO: Методы в Python именуются в lowercase либо snake_case. return self.floatDict def getRealTimings(self) -> dict: + # TODO: сделать через декоратор @property: + # TODO: Методы в Python именуются в lowercase либо snake_case. return self.timingsDict def getMode(self) -> bool: + # TODO: сделать через декоратор @property: + # TODO: Методы в Python именуются в lowercase либо snake_case. return self.theor_mode def _getBoolChanges(self, signalName) -> list: + # TODO: Методы в Python именуются в lowercase либо snake_case. timeCode = self.data['time'] signal_values = self.data[signalName] changes = [] - + # TODO: if len(signal_values)... if len(signal_values) > 0: changes.append([float(timeCode.iloc[0]), 'high' if bool(signal_values.iloc[0]) else 'low']) @@ -73,6 +92,7 @@ class DiagramParser: def _getFloatChanges(self, signalName) -> list: + # TODO: Методы в Python именуются в lowercase либо snake_case. timeCode = self.data['time'] signal_values = self.data[signalName] changes = [] diff --git a/src/utils/qt_settings.py b/src/utils/qt_settings.py index 7bc6a0c..c012c6e 100644 --- a/src/utils/qt_settings.py +++ b/src/utils/qt_settings.py @@ -1,3 +1,5 @@ +# TODO: Было бы логично разместить данный модуль в каталоге gui. + dark_style = """ /* ------------------------------------------------------