Чернышёв Александр by aleksch · Pull Request #42 · urfu-2016/javascript-task-3

aleksch · 2016-10-25T23:24:08Z

No description provided.

honest-hrundel · 2016-10-25T23:24:29Z

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

honest-hrundel · 2016-10-25T23:26:41Z

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

honest-hrundel · 2016-10-26T18:37:32Z

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

honest-hrundel · 2016-10-26T18:37:33Z

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

ninjagrizzly · 2016-10-27T22:06:55Z

robbery.js

 exports.getAppropriateMoment = function (schedule, duration, workingHours) {
-    console.info(schedule, duration, workingHours);
+
+    var arrayOfAllTime = getAllArray();


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

ninjagrizzly · 2016-10-27T22:07:47Z

robbery.js

+
+        var arrayOfTimeRusty = getArrayOfTimes(schedule.Rusty);
+
+        var arrayOfTimeLinus = getArrayOfTimes(schedule.Linus);


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

ninjagrizzly · 2016-10-27T22:08:01Z

robbery.js

+        var arrayOfTimeLinus = getArrayOfTimes(schedule.Linus);
+        var bankStartWork = getIntOfTimeWithMainUTC(workingHours.from);
+        var bankFinishWork = getIntOfTimeWithMainUTC(workingHours.to);
+        var array = arrayOfTimeDanny.concat(arrayOfTimeRusty).concat(arrayOfTimeLinus);


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

ninjagrizzly · 2016-10-27T22:08:18Z

robbery.js

+        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]);


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

ninjagrizzly · 2016-10-27T22:08:34Z

robbery.js

