Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Develop #5

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Develop #5

wants to merge 10 commits into from

Conversation

BrainRTP
Copy link
Owner

@BrainRTP BrainRTP commented Dec 1, 2024

No description provided.

@@ -10,18 +10,22 @@
"dev": "tsc && node dist/index.js"
},
"dependencies": {
"@types/node": "^18.16.1",
"@types/node": "^20.16.5",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Типизация явно должна быть в дев-зависимостях, скорее всего (вроде может быть не в дев если есть линтинг перед сборкой на стороне сервера в CI/CD)

this.bot.on('callback_query', async (ctx: CallbackQuery) => {
await this.callbackHandler.handleMessage(ctx);
});

this.logger.info(`Загружено ${handlerCount} обработчика событий`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прикрути plural для склонения чисел или переформулируй сообщение в "Загружено обработчиков событий: кол-во"

let dbUser = process.env.DB_USER;
let dbPassword = process.env.DB_PASSWORD;

if (this.config?.database.type == DataBaseType.POSTGRESQL && dbUser && dbPassword) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Используй строгое сравнение (===) везде, где это возможно

@@ -18,11 +18,11 @@ export abstract class DataBase implements IDatabase {

abstract queryInsert(sql: string, params?: any[]): Promise<void>;

abstract getAllVoices(botId: number | undefined): Promise<CustomVoice[]>;
abstract getAllVoices(botId: number | undefined, isHidden: boolean, limit: number, offset: number): Promise<CustomVoice[]>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

С таким числом аргументов лучше перейти на объект в качестве аргумента, если это возможно.

interface GetAllVoicesParams {
botId?: number;
isHidden: boolean;
...
}

abstract getAllVoices({ botId, isHidden, ...}: GetAllVoicesParams): ...


abstract getVoiceById(id: number): Promise<CustomVoice>;

abstract getVoiceByTitleInclude(title: string, botId: number | undefined): Promise<CustomVoice[]>;
abstract getVoiceByTitleInclude(title: string, botId: number | undefined, isHidden: boolean, limit: number, offset: number): Promise<CustomVoice[]>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аналогично комментарию выше, тем более что аргументы схожи. Общие параметры из объекта можно вынести в переиспользуемый интерфейс и указывать его в качестве аргумента, расширяя нужными свойствами. Ну или сделать отдельные интерфейсы для аргументов функций заранее, их расширяя от общего интерфейса

Comment on lines 36 to 37
if (query !== '') {
this.logger.debug(`${ctx.from.username} -> ${query}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здесь можно свапнуть блоки инструкций местами и избавиться от отрицания в проверяемом условии. А ещё лучше избавиться от блока else вовсе, кажется, можно обойтись одним if:

`ts
if (query === '') {
...
return;
}

здесь обработка случая непустой строки
`

Comment on lines 41 to -35

if (!this.config.getConfig()?.adminList.includes(msg.from.id)) {
this.bot.sendMessage(chatId, 'Загрузка аудио доступна только админам')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вынести в переменную (а то и две) перед проверкой и дать человекочитаемые имена. Длина этой портянки меня пугает

Comment on lines +192 to +194
const fileName: string = isAudio
? msg.audio?.file_name.split('.')[0].replace(/ /g, '_')
: (customSoundName?.replace(/ /g, '_') ?? randomUUID());
Copy link

@Arhonist Arhonist Dec 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Комментарий бы, что происходит. А действия в тернарке лучше вынести в отдельные файлы с переиспользуемыми утилитами, там дать им понятные названия и покрыть тестами. Особенно если они используются где-то ещё

Comment on lines +14 to +18
getAllVoices(botId: number | undefined, isHidden: boolean, limit: number, offset: number): Promise<CustomVoice[]>;

getVoiceById(id: number): Promise<CustomVoice>;

getVoiceByTitleInclude(title: string, botId: number | undefined): Promise<CustomVoice[]>;
getVoiceByTitleInclude(title: string, botId: number | undefined, isHidden: boolean, limit: number, offset: number): Promise<CustomVoice[]>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аналогично комментариям про один объект в качестве аргумента. Приняли объект, вернули объект или "принцип RoRo"

Comment on lines 3 to +5
export interface CustomVoice extends Pick<InlineQueryResultVoice, 'id' | 'title' | 'voice_url'> {
botId: number;
isHidden: boolean | number;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Интересная типизация у явно булевого типа данных (исходя из названия), вы точно делаете всё правильно?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants