Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Козлова Валерия #57

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
99678f0
Update robbery.js
OnaHochetNaMars Oct 26, 2016
e5d2f85
Update robbery.js
OnaHochetNaMars Oct 26, 2016
2b51aa0
Update robbery.js
OnaHochetNaMars Oct 26, 2016
3061156
Update robbery.js
OnaHochetNaMars Oct 26, 2016
b10d369
Update robbery.js
OnaHochetNaMars Oct 27, 2016
af12ee9
Update robbery.js
OnaHochetNaMars Oct 28, 2016
79354b5
Update robbery.js
OnaHochetNaMars Oct 28, 2016
d4f8f54
Update robbery.js
OnaHochetNaMars Oct 28, 2016
fdea8b6
Update robbery.js
OnaHochetNaMars Oct 28, 2016
332cf19
Update robbery.js
OnaHochetNaMars Oct 28, 2016
146978c
Update robbery.js
OnaHochetNaMars Oct 28, 2016
51d9e5e
Update robbery.js
OnaHochetNaMars Oct 31, 2016
a753198
Update robbery.js
OnaHochetNaMars Oct 31, 2016
22966d1
Update robbery.js
OnaHochetNaMars Oct 31, 2016
4ef3f09
привела к времени банка
OnaHochetNaMars Oct 31, 2016
a6ffd79
Update robbery.js
OnaHochetNaMars Oct 31, 2016
0784f5d
проверка на правильность порядка дат
OnaHochetNaMars Oct 31, 2016
fb0caa2
Update robbery.js
OnaHochetNaMars Oct 31, 2016
db69cec
исправления по линтингу
OnaHochetNaMars Oct 31, 2016
d019062
Update robbery.js
OnaHochetNaMars Oct 31, 2016
a53fafd
Update index.js
OnaHochetNaMars Nov 1, 2016
a4a8844
Update index.js
OnaHochetNaMars Nov 1, 2016
4faba5e
Update robbery.js
OnaHochetNaMars Nov 1, 2016
123f6c2
Update robbery.js
OnaHochetNaMars Nov 1, 2016
8d4a634
исправлен линтинг, добавлено разделение по дням
OnaHochetNaMars Nov 1, 2016
f06c1c8
Update robbery.js
OnaHochetNaMars Nov 1, 2016
18aaff8
Update robbery.js
OnaHochetNaMars Nov 1, 2016
42465df
переписаны функции
OnaHochetNaMars Nov 4, 2016
2df9932
линтинг
OnaHochetNaMars Nov 4, 2016
1712fe0
Update robbery.js
OnaHochetNaMars Nov 4, 2016
9a9bfed
Update robbery.js
OnaHochetNaMars Nov 5, 2016
5f83296
Update robbery.js
OnaHochetNaMars Nov 5, 2016
5d40043
Update robbery.js
OnaHochetNaMars Nov 5, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
198 changes: 186 additions & 12 deletions robbery.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,174 @@
* Сделано задание на звездочку
* Реализовано оба метода и tryLater
*/
exports.isStar = true;
exports.isStar = false;

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

Choose a reason for hiding this comment

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

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

var END_OF_WEEK = new Date (0, 0, 7, 23, 59);
var msInMinute = 60000;
Copy link

Choose a reason for hiding this comment

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

константы лучше писать в виде (как у тебя выше): MS_IN_MINUTE


function leadToDataType(date, bankTimeZone) {
Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

я бы назвал convertDateToZone

var dayOfWeek = date.slice(0, 2);
Copy link

Choose a reason for hiding this comment

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

лучше бы все эти даты строковые в объекты перегонять функцией-хелпером, чтобы потом не slice-ы с магическими константами писать, а писать что-то со смыслом - parse(date).day и все

var currentDay = WEEK.indexOf(dayOfWeek);
var hours = parseInt(date.slice(3, 5), 10);
Copy link

Choose a reason for hiding this comment

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

во-во и тут slice

var timeZone = parseInt(date.slice(8), 10);
hours = hours - timeZone + bankTimeZone;

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

var minutes = parseInt(date.slice(6, 8), 10);
Copy link

Choose a reason for hiding this comment

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

магия


return new Date (0, 0, currentDay, hours, minutes);
}

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

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

return (date2 - date1);
});

