Skip to content

Conversation

@ToshchevIvan
Copy link

No description provided.

@honest-hrundel
Copy link

🍅 Пройдено тестов 12 из 24

@honest-hrundel
Copy link

🍅 Пройдено тестов 10 из 24

@honest-hrundel
Copy link

🍅 Пройдено тестов 16 из 24

@honest-hrundel
Copy link

🍅 Пройдено тестов 18 из 24

@honest-hrundel
Copy link

🍅 Пройдено тестов 18 из 24

@honest-hrundel
Copy link

🍅 Пройдено тестов 16 из 24

@ToshchevIvan
Copy link
Author

🔔 Что же может быть не так?

@honest-hrundel
Copy link

🍅 Пройдено тестов 16 из 24

Copy link
Contributor

@f0rmat1k f0rmat1k left a comment

Choose a reason for hiding this comment

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

Сори за задержку фидбека, я добавлю несколько дней к срокам.
Что касается ошибок тестов. Давай я тебе подскажу, названия упавших тестов, а ты по ним подумаешь, что пошло не так. Если не будет идей, можно заглянуть в соседние решения.
Итак:

  1. Iterator
    а) должен перебирать всех друзей в алфавитоном порядке. (Попробуй без localCompare, имена друзей могут начинаться с чисел).

  2. LimitedIterator
    а) должен перебирать всех друзей

  3. Подумай о поведении кода, если круг передан не совсем корректный.

  4. Фильтры парней и девушек у тебя иногда обходят не свойственный им пол.

lib.js Outdated
*/
function Iterator(friends, filter) {
console.info(friends, filter);
function Iterator(friends, filter, maxLevel = Infinity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Не критично, но стоило попробовать сделать через классы.

lib.js Outdated
function MaleFilter() {
console.info('MaleFilter');
Object.setPrototypeOf(this, Filter.prototype);
this.check = friend => friend.gender === 'male';
Copy link
Contributor

Choose a reason for hiding this comment

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

Зачем нам для каждого инстанца этого класса генерировать новую такую функцию, почему она не в прототипе? Кроме того, т.к. она не оперирует внутри себя this'ом, ее вообще стоит сделать статическим свойством класса (кстати, как раз удобно, когда через Class).
А еще можно сделать ее только в прототитпе Filter'а, немного изменив:

Filter.prototype.check = function(friend){
   return friend === this.gender;
}

А gender можно сетить в конструкторах типа MaleFilter: this.gender = 'male';

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

lib.js Outdated
*/
function Filter() {
console.info('Filter');
this.check = () => true;
Copy link
Contributor

Choose a reason for hiding this comment

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

А это точно тут нужно? Я нигде не видел прямых вызовов new Filter, ты ток прототип наследуешь.

lib.js Outdated
*/
function MaleFilter() {
console.info('MaleFilter');
Object.setPrototypeOf(this, Filter.prototype);
Copy link
Contributor

Choose a reason for hiding this comment

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

Это не критично, но этот метод считается очень медленным и труднооптимизируемым. Вместо него рекомендуется пользоваться Object.create:

MaleFilter.prototype = Object.create(Filter.prototype);

К тому же сейчас ты прототип меняешь при каждом вызове new MaleFilter, а так он установится ток 1 раз для самого MakeFilter.

@honest-hrundel
Copy link

🍅 Пройдено тестов 19 из 24

@honest-hrundel
Copy link

🍅 Пройдено тестов 17 из 24

@honest-hrundel
Copy link

🍅 Пройдено тестов 19 из 24

@honest-hrundel
Copy link

🍅 Пройдено тестов 19 из 24

@f0rmat1k
Copy link
Contributor

f0rmat1k commented Nov 30, 2017

@ToshchevIvan

  1. Попробуй заменить в тестах name: 'Sam' на name: '1Sam' и посмотри, что получится.
  2. Попробуй зациклить лучших друзей, посмотри, все ли получается так, как ожидается.
  3. Проверь результаты limitedIterator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants