Левшин Михаил by LevshinMisha · Pull Request #18 · urfu-2016/javascript-task-3

LevshinMisha · 2016-10-23T10:35:36Z

No description provided.

honest-hrundel · 2016-10-23T10:35:58Z

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

honest-hrundel · 2016-10-23T10:35:59Z

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

chipolinka · 2016-10-24T16:15:01Z

robbery.js

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


это константа? посмотри, как в гайде рекомендуют называть константы.

Да Это по сути константа. Исправлю

chipolinka · 2016-10-24T16:15:45Z

robbery.js

- * @returns {Object}
- */
+var daysDict = { 'ПН': 0, 'ВТ': 1, 'СР': 2 };
+var daysArray = ['ПН', 'ВТ', 'СР'];


зачем тебе этот массив? можно просто взять ключи словаря)

Для читаемости кода. daysDict.keys()[i] хуже чем daysArray[i]

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

chipolinka · 2016-10-24T16:16:35Z

robbery.js

+    };
+
+    this.getNextDay = function () {
+        return new TimeInterval(this.start + 24 * 60, this.end + 24 * 60);


числа нужно вынести в константы

chipolinka · 2016-10-24T16:18:13Z

robbery.js

+        return new TimeInterval(this.start + 24 * 60, this.end + 24 * 60);
+    };
+
+    this.timeInInterval = function (time) {


в названии функции надо указывать, что она делает (то бишь глагол)

chipolinka · 2016-10-24T16:18:40Z

robbery.js

+    };
+}
+
+function timeInIntervals(timeIntervalsArray, time) {


аналогично с timeInInterval

chipolinka · 2016-10-24T16:19:43Z

robbery.js

+    for (var i = 0; i < timeIntervalsArray.length; i++) {
+        if (timeIntervalsArray[i].timeInInterval(time)) {
+
+            return true;


а зачем тут отступ перед return?)

chipolinka · 2016-10-24T16:21:36Z

robbery.js

+    var day = 0;
+    if (daysDict[str.slice(0, 2)] !== undefined) {
+        day = daysDict[str.split(' ')[0]];
+        str = str.split(' ')[1];


str.split(' ') наверное стоит вынести в переменную, раз часто используется

chipolinka · 2016-10-24T16:22:13Z

robbery.js

+    var h = parseInt(str.split(':')[0]);
+    var m = parseInt(str.split(':')[1]);
+
+    return day * 24 * 60 + (h - timezone) * 60 + m;


константы

chipolinka · 2016-10-24T16:23:19Z

robbery.js

+    var timezone = parseInt(str.split('+')[1]);
+    str = str.split('+')[0];
+    var h = parseInt(str.split(':')[0]);
+    var m = parseInt(str.split(':')[1]);


по-моему, это не самое подходящее название переменной. мало ли чего там m может быть?) да и h тоже

chipolinka · 2016-10-24T16:25:19Z

robbery.js

+        });
+    });
+    newSchedule.Bank.push(new TimeInterval(strToIntDate(workingHours.from) +
+        bankTimezone * 60, strToIntDate(workingHours.to) + bankTimezone * 60));


кажется, тут есть дублирование... и константы, конечно.

chipolinka · 2016-10-24T16:25:59Z

robbery.js

