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 30 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
215 changes: 203 additions & 12 deletions robbery.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,189 @@
* Сделано задание на звездочку
* Реализовано оба метода и tryLater
*/
exports.isStar = true;
exports.isStar = false;

var WEEK = ['', 'ПН', 'ВТ', 'СР', 'ЧТ', 'ПТ', 'СБ', 'ВС'];
var YEAR = 2016;
var MONTH = 9;
var BEGIN_OF_WEEK = new Date (YEAR, MONTH, 1, 0, 0);
var END_OF_WEEK = new Date (YEAR, MONTH, 7, 23, 59);

Choose a reason for hiding this comment

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

Вероятно, я не совсем так выразилась в прошлый раз)
В этой задаче к конкретным датам вообще привязываться не надо.


function toTypeDate(date, bankTimeZone) {

Choose a reason for hiding this comment

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

Название функции должно начинаться с глагола. Так оно ответит на вопрос, что функция делает

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) - parseInt (date.slice(8), 10) + bankTimeZone;

Choose a reason for hiding this comment

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

Лучше parseInt(date.slice(8), 10) вынести в отдельную переменную. И да, при вызове функции между её названием и открывающейся скобочкой не должно быть пробелов.

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

return new Date (YEAR, MONTH, currentDay, hours, minutes);
}

function toNewSchedule(schedule, bankTimeZone) {
var newSchedule = {};
var keys = Object.keys(schedule);
keys.forEach(function (key) {
var n = (schedule[key]).length;

Choose a reason for hiding this comment

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

Название не говорит о том, что лежит в этой переменной

if (n === 0) {
newSchedule[key] = [{
from: BEGIN_OF_WEEK,
to: BEGIN_OF_WEEK
}];
} else {
newSchedule[key] = [];
for (var i = 0; i < n; i++) {
newSchedule[key].push({
from: toTypeDate (schedule[key][i].from, bankTimeZone),
to: toTypeDate (schedule[key][i].to, bankTimeZone)
});
}
}
});

return newSchedule;
}

// minOrMax = 1 если нужно найти максимум и
// -1 если нужно найти минимум
function compareDate(minOrMax) {

Choose a reason for hiding this comment

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

Ой, а не проще ли вместо этой функции использовать min и max из Math?

Choose a reason for hiding this comment

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

И функция возвращает не результат сравнения (обычно компараторы возвращают 1, 0 или -1), а саму дату. Название должно соответствовать

var dates = [].slice.call(arguments, 1);
dates.sort (function (date1, date2) {
return (date2 - date1) * minOrMax;
});

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 toFreeSchedule(schedule) {

Choose a reason for hiding this comment

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

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

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

freeSchedule[key] = [];
var n = (schedule[key]).length;
freeSchedule[key][0] = {
from: BEGIN_OF_WEEK,
to: schedule[key][0].from
};
for (var i = 1; i < n; i++) {
freeSchedule[key][i] = {
from: schedule[key][i - 1].to,
to: schedule[key][i].from
};
}
freeSchedule[key][n] = {
from: schedule[key][n - 1].to,
to: END_OF_WEEK
};
freeSchedule[key].len = n + 1;

Choose a reason for hiding this comment

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

Зачем?)

});

return freeSchedule;
}

function findGangFreeTime(schedule) {
var freeTime = [];
var begin;
var end;
schedule.Danny.forEach(function (i) {
schedule.Rusty.forEach(function (j) {
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) {

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: compareDate (1, new Date (YEAR, MONTH, i, 0, 0), interval.from),
to: compareDate (-1, new Date (YEAR, MONTH, i, 23, 59), interval.to)

Choose a reason for hiding this comment

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

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

});
}
});

return res;
}

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

function findFreeIntervals(freeTime, workingHours, res) {

Choose a reason for hiding this comment

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

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

var startBankWork = dateToObject(workingHours.from);
var endBankWork = dateToObject(workingHours.to);
var bank = {};
freeTime.forEach(function (interval) {
var day = interval.from.getDate();
bank.start = new Date (YEAR, MONTH, day, startBankWork.hours, startBankWork.minutes);
bank.end = new Date (YEAR, MONTH, day, endBankWork.hours, endBankWork.minutes);
var begin = compareDate (1, interval.from, bank.start);
var end = compareDate (-1, 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 mainFunction(timeToRobbery, msInDuration) {
Copy link

@chipolinka chipolinka Nov 4, 2016

Choose a reason for hiding this comment

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

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

var res = [];
var n = timeToRobbery.length;
var c = 0;

Choose a reason for hiding this comment

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

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

for (var i = 0; i < n; i++) {
var a = (timeToRobbery[i].to - timeToRobbery[i].from);
if (a >= msInDuration) {
res[c] = {};
res[c] = timeToRobbery[i];
c++;
}
}
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 (c === 0) {
Copy link

@chipolinka chipolinka Nov 4, 2016

Choose a reason for hiding this comment

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

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

return null;
}

return res; // время для ограбления
}

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

Choose a reason for hiding this comment

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

Есть метод 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 +196,16 @@ exports.isStar = true;
* @param {String} workingHours.to – Время закрытия, например, "18:00+5"
* @returns {Object}
*/

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

Choose a reason for hiding this comment

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

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

newSchedule = toNewSchedule (schedule, gmt);
var freeSchedule = toFreeSchedule (newSchedule);
var freeTime = findGangFreeTime (freeSchedule);
var timeForRobbery = findFreeIntervals (freeTime, workingHours, []);
timeForRobbery = mainFunction (timeForRobbery, msInDuration);

return {

Expand All @@ -24,7 +214,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 +226,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

}
};
};