Трофимов Павел by tgkd · Pull Request #43 · urfu-2016/javascript-task-3

tgkd · 2016-10-26T06:45:43Z

No description provided.

honest-hrundel · 2016-10-26T06:46:04Z

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

honest-hrundel · 2016-10-26T06:46:05Z

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

bazhenova · 2016-10-27T18:11:22Z

@@ -1,49 +1,198 @@
 'use strict';

-/**


Зачем ты удалил все JsDoc? Давай их вернем обратно. Хорошей практикой считается документировать публичные методы, так как по параметрам сложно догадаться, что они содержат

bazhenova · 2016-10-27T18:12:01Z

- * @param {String} workingHours.to – Время закрытия, например, "18:00+5"
- * @returns {Object}
- */
+var days = ['ПН', 'ВТ', 'СР'];


Давай назовем как-нибудь по-другому. Задумайся, какие это дни?

bazhenova · 2016-10-27T18:14:58Z

+    var timeRegExp = /([А-Я]{2}) (\d{2}):(\d{2})\+(\d+)/g;
+    var parsedTime = timeRegExp.exec(date);
+    var day = days.indexOf(parsedTime[1]) + 1;
+    var hour = parsedTime[2] - (parseInt(parsedTime[4]) - bankTimezone);


у parseInt лучше указывать вторым аргументом основание системы счисления, чтобы избежать ошибок

bazhenova · 2016-10-27T18:16:03Z

