Чернышёв Александр 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

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


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

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

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


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

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

+        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

+        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

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


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

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

+        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

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


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

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

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


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

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

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


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

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

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


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

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

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


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

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

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


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

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

+                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

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


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

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

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


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

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

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


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

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

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


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

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

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


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

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

+
+        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

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


просто mergedArray

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

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


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

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

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


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

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

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

+
+            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

+            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

fix style

89f3b86

fix style

7e19e9f

minPerDay fix

61a7853

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