return dates[0];
}

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

return (date1 - date2);
});

return dates[0];
Copy link

Choose a reason for hiding this comment

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

и тут давай сделаем reduce

}

function findFreeGangsterTime(schedule, bankTimeZone) {

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

var freeSchedule = {};
var keys = Object.keys(schedule);
Copy link

Choose a reason for hiding this comment

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

зачем keys? Можно было так:

Object.keys(schedule).forEach(/* ... */);

keys.forEach(function (key) {
Copy link

Choose a reason for hiding this comment

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

key -> gangsterName

var length = (schedule[key]).length;
Copy link

Choose a reason for hiding this comment

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

скобки не нужны

freeSchedule[key] = [];
if (length === 0) {
freeSchedule[key] = [{
Copy link

Choose a reason for hiding this comment

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

а чего не push? А то массив вон выше просто выкинется

from: BEGIN_OF_WEEK,
to: END_OF_WEEK
}];
} else {
freeSchedule[key].push({
from: BEGIN_OF_WEEK,
to: leadToDataType (schedule[key][0].from, bankTimeZone)
});
for (var i = 1; i < length; i++) {
freeSchedule[key].push({
from: leadToDataType (schedule[key][i - 1].to, bankTimeZone),
to: leadToDataType (schedule[key][i].from, bankTimeZone)
});
}
freeSchedule[key].push({
from: leadToDataType (schedule[key][length - 1].to, bankTimeZone),
to: END_OF_WEEK
});
Copy link

Choose a reason for hiding this comment

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

все как бы правильно, но очень многословно. Задача верная да, нужно инвертировать интервалы. Как тебе такой алгоритм:

  • left = BEGIN_OF_WEEK
  • сортируем интервалы грабителей (или где-то сказано, что они по возрастанию?)
  • в цикле по schedule[key] (который, кстати, можно положить в переменную)
    • добавляем в freeSchedule [left, начало-i-ого-интервала]
    • left = конец-i-ого-интервала
  • после цикла добавляем [left, END_OF_WEEK]

Copy link

Choose a reason for hiding this comment

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

нигде не наврал?

}
});

return freeSchedule;
}

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

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

+1

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

schedule.Rusty.forEach(function (interval2) {
schedule.Linus.forEach(function (interval3) {
var begin = maxDate (interval1.from, interval2.from, interval3.from);
var end = minDate (interval1.to, interval2.to, interval3.to);
if (end > begin) {

Choose a reason for hiding this comment

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

  • Очень большая вложенность.
  • Идет привязка с именам бандитов, что не есть хорошо. Вдруг они изменят состав?
  • Переменные begin и end инициализированы далеко от места использования, и неясно, начало и конец чего они обозначают.
  • i, j, k тоже никак себя не объясняют.

freeTime.push({

Choose a reason for hiding this comment

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

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

from: begin,
to: end
});
}
});
});
});
Copy link

Choose a reason for hiding this comment

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

чисто математически алгоритм верный - да, надо получить пересечение "свободных" отрезков всех бандитов

freeTime = formateFreeTime (freeTime);

return freeTime;
}

function formateFreeTime(freeTime) {
Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

var res = [];

Choose a reason for hiding this comment

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

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

freeTime.forEach(function (interval) {
var day1 = interval.from.getDate();
var day2 = interval.to.getDate();
Copy link

Choose a reason for hiding this comment

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

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

for (var i = day1; i <= Math.min(day2, 3); i++) {
Copy link

Choose a reason for hiding this comment

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

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

res.push({
from: maxDate (new Date (0, 0, i, 0, 0), interval.from),
to: minDate (new Date (0, 0, i, 23, 59), interval.to)
Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

});
}
});

return res;
}

function leadDateToObject(date) {

Choose a reason for hiding this comment

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

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

return {
hours: parseInt(date.slice(0, 2), 10),
minutes: parseInt(date.slice(3, 5), 10)
};
}

