Skip to content

Катюшкина Настя#10

Open
NastyaKatyushkina wants to merge 3 commits intoRTF-Angular-2021:masterfrom
NastyaKatyushkina:katyushkina
Open

Катюшкина Настя#10
NastyaKatyushkina wants to merge 3 commits intoRTF-Angular-2021:masterfrom
NastyaKatyushkina:katyushkina

Conversation

@NastyaKatyushkina
Copy link

No description provided.

export class MoneyRepository {
private _repository: any;
constructor(initialRepository: any) {
private _repository: Array<object>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Object в типизации - страшный грех, также как и any


this._repository.forEach(i =>
{
for (let key in i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Для обхода массива следует использовать методы массива

{
for (let key in i)
{
if (i['moneyInfo']['currency']===currency && copy >= i['count'] && (rub.includes(i['count']) || doll.includes(i['count'])))
Copy link
Contributor

Choose a reason for hiding this comment

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

Обращаться к объекту через скобки и строчную нотацию следует только в одном случае - когда название поля состоит из нескольких слова, но это полная жесть и так никто не делает

return copy === 0;
}

public money (a: IMoneyUnit, b: IMoneyUnit): number
Copy link
Contributor

Choose a reason for hiding this comment

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

что с названием метода?


public authorize(userId: any, cardId: any, cardPin: any): any {

public authorize(userId: string, cardId: string, cardPin: string): boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Этот метод всегда возвращает false

{
return this.registerForUserNewCard(argsForChangeFunction);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

За эту задачу 3 балла

{
moneyInfo:
{
denomination: (+moneyUnit.moneyInfo.denomination * c).toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Неявное преобразование - зло


for (const moneyUnit of moneyUnits)
{
const factor = this._currentCard.currency === moneyUnit.moneyInfo.currency ? 1 : (this._currentCard.currency === Currency.RUB ? 70 : (1 / 70))
Copy link
Contributor

Choose a reason for hiding this comment

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

Опять мясо, нужно разбивать такое на более простую логику

@Toouren
Copy link
Contributor

Toouren commented Apr 14, 2021

Пока 3 балла только за одну задачу, за все остальные 0
Основные ошибки:

  1. Object в типах
  2. Неявные преобразования
  3. Местами слишком сложная логика, которую надо деструктуризировать
  4. Для обхода массива надо использовать методы объекта массива
  5. Получение поля объекта через квадратные скобки

}
}
} No newline at end of file
}

Choose a reason for hiding this comment

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

2 балла

}
}
}
} No newline at end of file

Choose a reason for hiding this comment

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

2 балла

return false;
}
} No newline at end of file
}

Choose a reason for hiding this comment

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

2 балла

return res;

}
} No newline at end of file

Choose a reason for hiding this comment

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

2 балла

@m-abrosimov
Copy link

m-abrosimov commented May 26, 2021

+8 баллов

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