+                }
+            }
+
+            for (; i < 3 * 24 * 60; i++) {


ой, как много констант

chipolinka · 2016-10-24T16:27:24Z

robbery.js

+            var i = 0;
+            function gangsterNotBusy(gangster) {
+                if (timeInIntervals(newSchedule[gangster], i)) {
+                    goodTimeToHack = false;


goodTimeToHack = !timeInIntervals(newSchedule[gangster], i) ?

В случае если время неподходит первому, но подходит второму и третьему, то значение переменной будет true. Что не совсем то что нужно.

chipolinka · 2016-10-24T16:29:24Z

robbery.js

+
+            var goodTimeToHack = true;
+            var i = 0;
+            function gangsterNotBusy(gangster) {


опять имя функции не по канону

chipolinka · 2016-10-24T16:31:45Z

robbery.js

+    function simplifyTimesIntervals() {
+        var newTimeIntervals = [];
+        goodTimesIntervals.forEach(function (timeInterval) {
+            if (!(timeInterval.getLength() < duration)) {


просто интересно: а почему не >=? а именно отрицание от <?

Потому что я могу)

chipolinka · 2016-10-24T16:35:15Z

robbery.js

        tryLater: function () {
+            if (this.exists()) {
+                var startInterval = goodTimesIntervals[0];
+                if (goodTimesIntervals.length > 1 || startInterval.getLength() >= duration + 30) {


кажется, что это можно переписать в один if, ну или как-то более сокращенно

chipolinka · 2016-10-24T16:50:34Z

везде есть константы, которые надо вынести/переименовать.

chipolinka · 2016-10-24T16:51:31Z

🍅

honest-hrundel · 2016-10-24T18:23:37Z

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

honest-hrundel · 2016-10-24T18:25:13Z

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

honest-hrundel · 2016-10-24T18:25:14Z

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

chipolinka · 2016-10-24T18:49:27Z

robbery.js

+    };
+}
+
+function timeIsInIntervals(timeIntervalsArray, time) {


лучше isTimeInIntervals

и аналогично во всех остальных местах)

chipolinka · 2016-10-24T19:06:55Z

robbery.js

+    });
+    newSchedule.Bank.push(new TimeInterval(
+        strToIntDate(workingHours.from) + bankTimezone * MINUTES_IN_HOUR,
+        strToIntDate(workingHours.to) + bankTimezone * MINUTES_IN_HOUR));


меня смущает, что у тебя 4 раза встречается строка
strToIntDate(something) + bankTimezone * MINUTES_IN_HOUR
это никак не сделать поизящнее?
имею в виду DRY

chipolinka · 2016-10-24T19:11:07Z

robbery.js

+
+        function createGoodTimesIntArray() {
+
+            var goodTimeToHack = true;


иногда отступы всплывают в самых неожиданных местах. не рекомендую их ставить после фигурных скобочек (не только здесь, но и во всём коде). тут же нечего отделять логически?

а ещё название этой переменной предполагает, что в ней хранится время.

chipolinka · 2016-10-24T19:13:10Z

robbery.js

+
+            var goodTimeToHack = true;
+            var i = 0;
+            function gangsterIsNotBusy(gangster) {


название функции следует начинать с глагола

honest-hrundel · 2016-10-27T10:52:26Z

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

honest-hrundel · 2016-10-27T10:59:29Z

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

honest-hrundel · 2016-10-27T10:59:29Z

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

chipolinka · 2016-10-28T04:09:33Z

robbery.js

+    var i = -1;
+    function isGangsterNotBusy(gangster) {
+        if (isTimeInIntervals(schedule[gangster], i)) {
+            isGoodTimeToHack = false;


можно сделать isGoodTimeToHack = !isTimeInIntervals(schedule[gangster], i)

Сделать, то можно. Только это не совсем правильно(я уже писал почему). Даже базовые тесты перестают проходить.

chipolinka · 2016-10-28T04:11:03Z

robbery.js

+        }
+    }
+
+    while (i < DAYS_TO_HACK * MINUTES_IN_DAY) {


В данном while у тебя i не изменяется, кроме как увеличивается при каждой итерации? Почему бы тогда не сделать это циклом?

функция isGangsterNotBusy использует внешние переменные, одной из которой является переменная i. А так как функции объявлять внутри цикла нельзя(lint), то следовательно нужно объявить эту переменную и функцию ее использующую заранее. Я вижу выход из этой ситуации созданием функции, которая возвращает другую функцию. Попробую это сделать.

исправил даже изящнее чем сам предполагал)

chipolinka · 2016-10-28T04:13:34Z

robbery.js

+        if (!isTimeInIntervals(schedule.Bank, i)) {
+            continue;
+        }
+        gangsterNames.forEach(isGangsterNotBusy);


А зачем ты это вынес в отдельную функцию? У тебя она вынесена далеко от этого места, да и содержит в себе 1 строчку...

В lint'е стоит ограничение, что нельзя создавать функции внутри цикла

chipolinka · 2016-10-28T04:14:48Z

robbery.js

+
+function createScheduleWithTimeIntervals(bankWorkingHours, schedule, gangsterNames) {
+    var newSchedule = {};
+    var bankShift = parseInt(bankWorkingHours.from.split('+')[1], 10) * MINUTES_IN_HOUR;


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

chipolinka · 2016-10-28T04:19:11Z

robbery.js

+    return goodTimesIntervals;
+}
+
+function createScheduleWithTimeIntervals(bankWorkingHours, schedule, gangsterNames) {


Эту функцию можно разбить на наполнение нового расписания гангстерами и банком. И объединить результат как раз в этой функции.

chipolinka · 2016-10-28T04:20:52Z

robbery.js

+    var gangsterNames = getGangsterNames(schedule);
+    var newSchedule = createScheduleWithTimeIntervals(workingHours, schedule, gangsterNames);
+    var goodTimesIntervals = getGoodTimeIntervals(newSchedule, gangsterNames);
+    goodTimesIntervals = simplifyTimesIntervals(goodTimesIntervals, duration);


Лучше создай новую переменную с соответствующим названием. А то не ясно, чем результат этой функции отличается от предыдущей. А так -- мне нравится организация кода 👍

chipolinka · 2016-10-28T04:21:20Z

robbery.js

-            return template;
+            if (!this.exists()) {
+
+                return '';


Кажется, тут лишний перенос строки

chipolinka · 2016-10-28T04:28:05Z

robbery.js

+function getGangsterNames(schedule) {
+    var gangsterNames = [];
+    for (var gangsterName in schedule) {
+        if (schedule.hasOwnProperty(gangsterName)) {


Попробуй написать это как-то по-другому. Попроще :)

Ну если честно я не знаю как. https://learn.javascript.ru/object-for-in смотрел тут. hasOwnProperty для lint'a

https://urfu-2016.github.io/javascript-slides/03-types/#/19

chipolinka · 2016-10-28T04:54:30Z

robbery.js

 exports.isStar = true;

-/**
- * @param {Object} schedule – Расписание Банды


Почему удалил доки?

мешает

Но они же несут полезную информацию!

chipolinka · 2016-10-28T04:55:48Z

robbery.js

+    return false;
+}
+
+function strDateToInt(strDate) {


Если честно, на первый взгляд тут адище происходит. Слайсы не подойдут?

chipolinka · 2016-10-28T05:03:15Z

🍅

honest-hrundel · 2016-10-28T10:58:05Z

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

honest-hrundel · 2016-10-28T10:58:05Z

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

chipolinka · 2016-10-28T11:36:28Z

robbery.js

+    if (DAYS_INDEXES[strDateCopy.slice(0, 2)] === undefined) {
+        strDateCopy = 'ПН ' + strDateCopy;
+    }
+    var o = {


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

chipolinka · 2016-10-28T11:38:07Z

robbery.js

+
+function addOrChangeLastTimeInterval(arrayIntervals, timeToStartNewInterval) {
+    if (arrayIntervals.length === 0 ||
+        arrayIntervals[arrayIntervals.length - 1].end !== timeToStartNewInterval - 1) {


В этой функции слишком часто используется arrayIntervals.length, лучше вынеси в отдельную переменную. Да и название покороче будет)

chipolinka · 2016-10-28T11:40:41Z

robbery.js

+    var gangstersIsNotBusy = true;
+    gangsterNames.forEach(function (gangster) {
+        if (isTimeInIntervals(schedule[gangster], time)) {
+            gangstersIsNotBusy = false;


Можно написать здесь return false;, а в конце функции -- return true;. Тогда без этой переменной обойдемся и меньше бегать по циклу будем)

К сожалению из forEach нельзя ничего возвращать(

chipolinka · 2016-10-28T11:46:37Z

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

chipolinka · 2016-10-28T11:46:50Z

🚀

honest-hrundel · 2016-10-28T12:55:56Z

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

honest-hrundel · 2016-10-28T12:55:56Z

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

mokhov

В данном решении очень много избыточного кода. Из-за этого его очень сложно читать. Хочу в ответном комментарии увидеть ответ на вопрос: как работает твой алгоритм?

mokhov · 2016-10-30T17:46:04Z

robbery.js

 exports.isStar = true;

+var DAYS_INDEXES = { 'ПН': 0, 'ВТ': 1, 'СР': 2, 'ЧТ': 3, 'ПТ': 4, 'СБ': 5, 'ВС': 6 };
+var DAYS_NAMES = ['ПН', 'ВТ', 'СР', 'ЧТ', 'ПТ', 'СБ', 'ВС'];


Это ключи предыдущего объекта

mokhov · 2016-10-30T17:48:23Z

robbery.js

+    };
+}
+
+function isTimeInIntervals(timeIntervalsArray, time) {


Название читаю, а понять что она делает не могу

mokhov · 2016-10-30T17:49:19Z

robbery.js

+}
+
+function strDateToDateObj(strDate) {
+    var strDateCopy = strDate.slice();


Зачем это нужно?

mokhov · 2016-10-30T17:52:52Z

robbery.js

+function getGangsterNames(schedule) {
+    var gangsterNames = [];
+    for (var gangsterName in schedule) {
+        if (schedule.hasOwnProperty(gangsterName)) {


https://urfu-2016.github.io/javascript-slides/03-types/#/19

mokhov · 2016-10-30T17:59:29Z

robbery.js

+    this.start = start;
+    this.end = end;
+
+    this.getLength = function () {


start и end известны на этапе создания объекта, почему length нужно вычислять, а нельзя сделать полем?

mokhov · 2016-10-30T18:02:27Z

robbery.js

+}
+
+function createTimeIntervalWithShift(time, shift) {
+    return new TimeInterval(strDateToDateObj(time.from).intValue + shift,


Это нечитаемый однострочник, нужно вынести аргументы TimeIntreval в читаемые переменные

mokhov · 2016-10-30T18:03:14Z

robbery.js

+        strDateToDateObj(time.to).intValue + shift);
+}
+
+function simplifyTimesIntervals(timeIntervals, duration) {


Название функции переводится как «Упростить временные интервалы». Я честно не понимаю что тут подразумевается

mokhov · 2016-10-30T18:04:13Z

robbery.js

+
+function simplifyTimesIntervals(timeIntervals, duration) {
+    var newTimeIntervals = [];
+    timeIntervals.forEach(function (timeInterval) {


Для фильтрации массивов придумали специальную функцию https://developer.mozilla.org/ru/docs/Web/JavaScript/Reference/Global_Objects/Array/filter

mokhov · 2016-10-30T18:05:24Z

robbery.js

+    return newTimeIntervals;
+}
+
+function addOrChangeLastTimeInterval(arrayIntervals, timeToStartNewInterval) {


Из названия функции абсолютно не понятно что она делает. И по коду я тоже ничего не понял

mokhov · 2016-10-30T18:08:24Z

🍅

Первый коммит

4aab65b

honest-hrundel assigned chipolinka Oct 23, 2016

chipolinka reviewed Oct 24, 2016

View reviewed changes

robbery.js Outdated

}

}

for (; i < 3 * 24 * 60; i++) {

Copy link

chipolinka Oct 24, 2016

Choose a reason for hiding this comment

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

ой, как много констант

chipolinka reviewed Oct 24, 2016

View reviewed changes

+Константы, +глаголы-функции, -костыли

82eda4e

К слову о костылях

c5e2d5a

chipolinka reviewed Oct 24, 2016

View reviewed changes

Систему счисления всем parseInt'ам за мой счет!

ad30429

chipolinka reviewed Oct 28, 2016

View reviewed changes

robbery.js

return template;

if (!this.exists()) {

return '';

Copy link

chipolinka Oct 28, 2016

Choose a reason for hiding this comment

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

Кажется, тут лишний перенос строки

chipolinka reviewed Oct 28, 2016

View reviewed changes

все что смог сделал

73a3ba5

chipolinka reviewed Oct 28, 2016

View reviewed changes

honest-hrundel assigned mokhov and unassigned chipolinka Oct 28, 2016

Вернул комментарии, переименовал некоторые переменные

652f8e8

mokhov reviewed Oct 30, 2016

View reviewed changes


		function createGoodTimesIntArray() {

		var goodTimeToHack = true;

Conversation

LevshinMisha commented Oct 23, 2016

Uh oh!

honest-hrundel commented Oct 23, 2016

Uh oh!

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

chipolinka Oct 24, 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!

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!

chipolinka commented Oct 24, 2016

Uh oh!

chipolinka commented Oct 24, 2016

Uh oh!

honest-hrundel commented Oct 24, 2016

Uh oh!

honest-hrundel commented Oct 24, 2016

Uh oh!

honest-hrundel commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chipolinka Oct 24, 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!

honest-hrundel commented Oct 27, 2016

Uh oh!

honest-hrundel commented Oct 27, 2016

Uh oh!

honest-hrundel commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

chipolinka Oct 24, 2016 •

edited

Loading

chipolinka Oct 24, 2016 •

edited

Loading

chipolinka Oct 28, 2016 •

edited

Loading