+}
+
+
+function getInterval(from, to, timezone) {


Непонятно, что за интервал ты хочешь получить

bazhenova · 2016-10-27T18:19:16Z

+
+
+function getSotredIntervals(schedule, bankTimezone) {
+    var sortedSchedule = parseSchedule(schedule, bankTimezone).sort(function (a, b) {


давай разобьем эту строчку: сначала ты парсишь - это одна переменная, а потом на ней вызывай метод sort - тебе не придется создавать новую переменную, так как данный метод изменяет исходный массив

bazhenova · 2016-10-27T18:22:25Z

+
+function joinSchedule(schedule) {
+    var fullSchedule = [];
+    var firstInterval = schedule[0];


Надо переименовать, неочевидно название. Первый интервал чего?

bazhenova · 2016-10-27T18:22:25Z

+
+
+function joinSchedule(schedule) {
+    var fullSchedule = [];


Полное расписание? Подумай, как по-другому это можно назвать

bazhenova · 2016-10-27T18:28:54Z

+
+function isCrossed(bankInterval, freeInterval) {
+    var checkFrom = checkInterval(bankInterval.from, freeInterval);
+    var checkTo = checkInterval(bankInterval.to, freeInterval);


Здесь и в строчке выше надо переименовать переменные, они должны отражать смысл

bazhenova · 2016-10-27T18:33:59Z

+function isCrossed(bankInterval, freeInterval) {
+    var checkFrom = checkInterval(bankInterval.from, freeInterval);
+    var checkTo = checkInterval(bankInterval.to, freeInterval);
+    var compareIntervals = freeInterval.from > bankInterval.from &&


Надо переименовать, непонятно, что за сравнение интервалов. По переменной должно быть понятно, что в ней лежит

bazhenova · 2016-10-27T18:34:42Z

+function getBankTimezone(time) {
+    var bankTimezone = /\+(\d+)/.exec(time)[1];
+
+    return parseInt(bankTimezone);


Укажи второй аргумент, как я писала выше

bazhenova · 2016-10-27T18:36:15Z

+    return parseInt(bankTimezone);
+}
+
+function getMomentForAttack(freeSchedule, bankSchedule, duration) {


Может быть не getMomentForAttack, а getMomentsForRobbery? :) + не забывай, что ты возвращаешь массив, поэтому в названии присутствует множественное число

bazhenova · 2016-10-27T18:37:10Z

+                    to: new Date(Math.min(bankInterval.to, freeInterval.to))
+                };
+
+                var laterTime = new Date(crossInterval.from.getTime() + (duration * 60 * 1000));


Вынеси 60 * 1000 в константу

bazhenova · 2016-10-27T18:37:33Z

+
+function getFreeSchedule(busyTime, bankTimezone) {
+    var freeTime = [];
+    var interval = {};


Непонятно, что за интервал

bazhenova · 2016-10-27T18:38:14Z

+}
+
+function getFreeSchedule(busyTime, bankTimezone) {
+    var freeTime = [];


может тогда уж freeTimes, раз это массив

bazhenova · 2016-10-27T18:39:50Z

+        }
+        interval.to = currentInterval.from;
+        freeTime.push(interval);
+        interval = {};


Ого, какое-то 'обнуление' переменной. Можешь пояснить, что ты имел в виду?

bazhenova · 2016-10-27T18:41:34Z

+
+function formatTime(time) {
+
+    return (time.toString().length === 2 ? '' : '0') + time.toString();


На вход приходит численное значение? Может тогда лучше написать так:

return (time < 10 ? '0' : '') + time;

bazhenova · 2016-10-27T18:42:10Z

+
+function createLaterTime(date, shift) {
+
+    return new Date(date.getTime() + (shift * 60 * 1000));


60 * 1000 -> константа

bazhenova · 2016-10-27T18:43:09Z

+    var bankTimezone = getBankTimezone(workingHours.from);
+    var freeTimeIntervals = getSotredIntervals(schedule, bankTimezone);
+    var bankSchedule = getBankSchedule(workingHours, bankTimezone);
+    var robberyTime = getMomentForAttack(freeTimeIntervals, bankSchedule, duration);


Тут же массив? Значит, robberyTimes

bazhenova · 2016-10-27T18:44:31Z

+
+            var time = robberyTime[0].from;
+
+            return template.replace('%HH', formatTime(time.getHours()))


Когда идет вызов нескольких функций подряд, то лучше каждую функцию начинать с новой строки. Перенеси этот replace на новую строчку. Код станет и красивее и читабельнее)

bazhenova · 2016-10-27T18:44:59Z

-         */
        tryLater: function () {
+            if (this.exists()) {
+                var laterTime = createLaterTime(robberyTime[0].from, 30);


30 -> константа

bazhenova · 2016-10-27T18:46:02Z

+                    robberyTime[0].from = laterTime;
+
+                    return true;
+                } else if (robberyTime.length > 1) {


почему именно больше 1, почему не больше 0?

bazhenova · 2016-10-27T18:46:26Z

🍅

honest-hrundel · 2016-10-28T18:00:19Z

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

honest-hrundel · 2016-10-28T18:00:20Z

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

bazhenova · 2016-10-29T17:47:19Z

+    return getFreeSchedule(joinedSchedule, bankTimezone);
+}
+
+function checkInterval(time, interval) {


В названии не отражено, на что именно ты проверяешь данный интервал. Раз ты возвращаешь булевское значение, то может быть лучше назвать как-нибудь isIntersected или isIncluded или что-то в этом роде (посмотри, что больше подходит)

bazhenova · 2016-10-29T17:54:00Z

+function isCrossedIntervals(bankInterval, freeInterval) {
+    var checkTimeFrom = checkInterval(bankInterval.from, freeInterval);
+    var checkTimeTo = checkInterval(bankInterval.to, freeInterval);
+    var compareTimes = freeInterval.from > bankInterval.from &&


Если я правильно поняла, то compareTimes - это isRobberyTime

bazhenova · 2016-10-29T17:56:05Z

+}
+
+function getMomentsForRobbery(freeSchedule, bankSchedule, duration) {
+    var timesToRob = [];


Лучше не сокращать слова в названиях: timesOfRobbery либо robberyTimes

bazhenova · 2016-10-29T18:00:01Z

И еще немного комментариев)

bazhenova · 2016-10-29T18:00:08Z

🍅

bazhenova · 2016-11-02T03:51:18Z

Ты будешь доделывать задачу?

tgkd · 2016-11-02T04:02:18Z

@bazhenova, да, в процессе. Не знаю как правильно переименовать переменные.

bazhenova · 2016-11-02T13:29:01Z

@tgkd Скажи, с чем проблемы, лучше напиши в slack, помогу

honest-hrundel · 2016-11-02T16:59:46Z

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

honest-hrundel · 2016-11-02T17:04:15Z

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

honest-hrundel · 2016-11-02T17:04:16Z

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

bazhenova · 2016-11-02T17:14:07Z

+    return interval.from <= time && time <= interval.to;
+}
+
+function createDate(currentTime, firstTime) {


еще бы вот тут переименовать, ты же выбираешь максимум из дат, как я поняла; то есть по сути позднее из двух времен. И раз ты возвращаешь дату, то можно назвать getLaterTime. В аргументы передавай firstTime и secondTime

Ну или getLaterDate

bazhenova · 2016-11-02T17:17:03Z

Еще пару переименований сделай, и я отдам тебя ментору. Вроде что-то хочется поправить, но не могу понять, что именно. Посмотрим, что скажет ментор

bazhenova · 2016-11-02T17:17:14Z

🍅

honest-hrundel · 2016-11-02T17:31:52Z

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

honest-hrundel · 2016-11-02T17:31:53Z

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

bazhenova · 2016-11-02T17:35:05Z

🚀

gogoleff · 2016-11-02T19:59:50Z

🚀

ulitcos · 2016-11-05T16:05:00Z

+    var bankTimezone = /\+(\d+)/.exec(time)[1];
+
+    return parseInt(bankTimezone, 10);
+}


Ты определяешь часовой пояс банка, а потом везде его прокидываешь в качестве аргумента. Проще сохранить в переменную

ulitcos · 2016-11-05T16:09:08Z

+
+
+function getSortedIntervals(schedule, bankTimezone) {
+    var sortedSchedule = parseSchedule(schedule, bankTimezone);


Тут еще пока не отсортировано. Отсортированным становится только на следующем шаге

ulitcos · 2016-11-05T16:28:29Z

+function getNewDate(date, bankTimezone) {
+    var timeRegExp = /([А-Я]{2}) (\d{2}):(\d{2})\+(\d+)/g;
+    var parsedTime = timeRegExp.exec(date);
+    var day = ROBBERY_DAYS.indexOf(parsedTime[1]) + 1;


ROBBERY_DAYS.indexOf(parsedTime[1]) + 1
В условии сказано, что проверять данные не надо, но допустим в расписании появится ЧТ. ЧТ, как день, является валидным значением, но твой код вернет 0(воскресенье), вместо 4(четверга).

Получается, что шаг в лево и твое решение перестанет работать. Надо переписать на что-то более надежное

ulitcos · 2016-11-05T16:34:58Z

+            busyIntervals.push(totalBusyInterval);
+            totalBusyInterval = schedule[i];
+        }
+    }


Почему тут for, а везде выше forEach?

ulitcos · 2016-11-05T16:39:54Z

+    var freeIntervals = [];
+    var currentFreeInterval = {};
+
+    currentFreeInterval.from = getNewDate('ПН 00:00+' + bankTimezone, bankTimezone);


'ПН 00:00+' - надо унести в константы.

ulitcos · 2016-11-05T16:42:34Z

+    busyTime.forEach(function (currentBusyInterval) {
+        if (currentBusyInterval.from === currentBusyInterval.to) {
+            return;
+        }


А в каком случае может отработать это условие? Ведь если у интервала начало и конец совпадают, то это уже не интервал, а момент времени.

ulitcos · 2016-11-05T16:48:30Z

+                }
+            }
+        });
+    });


Тут все интервалы сравниваются с другими интервалами, но это не очень хорошо с точки зрения производительности. Зачем сравнивать время работы банка в понедельник с свободным временем грабителей во вторник или среду. Попробуй как-нибудь оптимизировать проверку по дням

ulitcos · 2016-11-07T04:03:04Z

🍅

ulitcos · 2016-12-12T07:40:56Z

Последняя активность 7-го ноября

init

53099ef

honest-hrundel assigned bazhenova Oct 26, 2016

bazhenova reviewed Oct 27, 2016

View reviewed changes

изменения по комментариям

7b242b0

bazhenova reviewed Oct 29, 2016

View reviewed changes

изменены названия переменных

81d99a7

lint

df9c5d0

bazhenova reviewed Nov 2, 2016

View reviewed changes

lint

e5b889a

honest-hrundel assigned meded90 and unassigned bazhenova Nov 2, 2016

gogoleff unassigned meded90 Nov 2, 2016

honest-hrundel assigned ulitcos Nov 2, 2016

ulitcos added the extra label Nov 5, 2016

ulitcos reviewed Nov 5, 2016

View reviewed changes

ulitcos assigned mokhov Dec 12, 2016

ulitcos removed their assignment Dec 12, 2016



		function getSotredIntervals(schedule, bankTimezone) {
		var sortedSchedule = parseSchedule(schedule, bankTimezone).sort(function (a, b) {


		function formatTime(time) {

		return (time.toString().length === 2 ? '' : '0') + time.toString();


		function createLaterTime(date, shift) {

		return new Date(date.getTime() + (shift * 60 * 1000));


		var time = robberyTime[0].from;

		return template.replace('%HH', formatTime(time.getHours()))



		function getSortedIntervals(schedule, bankTimezone) {
		var sortedSchedule = parseSchedule(schedule, bankTimezone);

Conversation

tgkd commented Oct 26, 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!

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!

Choose a reason for hiding this comment

Uh oh!

bazhenova commented Oct 27, 2016

Uh oh!

honest-hrundel commented Oct 28, 2016

Uh oh!

honest-hrundel commented Oct 28, 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!

bazhenova commented Oct 29, 2016

Uh oh!

bazhenova commented Oct 29, 2016

Uh oh!

bazhenova commented Nov 2, 2016

Uh oh!

tgkd commented Nov 2, 2016

Uh oh!

bazhenova commented Nov 2, 2016

Uh oh!

honest-hrundel commented Nov 2, 2016

Uh oh!

honest-hrundel commented Nov 2, 2016

Uh oh!

honest-hrundel commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment