Козлова Валерия by OnaHochetNaMars · Pull Request #57 · urfu-2016/javascript-task-3

OnaHochetNaMars · 2016-10-26T18:33:01Z

No description provided.

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

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

honest-hrundel · 2016-10-26T19:26:12Z

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

honest-hrundel · 2016-10-26T19:49:06Z

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

honest-hrundel · 2016-10-26T19:52:58Z

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

honest-hrundel · 2016-10-27T19:39:00Z

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

honest-hrundel · 2016-10-28T13:45:09Z

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

honest-hrundel · 2016-10-28T16:22:50Z

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

honest-hrundel · 2016-10-28T16:40:04Z

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

honest-hrundel · 2016-10-28T17:37:58Z

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

honest-hrundel · 2016-10-28T19:29:06Z

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

honest-hrundel · 2016-10-28T20:03:46Z

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

honest-hrundel · 2016-10-31T07:31:05Z

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

по временной зоне банка

honest-hrundel · 2016-10-31T08:01:15Z

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

honest-hrundel · 2016-10-31T15:24:13Z

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

honest-hrundel · 2016-10-31T15:26:54Z

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

chipolinka · 2016-11-04T19:32:46Z

robbery.js

+    return dates[0];
+}
+
+function toFreeSchedule(schedule) {


Наверное, здесь функция должна называться getFreeTimes? Или можешь придумать еще понятнее?

chipolinka · 2016-11-04T19:35:25Z

robbery.js

+            schedule.Linus.forEach(function (k) {
+                begin = compareDate (1, i.from, j.from, k.from);
+                end = compareDate (-1, i.to, j.to, k.to);
+                if (end > begin) {


Очень большая вложенность.

Идет привязка с именам бандитов, что не есть хорошо. Вдруг они изменят состав?

Переменные begin и end инициализированы далеко от места использования, и неясно, начало и конец чего они обозначают.

i, j, k тоже никак себя не объясняют.

chipolinka · 2016-11-04T19:35:56Z

robbery.js

+        for (var i = day1; i <= Math.min(day2, 3); i++) {
+            res.push({
+                from: compareDate (1, new Date (YEAR, MONTH, i, 0, 0), interval.from),
+                to: compareDate (-1, new Date (YEAR, MONTH, i, 23, 59), interval.to)


Привязка к конкретной дате

chipolinka · 2016-11-04T19:36:38Z

robbery.js

+    };
+}
+
+function findFreeIntervals(freeTime, workingHours, res) {


Здесь больше подойдет глагол get. Ты же его возвращаешь?)

chipolinka · 2016-11-04T19:38:35Z

robbery.js

+    return res;
+}
+
+function mainFunction(timeToRobbery, msInDuration) {


Это название функции тоже не годится)

chipolinka · 2016-11-04T19:39:25Z

robbery.js

+function mainFunction(timeToRobbery, msInDuration) {
+    var res = [];
+    var n = timeToRobbery.length;
+    var c = 0;


Однобуквенных переменных вообще быть не должно, только если в крайних случаях это не цикл по индексам.

chipolinka · 2016-11-04T19:40:05Z

robbery.js

+            c++;
+        }
+    }
+    if (c === 0) {


Можно записать в 1 строку тернарными операторами)

chipolinka · 2016-11-04T19:40:47Z

robbery.js

+    if (number < 10) {
+        number = '0' + number;
+    }
+    number = String(number);


Есть метод toString на крайний случай

chipolinka · 2016-11-04T19:41:11Z

robbery.js

-    console.info(schedule, duration, workingHours);
+    var newSchedule = {};
+    var gmt = parseInt (workingHours.from.slice(5), 10);
+    var msInDuration = 60 * 1000 * duration;


Ай-ай-ай, константы!

chipolinka · 2016-11-04T19:43:36Z

Исправлены не все замечания с прошлого раза. Советую тебе перечитать гайд по оформлению кода.

chipolinka · 2016-11-04T19:43:48Z

🍅

honest-hrundel · 2016-11-05T22:00:50Z

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

honest-hrundel · 2016-11-05T22:19:51Z

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

honest-hrundel · 2016-11-05T22:35:50Z

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

chipolinka · 2016-11-07T09:30:46Z

robbery.js

+    var currentDay = WEEK.indexOf(dayOfWeek);
+    var hours = parseInt(date.slice(3, 5), 10);
+    var timeZone = parseInt(date.slice(8), 10);
+    hours = hours - timeZone + bankTimeZone;


Это уже не просто hours

можно же так: hours += bankTimeZone - timeZone

chipolinka · 2016-11-07T09:31:06Z

robbery.js

+function maxDate() {
+    var dates = [].slice.call(arguments);
+    dates.sort(function (date1, date2) {
+


Тут не нужна пустая строка

Почему нельзя воспользоваться функцией Math.max?

у меня не работает Math.max для дат

А если один из этих способов?

О, давай через reduce. Типа пройти по всем датам и найти такую, которая больше других. Очевидно, что sort имеет сложность больше линейной - O(log(n) * n), а reduce будет O(n)

chipolinka · 2016-11-07T09:34:13Z

robbery.js

+    return dates[0];
+}
+
+function findFreeGangsterTime(schedule, bankTimeZone) {


Функция должна занимать не больше 25 строк

Кажется, что можно сначала просчитать все leadToDataType(schedule[key][i].to, bankTimeZone) для каждого i, а потом заполнить ими словарь.

И попробуй использовать reduce

chipolinka · 2016-11-07T09:37:01Z

robbery.js

+
+function getGangFreeTime(schedule) {
+    var freeTime = [];
+    schedule.Danny.forEach(function (interval1) {


Привязка к именам бандитов. И слишком большая вложенность. Такого быть не должно. Нельзя ли просто по ключам пробежаться?

+1

так делать совсем нельзя - а если будет сколько угодно бандитов или имена могу быть другими?

chipolinka · 2016-11-07T09:40:19Z

robbery.js

+}
+
+function formateDate(date) {
+    var day = date.getDate();


Не проще ли сделать так: var day = WEEK[date.getDate()]?
Почему словарь называется WEEK? Почему метод getDate возвращает день?

chipolinka · 2016-11-07T09:40:56Z

robbery.js

+    return res;
+}
+
+function leadDateToObject(date) {


Что значит это название?)

chipolinka · 2016-11-07T09:42:31Z

robbery.js

+exports.isStar = false;
+
+var WEEK = ['', 'ПН', 'ВТ', 'СР', 'ЧТ', 'ПТ', 'СБ', 'ВС'];
+var BEGIN_OF_WEEK = new Date (0, 0, 1, 0, 0);


Ты уверена, что без этих волшебных дат тут не обойтись?

chipolinka · 2016-11-07T09:48:25Z

robbery.js

+}
+
+function formateFreeTime(freeTime) {
+    var res = [];


Лучше названия не сокращать

chipolinka · 2016-11-07T09:49:54Z

robbery.js

+                var begin = maxDate (interval1.from, interval2.from, interval3.from);
+                var end = minDate (interval1.to, interval2.to, interval3.to);
+                if (end > begin) {
+                    freeTime.push({


Ты много где пушишь объект с этими полями. Можно вынести в отдельную функцию

chipolinka · 2016-11-07T09:52:59Z

🚀

Vittly

в целом неплохо, но:

есть косяки в названиях
где-то слишком многословно
совсем не используешь reduce

надо поправить, сорри за задержку - мой косяк не увидел отбивку. В следующий раз пиши в slack если буду тормозить

🍅

Vittly · 2016-11-11T09:55:00Z

robbery.js

+    return freeTime;
+}
+
+function formateFreeTime(freeTime) {


судя по смыслу, аргумент лучше назвать freeIntervals

а почему такое название функции? formatE ? Судя по коду, мы тут интервалы пересекаем с [ПН, СР] и интервалы разбиваем по дням. Это скорее некая нормализация - normalizeIntervals

Vittly · 2016-11-11T09:56:25Z

robbery.js

+        for (var i = day1; i <= Math.min(day2, 3); i++) {
+            res.push({
+                from: maxDate (new Date (0, 0, i, 0, 0), interval.from),
+                to: minDate (new Date (0, 0, i, 23, 59), interval.to)


а что за даты такие? Чтобы стало понятнее, нужно сделать переменные с говорящими именами

мы тут пытаемся разложить interval на отрезки по дням? и в итоге будут теже интервалы разложенные по дням - так? forEach, который собирает некий результат - лучше писать через reduce

Vittly · 2016-11-11T09:58:04Z

robbery.js

+    var res = [];
+    freeTime.forEach(function (interval) {
+        var day1 = interval.from.getDate();
+        var day2 = interval.to.getDate();


хм, возможно, лучше назвать fromDay и toDay.

Vittly · 2016-11-11T09:58:39Z

robbery.js

+    freeTime.forEach(function (interval) {
+        var day1 = interval.from.getDate();
+        var day2 = interval.to.getDate();
+        for (var i = day1; i <= Math.min(day2, 3); i++) {


типа не дальше среды?

Vittly · 2016-11-11T10:03:16Z

robbery.js

+function maxDate() {
+    var dates = [].slice.call(arguments);
+    dates.sort(function (date1, date2) {
+


О, давай через reduce. Типа пройти по всем датам и найти такую, которая больше других. Очевидно, что sort имеет сложность больше линейной - O(log(n) * n), а reduce будет O(n)

Vittly · 2016-11-11T11:01:23Z

robbery.js

+                to: end
+            });
+        }
+    });


опять по смыслу reduce = проход по массиву и получение одного результата

Vittly · 2016-11-11T11:02:16Z

robbery.js

+    var bank = {};
+    freeTime.forEach(function (interval) {
+        var day = interval.from.getDate();
+        bank.start = new Date (0, 0, day, startBankWork.hours, startBankWork.minutes);


ты часто используешь конструкцию: new Date(0, 0, x, y, z) - кмк для нее лучше завести хелпер-функцию с говорящим именем, тогда такие вызовы будут не такими таинственными

Vittly · 2016-11-11T11:03:36Z

robbery.js

+    freeTime.forEach(function (interval) {
+        var day = interval.from.getDate();
+        bank.start = new Date (0, 0, day, startBankWork.hours, startBankWork.minutes);
+        bank.end = new Date (0, 0, day, endBankWork.hours, endBankWork.minutes);


а зачем нам объект? можно было бы сделать две переменные - bankStart и bankEnd

Vittly · 2016-11-11T11:07:19Z

robbery.js

+        if (timeToRobbery[i].to - timeToRobbery[i].from >= msInDuration) {
+            res.push(timeToRobbery[i]);
+        }
+    }


опять reduce нужен, смотри как коротко получается:

var intervals = timeToRobbery.reduce(function(res, interval) { if (interval.to - interval.from >= msInDuration) { res.push(interval); } return res; }, []);

и в конце можно написать тернарник: return intervals.length === 0 ? nul : intervals;

Vittly · 2016-11-11T11:07:36Z

robbery.js

        exists: function () {
-            return false;
+
+            return (timeForRobbery !== null);


можно без скобок

Update robbery.js

99678f0

Update robbery.js

e5d2f85

Update robbery.js

2b51aa0

Update robbery.js

3061156

Update robbery.js

b10d369

Update robbery.js

af12ee9

Update robbery.js

79354b5

Update robbery.js

d4f8f54

Update robbery.js

fdea8b6

Update robbery.js

332cf19

Update robbery.js

146978c

Update robbery.js

51d9e5e

Update robbery.js

a753198

по временной зоне банка

Update robbery.js

22966d1

привела к времени банка

4ef3f09

chipolinka reviewed Nov 4, 2016

View reviewed changes

Update robbery.js

9a9bfed

Update robbery.js

5f83296

Update robbery.js

5d40043

chipolinka reviewed Nov 7, 2016

View reviewed changes

robbery.js

return res;

}

function leadDateToObject(date) {

Copy link

chipolinka Nov 7, 2016

Choose a reason for hiding this comment

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

Что значит это название?)

chipolinka reviewed Nov 7, 2016

View reviewed changes

robbery.js

}

function formateFreeTime(freeTime) {

var res = [];

Copy link

chipolinka Nov 7, 2016

Choose a reason for hiding this comment

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

Лучше названия не сокращать

chipolinka reviewed Nov 7, 2016

View reviewed changes

honest-hrundel assigned Vittly and unassigned chipolinka Nov 7, 2016

Vittly requested changes Nov 11, 2016

View reviewed changes

Conversation

OnaHochetNaMars commented Oct 26, 2016

Uh oh!

honest-hrundel commented Oct 26, 2016

Uh oh!

honest-hrundel commented Oct 26, 2016

Uh oh!

honest-hrundel commented Oct 26, 2016

Uh oh!

honest-hrundel commented Oct 26, 2016

Uh oh!

honest-hrundel commented Oct 27, 2016

Uh oh!

honest-hrundel commented Oct 28, 2016

Uh oh!

honest-hrundel commented Oct 28, 2016

Uh oh!

honest-hrundel commented Oct 28, 2016

Uh oh!

honest-hrundel commented Oct 28, 2016

Uh oh!

honest-hrundel commented Oct 28, 2016

Uh oh!

honest-hrundel commented Oct 28, 2016

Uh oh!

honest-hrundel commented Oct 31, 2016

Uh oh!

honest-hrundel commented Oct 31, 2016

Uh oh!

honest-hrundel commented Oct 31, 2016

Uh oh!

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

chipolinka Nov 4, 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!

chipolinka Nov 4, 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!

chipolinka commented Nov 4, 2016

Uh oh!

chipolinka commented Nov 4, 2016

Uh oh!

honest-hrundel commented Nov 5, 2016

Uh oh!

honest-hrundel commented Nov 5, 2016

Uh oh!

honest-hrundel commented Nov 5, 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

chipolinka Nov 4, 2016 •

edited

Loading

chipolinka Nov 4, 2016 •

edited

Loading