Search code, repositories, users, issues, pull requests...

@aleksch aleksch commented Oct 25, 2016

Copy link

Copy Markdown

No description provided.


 first

b7f9dcb

@honest-hrundel honest-hrundel changed the title ~~(削除) Александр Чернышев (削除ここまで)~~ (追記) Чернышёв Александр (追記ここまで)

Oct 25, 2016

honest-hrundel commented Oct 25, 2016

Copy link

Copy Markdown

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


 first isStar false

f8112ec

honest-hrundel commented Oct 25, 2016

Copy link

Copy Markdown

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

@honest-hrundel honest-hrundel assigned ninjagrizzly

Oct 25, 2016


 add start

5239dbb

honest-hrundel commented Oct 26, 2016

Copy link

Copy Markdown

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

honest-hrundel commented Oct 26, 2016

Copy link

Copy Markdown

@NiKiT0S обрати внимание решено доп. задание

ninjagrizzly reviewed

robbery.js Outdated

exports.getAppropriateMoment = function (schedule, duration, workingHours) {

console.info(schedule, duration, workingHours);

var arrayOfAllTime = getAllArray();

@ninjagrizzly ninjagrizzly Oct 27, 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.

Массив всего времени? Стоит вложить больше семантики в название переменной

ninjagrizzly reviewed

robbery.js Outdated

var arrayOfTimeRusty = getArrayOfTimes(schedule.Rusty);

var arrayOfTimeLinus = getArrayOfTimes(schedule.Linus);

@ninjagrizzly ninjagrizzly Oct 27, 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.

А если банда из 100 человек, 100 переменных создавать?
Смотри в сторону таких методов как map, reduce и т.п.

ninjagrizzly reviewed

robbery.js Outdated

var arrayOfTimeLinus = getArrayOfTimes(schedule.Linus);

var bankStartWork = getIntOfTimeWithMainUTC(workingHours.from);

var bankFinishWork = getIntOfTimeWithMainUTC(workingHours.to);

var array = arrayOfTimeDanny.concat(arrayOfTimeRusty).concat(arrayOfTimeLinus);

@ninjagrizzly ninjagrizzly Oct 27, 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.

array - слишком абстрактно. Массив чего?

ninjagrizzly reviewed

robbery.js Outdated

var array = arrayOfTimeDanny.concat(arrayOfTimeRusty).concat(arrayOfTimeLinus);

array.push([0, bankStartWork], [bankFinishWork, bankStartWork + 1440],

[bankFinishWork + 1440, bankStartWork + 1440 * 2],

[bankFinishWork + 1440 * 2, 1440 * 3 - 1]);

@ninjagrizzly ninjagrizzly Oct 27, 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.

Что за число 1440 ? Стоит вынести в константы

ninjagrizzly reviewed

robbery.js Outdated

return timeArray;

}

