Квашнина Ульяна#43

3 нояб. 2016 г., в 11:29, Ekaterina Onufrienko notifications@github.com написал(а):

Привет! :) Я посмотрю твой пр сегодня вечером либо завтра, не теряй меня, если что.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

onufrienko

onufrienko reviewed

Nov 4, 2016

View reviewed changes

lego.js Outdated

exports.isStar = false;

var priority = {

'filterIn': 1,

@onufrienko onufrienko Nov 4, 2016

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

Кавычки не нужны

onufrienko

onufrienko reviewed

Nov 4, 2016

View reviewed changes

lego.js Outdated

exports.query = function (collection) {

return collection;

var newCollection = copyCollections(collection);

var fields = Array.prototype.slice.call(arguments).slice(1);

@onufrienko onufrienko Nov 4, 2016

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

fields или все же functions?

onufrienko

onufrienko reviewed

Nov 4, 2016

View reviewed changes

lego.js

return collection;

var newCollection = copyCollections(collection);

var fields = Array.prototype.slice.call(arguments).slice(1);

function compare(one, another) {

@onufrienko onufrienko Nov 4, 2016

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

Эту функцию можно сильно сократить, возвращая, например, x - y

onufrienko

onufrienko reviewed

Nov 4, 2016

View reviewed changes

lego.js Outdated

* Выбор полей

* @params {...String}

function copyCollections(collection) {

@onufrienko onufrienko Nov 4, 2016

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

По названию можно подумать, что метод копирует несколько коллекций

onufrienko

onufrienko reviewed

Nov 4, 2016

View reviewed changes

lego.js

exports.sortBy = function (property, order) {

console.info(property, order);

return function sortBy(collection) {

@onufrienko onufrienko Nov 4, 2016

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

у тебя получилась достаточно большая функция сортировки, ее можно сделать проще

onufrienko

onufrienko reviewed

Nov 4, 2016

View reviewed changes

lego.js Outdated

collection.sort(function (one, another) {

var x = one.name;

var y = another.name;

if (x < y) {

@onufrienko onufrienko Nov 4, 2016

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

Копипаст

onufrienko

onufrienko reviewed

Nov 4, 2016

View reviewed changes

lego.js

};

exports.filterIn = function (property, values) {

return function filterIn(collection) {

@onufrienko onufrienko Nov 4, 2016

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

попробуй метод filter

@onufrienko

onufrienko commented Nov 4, 2016

Copy link

Copy Markdown

🍅

@Uliana1997


 Квашнина Ульяна

c980067

@honest-hrundel

honest-hrundel commented Nov 6, 2016

Copy link

Copy Markdown

🍏 Пройдено тестов 15 из 15

@Uliana1997


 Квашнина Ульяна

319c9fc

Sort

@honest-hrundel

honest-hrundel commented Nov 6, 2016

Copy link

Copy Markdown

🍏 Пройдено тестов 15 из 15

@Uliana1997

Uliana1997 commented Nov 6, 2016

Copy link

Copy Markdown

Author

🍏

@onufrienko

onufrienko commented Nov 8, 2016

Copy link

Copy Markdown

🚀

@honest-hrundel honest-hrundel assigned evilj0e and unassigned onufrienko

Nov 8, 2016

evilj0e

evilj0e reviewed

Nov 9, 2016

View reviewed changes

lego.js

return newCollection;

};

/**

Choose a reason for hiding this comment

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

Зачем поудаляла все jsdoc?

@Uliana1997 Uliana1997 Nov 9, 2016

Copy link

Copy Markdown

Author

Choose a reason for hiding this comment

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

мне с ними было неудобно, тут маленький код и так было понятно( мне их вернуть назад??

Choose a reason for hiding this comment

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

Их же можно сворачивать в IDE :) В данном случае, да маленький, но стоит привыкать с ним работать. Писать *doc – хороший тон.

lego.js

exports.isStar = false;

var priority = {

Choose a reason for hiding this comment

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

Стоит написать комментарий, что значат эти цифры – больше число выполнится "быстрее" или в последнюю очередь

lego.js

return x - y;

}

functions.sort(compare);

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) {

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) {

Choose a reason for hiding this comment

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

Смотри, ты здесь создаёшь массив, бегаешь в цикле, пушишь в результирующий массив. Всё это делает Array.reduce() (MDN), только более красиво.
Посмотри в его сторону. Это то, что тебе здесь нужно.

lego.js

exports.sortBy = function (property, order) {

console.info(property, order);

return function sortBy(collection) {

Choose a reason for hiding this comment

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

Ульяна, у тебя есть идеи как оптимизировать эту функцию?

@Uliana1997 Uliana1997 Nov 9, 2016

Copy link

Copy Markdown

Author

Choose a reason for hiding this comment

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

есть, сейчас я исправлю всё

Choose a reason for hiding this comment

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

👍
Супер! Спасибо

@evilj0e

honest-hrundel commented Nov 10, 2016

Copy link

Copy Markdown

🍅 Не пройден линтинг или базовые тесты

@Uliana1997


 Квашнина Ульяна

b0bb7b4

@honest-hrundel

honest-hrundel commented Nov 10, 2016

Copy link

Copy Markdown

🍏 Пройдено тестов 15 из 15

@Uliana1997

Uliana1997 commented Nov 10, 2016

Copy link

Copy Markdown

Author

🍏

@Uliana1997

Uliana1997 commented Nov 10, 2016

Copy link

Copy Markdown

Author

Только я не знаю как заменить reduce, то что вы описываете делает вроде map, то есть мы создаем новый массив вызывая функцию в нем
А reduce применяет функцию и сводит к одному значению, или я не так что то понимаю??