+        return timeArray;
+    }
+
+    function mergeRange(array) {


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

ninjagrizzly · 2016-10-27T22:08:46Z

robbery.js

+        var resultArray = [];
+        var prevStart = sortedArray[0][0];
+        var prevFinish = sortedArray[0][1];
+        for (var e = 1; e < sortedArray.length; e++) {


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

ninjagrizzly · 2016-10-27T22:09:07Z

robbery.js

+    }
+
+    function getUTC(stringTime) {
+        return Number(stringTime.substring(stringTime.length - 1));


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

ninjagrizzly · 2016-10-27T22:09:55Z

robbery.js

+        return Number(stringTime.substring(stringTime.length - 1));
+    }
+
+    function getIntOfTimeWithMainUTC(stringTime) {


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

ninjagrizzly · 2016-10-27T22:10:00Z

robbery.js

+
+    function getIntOfTimeWithMainUTC(stringTime) {
+        var mainUTC = getUTC(workingHours.from);
+        if (stringTime.length === 7) {


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

ninjagrizzly · 2016-10-27T22:11:53Z

robbery.js

+        var day = 0;
+        if (stringTime.substring(0, 2) === 'ВТ') {
+            day = 1440;
+        } else if (stringTime.substring(0, 2) === 'СР') {


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

ninjagrizzly · 2016-10-27T22:12:48Z

robbery.js

+        if (stringTime.substring(0, 2) === 'ВТ') {
+            day = 1440;
+        } else if (stringTime.substring(0, 2) === 'СР') {
+            day = 2880;


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

ninjagrizzly · 2016-10-27T22:13:34Z

robbery.js

+            if (!this.exists()) {
+                return '';
+            }
+            var DD = Math.floor(answer / 1440);


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

ninjagrizzly · 2016-10-27T22:14:29Z

robbery.js

+                day = 'ВТ';
+            } else if (DD === 2) {
+                day = 'СР';
+            }


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

ninjagrizzly · 2016-10-27T22:19:44Z

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

ninjagrizzly · 2016-10-27T22:20:01Z

🍅

honest-hrundel · 2016-10-30T15:04:15Z

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

honest-hrundel · 2016-10-30T15:07:57Z

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

honest-hrundel · 2016-10-30T15:30:07Z

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

honest-hrundel · 2016-10-30T15:30:07Z

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

ninjagrizzly · 2016-11-01T10:19:37Z

robbery.js

 exports.getAppropriateMoment = function (schedule, duration, workingHours) {
-    console.info(schedule, duration, workingHours);
+
+    var minPerDay = 1440;


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

ninjagrizzly · 2016-11-01T10:20:07Z

robbery.js

+
+    var answerArray = getAnswerArray();
+
+    var answer = answerArray.length === 0 ? -1 : answerArray[0][0];


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

ninjagrizzly · 2016-11-01T10:20:28Z

robbery.js

+
+    var tryLaterCounter = 0;
+
+    function getArrOfGoodTime() {


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

ninjagrizzly · 2016-11-01T10:21:32Z

robbery.js

+            return [];
+        }
+        var resultArray = [];
+        var arr = answerArray;


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

ninjagrizzly · 2016-11-01T10:22:20Z

robbery.js

+
+        var arrIntervals = [];
+
+        for (var friend in schedule) {


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

ninjagrizzly · 2016-11-01T10:23:03Z

robbery.js

+
+        var bankStartWork = getTimestamp(workingHours.from);
+        var bankFinishWork = getTimestamp(workingHours.to);
+        arrIntervals.push([0, bankStartWork], [bankFinishWork, bankStartWork + minPerDay],


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

ninjagrizzly · 2016-11-01T10:23:23Z

robbery.js

+    function getAnswerArray() {
+        var busyIntervals = getBusyIntervals();
+
+        var sortAndMergeArray = mergeRange(busyIntervals);


просто mergedArray

ninjagrizzly · 2016-11-01T10:28:41Z

robbery.js

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


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

ninjagrizzly · 2016-11-01T10:29:33Z

robbery.js

+
+    function getTimestamp(stringTime) {
+        var mainUTC = getUTC(workingHours.from);
+        if (stringTime.length === 7) {


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

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

ninjagrizzly · 2016-11-01T10:31:31Z

robbery.js

+
+            return m + minPerHour * h;
+        }
+        var day = 0;


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

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

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

ninjagrizzly · 2016-11-01T10:31:55Z

robbery.js

+            var hours = Math.floor((answer - day * minPerDay) / minPerHour);
+            var minutes = answer - day * minPerDay - hours * minPerHour;
+
+            var days = ['ПН', 'ВТ', 'СР'];


В константы можно вынести

ninjagrizzly · 2016-11-01T10:36:19Z

🍅

first

b7f9dcb

honest-hrundel changed the title ~~Александр Чернышев~~ Чернышёв Александр Oct 25, 2016

first isStar false

f8112ec

honest-hrundel assigned ninjagrizzly Oct 25, 2016

add start

5239dbb

ninjagrizzly reviewed Oct 27, 2016

View reviewed changes

robbery.js Outdated

day = 'ВТ';

} else if (DD === 2) {

day = 'СР';

}

Copy link

ninjagrizzly Oct 27, 2016

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];

fix style

89f3b86

fix style

7e19e9f

minPerDay fix

61a7853

ninjagrizzly reviewed Nov 1, 2016

View reviewed changes

robbery.js

return [];

}

var resultArray = [];

var arr = answerArray;

Copy link

ninjagrizzly Nov 1, 2016

Choose a reason for hiding this comment

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

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

ninjagrizzly reviewed Nov 1, 2016

View reviewed changes


		var arrayOfTimeRusty = getArrayOfTimes(schedule.Rusty);

		var arrayOfTimeLinus = getArrayOfTimes(schedule.Linus);


		var answerArray = getAnswerArray();

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

Conversation

aleksch commented Oct 25, 2016

Uh oh!

honest-hrundel commented Oct 25, 2016

Uh oh!

honest-hrundel commented Oct 25, 2016

Uh oh!

honest-hrundel commented Oct 26, 2016

Uh oh!

honest-hrundel commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninjagrizzly Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninjagrizzly commented Oct 27, 2016

Uh oh!

ninjagrizzly commented Oct 27, 2016

Uh oh!

honest-hrundel commented Oct 30, 2016

Uh oh!

honest-hrundel commented Oct 30, 2016

Uh oh!

honest-hrundel commented Oct 30, 2016

Uh oh!

honest-hrundel commented Oct 30, 2016

Uh oh!

ninjagrizzly Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninjagrizzly Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninjagrizzly Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ninjagrizzly Oct 27, 2016 •

edited

Loading

ninjagrizzly Nov 1, 2016 •

edited

Loading

ninjagrizzly Nov 1, 2016 •

edited

Loading

ninjagrizzly Nov 1, 2016 •

edited

Loading