function mergeRange(array) {

@ninjagrizzly ninjagrizzly Oct 27, 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.

Функция называется merge, но внутри еще и сортирует, надо разбить эти действия на 2

ninjagrizzly reviewed

robbery.js Outdated

var resultArray = [];

var prevStart = sortedArray[0][0];

var prevFinish = sortedArray[0][1];

for (var e = 1; e < sortedArray.length; e++) {

@ninjagrizzly ninjagrizzly Oct 27, 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.

Для названия счетчика принято использовать i, j

ninjagrizzly reviewed

robbery.js

}

function getUTC(stringTime) {

return Number(stringTime.substring(stringTime.length - 1));

@ninjagrizzly ninjagrizzly Oct 27, 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.

А если будет +10 ? тогда вернет 0, неверно

ninjagrizzly reviewed

robbery.js Outdated

return Number(stringTime.substring(stringTime.length - 1));

}

function getIntOfTimeWithMainUTC(stringTime) {

@ninjagrizzly ninjagrizzly Oct 27, 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.

Слишком замысловатое имя функции, getTimestamp подойдет лучше

ninjagrizzly reviewed

robbery.js

function getIntOfTimeWithMainUTC(stringTime) {

var mainUTC = getUTC(workingHours.from);

if (stringTime.length === 7) {

@ninjagrizzly ninjagrizzly Oct 27, 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.

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

ninjagrizzly reviewed

robbery.js

var day = 0;

if (stringTime.substring(0, 2) === 'ВТ') {

day = 1440;

} else if (stringTime.substring(0, 2) === 'СР') {

@ninjagrizzly ninjagrizzly Oct 27, 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.

Для вычленения частей даты проще написать одно регулярное выражение, чем каждый раз искать substring.
Попробуй реализовать через него

ninjagrizzly reviewed

robbery.js Outdated

if (stringTime.substring(0, 2) === 'ВТ') {

day = 1440;

} else if (stringTime.substring(0, 2) === 'СР') {

day = 2880;

@ninjagrizzly ninjagrizzly Oct 27, 2016 •

edited

Loading

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

Еще магические числа. Количество минут в дне можно вынести в константы, так будет семантичнее

ninjagrizzly reviewed

robbery.js Outdated

if (!this.exists()) {

return '';

}

var DD = Math.floor(answer / 1440);

@ninjagrizzly ninjagrizzly Oct 27, 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.

DD - как будто константа, можно же назвать просто days, MM - minutes, HH - hours

ninjagrizzly reviewed

robbery.js Outdated

day = 'ВТ';

} else if (DD === 2) {

day = 'СР';

}

@ninjagrizzly ninjagrizzly Oct 27, 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.

days = ['ПН', 'ВТ', 'CР'];
day = days[DD];

@ninjagrizzly

ninjagrizzly commented Oct 27, 2016

Copy link

Copy Markdown

Много замечаний к именованию переменных и функций, пытайся вложить больше смысла в названия.
Используй методы forEach, map, reduce они так и напрашиваются быть использованными в этой задаче
Хорошо бы снабдить функции хотя бы краткими комментариями (что на входе, что происходит внутри)

@ninjagrizzly

ninjagrizzly commented Oct 27, 2016

Copy link

Copy Markdown

🍅


 fix style

89f3b86

honest-hrundel commented Oct 30, 2016

Copy link

Copy Markdown

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


 fix style

7e19e9f

honest-hrundel commented Oct 30, 2016

Copy link

Copy Markdown

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


 minPerDay fix

61a7853

honest-hrundel commented Oct 30, 2016

Copy link

Copy Markdown

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

honest-hrundel commented Oct 30, 2016

Copy link

Copy Markdown

@NiKiT0S обрати внимание решено доп. задание

ninjagrizzly reviewed

robbery.js

exports.getAppropriateMoment = function (schedule, duration, workingHours) {

console.info(schedule, duration, workingHours);

var minPerDay = 1440;

@ninjagrizzly ninjagrizzly Nov 1, 2016 •

edited

Loading

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

Это константы, их нужно вынести из функции и именовать согласно принятым стандартам CONST_HUNDRED = 100

ninjagrizzly reviewed

robbery.js

var answerArray = getAnswerArray();

var answer = answerArray.length === 0 ? -1 : answerArray[0][0];

@ninjagrizzly ninjagrizzly Nov 1, 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.

Странное название переменной. Если есть ответ зачем что-то еще искать?)

ninjagrizzly reviewed

robbery.js

var tryLaterCounter = 0;

function getArrOfGoodTime() {

@ninjagrizzly ninjagrizzly Nov 1, 2016 •

edited

Loading

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

Хотелось бы увидеть комментарии к каждой функции, что на входе и что она делает со своими входными данными (т.е. этот комментарий относится ко всем функциям, а не конкретно к этой)

ninjagrizzly reviewed

robbery.js

return [];

}

var resultArray = [];

var arr = answerArray;

@ninjagrizzly ninjagrizzly Nov 1, 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.

Зачем answerArray пихать в arr

ninjagrizzly reviewed

robbery.js

var arrIntervals = [];

for (var friend in schedule) {

@ninjagrizzly ninjagrizzly Nov 1, 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.

Используй Object.keys(schedule) там встроенный hasOwnProperty

ninjagrizzly reviewed

robbery.js

var bankStartWork = getTimestamp(workingHours.from);

var bankFinishWork = getTimestamp(workingHours.to);

arrIntervals.push([0, bankStartWork], [bankFinishWork, bankStartWork + minPerDay],

@ninjagrizzly ninjagrizzly Nov 1, 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.

А если банк начинает работу с 0 часов? тогда в busyIntervals появится [0, 0]. В этом случае программа отработает правильно?

ninjagrizzly reviewed

robbery.js

function getAnswerArray() {

var busyIntervals = getBusyIntervals();

var sortAndMergeArray = mergeRange(busyIntervals);

@ninjagrizzly ninjagrizzly Nov 1, 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.

просто mergedArray

ninjagrizzly reviewed

robbery.js

}

function sortArray(array) {

return array.sort(function compareArrays(a, b) {

@ninjagrizzly ninjagrizzly Nov 1, 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.

Необязаттельно давать этой функции имя compareArrays, эту функцию ты по имени не вызываешь, она вполне может быть анонимной

ninjagrizzly reviewed

robbery.js

function getTimestamp(stringTime) {

var mainUTC = getUTC(workingHours.from);

if (stringTime.length === 7) {

@ninjagrizzly ninjagrizzly Nov 1, 2016 •

edited

Loading

Copy link

Copy Markdown

Choose a reason for hiding this comment

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

Длина строки времени работы банка может быть равна 8, в случае если сдвиг больше +9
Для вычленения минут/часов/дней я бы использовал regexp

@ninjagrizzly ninjagrizzly Nov 1, 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.

Кстати я обращал внимание на это еще в первом review

ninjagrizzly reviewed

robbery.js

return m + minPerHour * h;

}

var day = 0;

@ninjagrizzly ninjagrizzly Nov 1, 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.

var DAYS = ['ПН', 'ВТ', 'СР', 'ЧТ', 'ПТ', 'CБ', 'ВС'];
var day = DAYS[stringTime.substring(0, 2)];

Итого 2 строки вместо 6

@ninjagrizzly ninjagrizzly Nov 1, 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.

И это тоже я уже писал

ninjagrizzly reviewed