Трофимов Павел 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

robbery.js

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

-/**


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

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

robbery.js

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


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

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

robbery.js

+    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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

+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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

+                    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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

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


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

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

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

robbery.js

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


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

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

robbery.js

+    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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

+                    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

robbery.js

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


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

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

robbery.js

+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

robbery.js

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

robbery.js

+    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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

+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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

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


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

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

robbery.js

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


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

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