Левшин Михаил 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

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


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

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

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

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


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

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

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

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

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


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

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

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


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

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

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


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

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

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


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

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

+    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

+    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

+    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

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


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

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

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


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

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

+            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

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


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

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

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


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

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

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

        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

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


лучше isTimeInIntervals

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

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

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

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


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

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

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

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

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


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

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

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

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


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

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

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

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

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


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

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

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

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


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

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

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


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

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

+    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

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


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

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

+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

 exports.isStar = true;

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


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

мешает

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

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

+    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

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


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

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

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


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

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

+    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

 exports.isStar = true;

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


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

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

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


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

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

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


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

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

+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

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


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

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

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


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

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

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


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

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

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

+    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

Comment thread robbery.js Outdated

}

}

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

Copy link
Copy Markdown

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

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

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