function getFreeIntervals(freeTime, workingHours, res) {
var startBankWork = leadDateToObject(workingHours.from);
Copy link

Choose a reason for hiding this comment

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

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

var endBankWork = leadDateToObject(workingHours.to);
var bank = {};
freeTime.forEach(function (interval) {
var day = interval.from.getDate();
bank.start = new Date (0, 0, day, startBankWork.hours, startBankWork.minutes);
Copy link

Choose a reason for hiding this comment

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

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

bank.end = new Date (0, 0, day, endBankWork.hours, endBankWork.minutes);
Copy link

Choose a reason for hiding this comment

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

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

var begin = maxDate (interval.from, bank.start);
var end = minDate (interval.to, bank.end);
if (end > begin) {
res.push({
from: begin,
to: end
});
}
});
Copy link

Choose a reason for hiding this comment

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

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


return res;
}

function getTimeForRobbery(timeToRobbery, msInDuration) {
var res = [];
for (var i = 0; i < timeToRobbery.length; i++) {
if (timeToRobbery[i].to - timeToRobbery[i].from >= msInDuration) {
res.push(timeToRobbery[i]);
}
}
Copy link

Choose a reason for hiding this comment

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

опять 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;

if (res.length === 0) {
return null;
}

return res;
}

function toString(number) {
if (number < 10) {
number = '0' + number;
}
number.toString();

return number;
Copy link

Choose a reason for hiding this comment

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

лучше короче: return number < 10 ? '0' + number : number.toString();. И название не подходящее, потому что toString - слишком общее, оно только числа (и то не все < 100) превращает в строки и только одним способом - добавляя спереди ноль, если число с одним знаком. Такое можно называть formatDateNumber()

}

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

Choose a reason for hiding this comment

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

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

var hours = date.getHours();
var minutes = date.getMinutes();
day = WEEK[day];
hours = toString (hours);
minutes = toString(minutes);

return [day, hours, minutes];
Copy link

Choose a reason for hiding this comment

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

Нехорошо писать так:

var x = rawData;

x = f(x);
x = g(x);

return [..., x, ...];

Типа перекладывать в одну и туже переменную разное содержимое. Сначала часы у тебя просто число, потом форматированная строка. Тут можно переделать так:

var day = date.getDate();

// ...

return [WEEK[day], ...];

}

/**
* @param {Object} schedule – Расписание Банды
Expand All @@ -14,8 +181,14 @@ exports.isStar = true;
* @param {String} workingHours.to – Время закрытия, например, "18:00+5"
* @returns {Object}
*/

exports.getAppropriateMoment = function (schedule, duration, workingHours) {
console.info(schedule, duration, workingHours);
var gmt = parseInt(workingHours.from.slice(5), 10);
Copy link

Choose a reason for hiding this comment

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

ого, а что такое вернет workingHours.from.slice(5) ?

Copy link

Choose a reason for hiding this comment

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

неочевидно совсем, что он вернет сдвиг - выше я писал, что помог бы хелпер

var msInDuration = msInMinute * duration;
var freeSchedule = findFreeGangsterTime (schedule, gmt);
var freeTime = getGangFreeTime (freeSchedule);
var timeForRobbery = getFreeIntervals (freeTime, workingHours, []);
timeForRobbery = getTimeForRobbery (timeForRobbery, msInDuration);
Copy link

Choose a reason for hiding this comment

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

уже писал почему это плохой паттерн - переменная одна, а в разное время в ней лежит разное


return {

Expand All @@ -24,7 +197,8 @@ exports.getAppropriateMoment = function (schedule, duration, workingHours) {
* @returns {Boolean}
*/
exists: function () {
return false;

return (timeForRobbery !== null);
Copy link

Choose a reason for hiding this comment

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

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

},

/**
Expand All @@ -35,16 +209,16 @@ exports.getAppropriateMoment = function (schedule, duration, workingHours) {
* @returns {String}
*/
format: function (template) {
return template;
},
if (!this.exists()) {
return '';
}
var format = formateDate(timeForRobbery[0].from);
var newTemplate = template
.replace('%DD', format[0])
.replace('%HH', format[1])
.replace('%MM', format[2]);

/**
* Попробовать найти часы для ограбления позже [*]
* @star
* @returns {Boolean}
*/
tryLater: function () {
return false;
return newTemplate;
Copy link

Choose a reason for hiding this comment

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

переменная newTemplate не нужна, можно сразу return делать. Неприкольно, что formateDate возвращает массив - нам приходитя помнить, что там на каком месте лежит. Можно переделать несколькими способами. Например, возвращать объект с полями hours, minutes ... или DD, HH

}
};
};