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

Бёрдов Дмитрий #36

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

Conversation

CLearERR
Copy link

No description provided.

@Zhigalov Zhigalov self-assigned this Apr 27, 2017
@Zhigalov
Copy link
Contributor

Код не собирается
image

@CLearERR
Copy link
Author

CLearERR commented May 1, 2017

Файл компилируется, расширено покрытие тестами.

Copy link
Contributor

@Zhigalov Zhigalov left a comment

Choose a reason for hiding this comment

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

Переделай работу с ошибками, после этого посмотрю ещё раз

getPokerHand.js Outdated
return 'Покер';
try {

dice.sort();
Copy link
Contributor

Choose a reason for hiding this comment

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

Проверить является ли переменная массивом можно оператором Array.isArray(dice)

getPokerHand.js Outdated
catch (err) {

res = ('Не массив');
return res;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ошибку надо не возвращать return а выбрасывать throw:

throw new Error('Не массив');

});


describe('getPokerHand', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Нужно объявить один describe а внутри него несколько it:

describe('getPokerHand', () => {
   it('should ...', () => {});
   it('should ...', () => {});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Это замечание всё ещё актуально

@CLearERR
Copy link
Author

CLearERR commented May 2, 2017

Переделана работа с ошибками, из TODO - рефакторинг файла с тестами.

Copy link
Contributor

@Zhigalov Zhigalov left a comment

Choose a reason for hiding this comment

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

Уже гораздо лучше!


j = 0;
for(var i = 1; i<5; i++){
if ((isNaN(dice[i])) || (isNaN(dice[j]))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

А можно ограничиться только одной проверкой?
То есть не два числа проверять, а только одно во всех случаях?

});


describe('getPokerHand', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Это замечание всё ещё актуально

} catch (error) {
assert.equal(error.message, 'Не массив');
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Попробуй воспользоваться assert.throws
Твои тесты станут компактнее
Документацию можно почитать по ссылке https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message

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