-
Notifications
You must be signed in to change notification settings - Fork 70
Квашнина Ульяна #43
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
base: master
Are you sure you want to change the base?
Квашнина Ульяна #43
Conversation
|
🍅 Пройдено тестов 10 из 15 |
sort
|
🍅 Пройдено тестов 10 из 15 |
sortby
|
🍅 Пройдено тестов 14 из 15 |
check
|
🍅 Пройдено тестов 14 из 15 |
format
|
🍅 Пройдено тестов 14 из 15 |
|
🍅 Пройдено тестов 14 из 15 |
last
|
🍅 Пройдено тестов 14 из 15 |
laaaast check
|
🍅 Пройдено тестов 10 из 15 |
check
|
🍏 Пройдено тестов 15 из 15 |
|
Привет! :) Я посмотрю твой пр сегодня вечером либо завтра, не теряй меня, если что. |
|
Хорошо) Отправлено с iPhone
|
lego.js
Outdated
| exports.isStar = false; | ||
|
|
||
| var priority = { | ||
| 'filterIn': 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Кавычки не нужны
lego.js
Outdated
| exports.query = function (collection) { | ||
| return collection; | ||
| var newCollection = copyCollections(collection); | ||
| var fields = Array.prototype.slice.call(arguments).slice(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fields или все же functions?
| return collection; | ||
| var newCollection = copyCollections(collection); | ||
| var fields = Array.prototype.slice.call(arguments).slice(1); | ||
| function compare(one, another) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Эту функцию можно сильно сократить, возвращая, например, x - y
lego.js
Outdated
| * Выбор полей | ||
| * @params {...String} | ||
| */ | ||
| function copyCollections(collection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По названию можно подумать, что метод копирует несколько коллекций
| */ | ||
| exports.sortBy = function (property, order) { | ||
| console.info(property, order); | ||
| return function sortBy(collection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
у тебя получилась достаточно большая функция сортировки, ее можно сделать проще
lego.js
Outdated
| collection.sort(function (one, another) { | ||
| var x = one.name; | ||
| var y = another.name; | ||
| if (x < y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Копипаст
| }; | ||
|
|
||
| exports.filterIn = function (property, values) { | ||
| return function filterIn(collection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
попробуй метод filter
|
🍅 |
|
🍏 Пройдено тестов 15 из 15 |
Sort
|
🍏 Пройдено тестов 15 из 15 |
|
🍏 |
|
🚀 |
| return newCollection; | ||
| }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зачем поудаляла все jsdoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
мне с ними было неудобно, тут маленький код и так было понятно( мне их вернуть назад??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Их же можно сворачивать в IDE :) В данном случае, да маленький, но стоит привыкать с ним работать. Писать *doc – хороший тон.
| */ | ||
| exports.isStar = false; | ||
|
|
||
| var priority = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Стоит написать комментарий, что значат эти цифры – больше число выполнится "быстрее" или в последнюю очередь
|
|
||
| return x - y; | ||
| } | ||
| functions.sort(compare); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не старайся усложнять там, где всё можно сделать проще. Когда функция сортировки достаточно простая, не стоит выносить её в отдельную функцию. К тому же её можно написать сильно короче и проще:
functions.sort(function (a, b) {
return priority[a.name] - priority[b.name];
});
lego.js
Outdated
| return x - y; | ||
| } | ||
| functions.sort(compare); | ||
| functions.forEach(function (func) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func стоит переименовать. Нейминг в данном случае ничего не говорит.
lego.js
Outdated
| console.info(property, values); | ||
| return function select(collection) { | ||
| var result = []; | ||
| collection.forEach(function (friend) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Смотри, ты здесь создаёшь массив, бегаешь в цикле, пушишь в результирующий массив. Всё это делает Array.reduce() (MDN), только более красиво.
Посмотри в его сторону. Это то, что тебе здесь нужно.
| */ | ||
| exports.sortBy = function (property, order) { | ||
| console.info(property, order); | ||
| return function sortBy(collection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ульяна, у тебя есть идеи как оптимизировать эту функцию?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
есть, сейчас я исправлю всё
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Супер! Спасибо
|
🍅 |
|
🍅 Не пройден линтинг или базовые тесты |
|
🍏 Пройдено тестов 15 из 15 |
|
🍏 |
|
Только я не знаю как заменить reduce, то что вы описываете делает вроде map, то есть мы создаем новый массив вызывая функцию в нем |
|
Итоговым значением может же быть и массив. Вот, прочитай, тут про reduce. |
|
🍏 Пройдено тестов 15 из 15 |
|
🍏 |
|
исправила все замечания |
|
Ульяна, не все комментарии учтены. |
|
🍅 Не пройден линтинг или базовые тесты |
|
🍏 Пройдено тестов 15 из 15 |
|
🍏 |
|
|
||
| return; | ||
| if (order === 'asc') { | ||
| collection.sort(compare); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Метод sort – не чистая функция. Только что ты мутировала коллекцию. Требования задания не учтены.
|
🍅 |
|
⬆️ @Uliana1997 |
No description provided.