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

Codewars + задание на классы #14

Open
wants to merge 4 commits into
base: Midjaeva_Evgenija_Gennadevna
Choose a base branch
from

Conversation

Rey1147
Copy link

@Rey1147 Rey1147 commented Oct 10, 2023

No description provided.

@@ -4,7 +4,8 @@
"description": "",
"main": "index.js",
"scripts": {
"dev": "ts-node --script-mode src/index.ts",
"dev": "ts-node --script-mode src/gun.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

лучше оставить как было - запуск - это запуск основного файла в проекте

@@ -0,0 +1,42 @@
export class Gun {
readonly serialNumber: string = 's00001';
Copy link
Contributor

Choose a reason for hiding this comment

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

присваиваете значение в конструкторе - для счетчика можно завести приватную статическую переменную и исходя из нее увеличивать счетчик и устанавливать соответствующий серийный номер

constructor(modelName: string, magazine: number, public bullets?: number) {
this.model = modelName;
this.magazine = magazine;
this.bullets = this.bullets < this.magazine && this.bullets >= 0 ? this.bullets : this.magazine;
Copy link
Contributor

Choose a reason for hiding this comment

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

если я передам отрицательное или слишком большое число - то стоит бросать исключение - exception

this.bullets = this.bullets < this.magazine && this.bullets >= 0 ? this.bullets : this.magazine;
}
// инфо :)
info() {
Copy link
Contributor

Choose a reason for hiding this comment

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

давайте переименуем в toString() : string <- указывайте тип возвращаемого значения

export class Gun {
readonly serialNumber: string = 's00001';
private aMagazine: number;
model: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

protected

constructor(modelName: string, magazine: number, public bullets?: number) {
this.model = modelName;
this.magazine = magazine;
this.bullets = this.bullets < this.magazine && this.bullets >= 0 ? this.bullets : this.magazine;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


shotCount = 0;

constructor(modelName: string, magazine: number, public bullets?: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constructor(modelName: string, magazine: number, public bullets?: number) {
constructor(modelName: string, magazine: number, protected bullets?: number) {

shot(): string {
if (this.bullets > 0) {
this.shotCount += 1;
this.bullets -= 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

создайте get для protected поля bullets


// проверка магазина
set magazine(magazine: number) {
this.aMagazine = magazine >= 0 && magazine < 21 ? magazine : this.aMagazine ?? 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

если указано неверное число патрон - бросайте exception

Copy link
Contributor

Choose a reason for hiding this comment

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

trow new Exception(wrong amount of bullets)

});
});

describe('Testing gun methods', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🔕 different describe test suites could be placed in separate files.

Comment on lines +53 to +54
expect(first.shot()).toEqual('The gun Пистолет 8 fired 3 times');
expect(first.shot()).toEqual('The gun Пистолет 8 fired 4 times');
Copy link
Contributor

Choose a reason for hiding this comment

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

наверное, достаточно было бы 2х вызовов


describe('Testing gun methods', () => {
it('The gun is firing', () => {
const first = new Gun('Пистолет 7', 20, 20);
Copy link
Contributor

Choose a reason for hiding this comment

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

можно объявить gun вне it, но внутри describe - тогда этот gun использовался бы в нескольких тестах

@jskonst
Copy link
Contributor

jskonst commented Nov 18, 2023

Пока поправьте предыдущие review point'ы

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