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

Покоритель баз данных #5

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

evtimofeev87
Copy link

@evtimofeev87 evtimofeev87 commented Dec 10, 2024

package.json Outdated
"import:default": "npm run ts ./src/main.cli.ts -- --import ./mocks/mock-data.tsv",
"import:generated": "npm run ts ./src/main.cli.ts -- --import ./mocks/generated-mock.tsv",
"import:default": "npm run ts ./src/main.cli.ts -- --import ./mocks/mock-data.tsv admin test localhost six-cities qwerty",
"import:generated": "npm run ts ./src/main.cli.ts -- --import ./mocks/generated-mock.tsv admin test localhost six-cities qwerty",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Коннекты к БД нужно читать из .env файла. Эти данные могут быть секретные и в гите они не должны светится

this.onCompleteImport = this.onCompleteImport.bind(this);

this.logger = new ConsoleLogger();
this.offerService = new DefaultOfferService(this.logger, OfferModel);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не обязательно к выполнению
Тут мы создаем экземпляр класса, которые трубуют разные зависимости. Ранее в файле src/main.rest.ts мы сделали контейнер, в котором есть все классы, которые тут мы создаем. Предлагаю внедрить сюда эти классы как завимисости через декоратор @inject. Это даст след. приемущества:

  • код будет более читабельнее
  • если вдруг в какой-нибудь класс, например DefaultOfferService мы захотим добавить в конструктор новую зависимость, её надо будет прокидывать тут, хотя это задача контейнера
  • тут мы оперируем конкретным классом, а это не правильно. Надо ссылаться на интерфейс. Такой подход позволяет подменять реализации без смены логики потребителя. Представь, ты захочешь пользователей хранить не в mongodb, а в другой базе данных. При таком подходе как сейчас, тебе нужно будет менять тут код, а в случае контейнерного подхода этого делать не нужно будет. Внедрится нужный класс, логика которого спрятана под интерфейсом.

public async execute(...parameters: string[]): Promise<void> {
const [filename] = parameters;
const fileReader = new TSVFileReader(filename.trim());
public async execute(filename: string, login: string, password: string, host: string, dbname: string, salt: string): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Когда данные подключения к БД будешь брать из .env файла, то эти параметры станут не нужными.

tsconfig.json Outdated
@@ -8,6 +8,7 @@
"noImplicitAny": true,
"strictNullChecks": true,
"strictFunctionTypes": true,
"strictPropertyInitialization": false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не советую использовать эту настройку в таком виде, тк могут возникнуть проблемы в рантайме когда ты будешь ожидать, что свойтсво заполнено, а на самом деле его нет.

Если в классе все таки необходимо сделать свойство без инициализации, нужно добавить восклицательный знак в декларации свойтсва.
Вместо

export class CreateOfferDto {
  public name: string;
}

сделать так

export class CreateOfferDto {
  public name!: string;
}

Но ставить восклицательный зна нужно ставить осторожно. При использовании этого свойства ты ДОЛЖЕН БЫТЬ УВЕРЕН, ЧТО СВОЙСТВО ЗАПОЛНЕНО, иначе будет ошибка в рантайме.

Если ты ожидаешь, что оно может быть не заполнено, то нужно поставить вопросительный знак

export class CreateOfferDto {
  public name?: string;
}

@keksobot keksobot merged commit ee81966 into htmlacademy-nodejs-api:master Dec 16, 2024
1 check failed
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.

3 participants