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

Левшин Михаил #18

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
191 changes: 161 additions & 30 deletions robbery.js
Original file line number Diff line number Diff line change
@@ -1,49 +1,180 @@
'use strict';

/**
* Сделано задание на звездочку
* Реализовано оба метода и tryLater
*/
exports.isStar = true;

/**
* @param {Object} schedule – Расписание Банды
* @param {Number} duration - Время на ограбление в минутах
* @param {Object} workingHours – Время работы банка
* @param {String} workingHours.from – Время открытия, например, "10:00+5"
* @param {String} workingHours.to – Время закрытия, например, "18:00+5"
* @returns {Object}
*/
var daysDict = { 'ПН': 0, 'ВТ': 1, 'СР': 2 };

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

var daysArray = ['ПН', 'ВТ', 'СР'];

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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


function TimeInterval(start, end) {
this.start = start;
this.end = end;

Choose a reason for hiding this comment

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

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

Copy link
Author

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.

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


this.getLength = function () {
Copy link

Choose a reason for hiding this comment

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

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

return this.end - this.start;
};

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

Choose a reason for hiding this comment

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

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

};

this.timeInInterval = function (time) {

Choose a reason for hiding this comment

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

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

return this.start <= time && time <= this.end;
};
}

function timeInIntervals(timeIntervalsArray, time) {
Copy link

@chipolinka chipolinka Oct 24, 2016

Choose a reason for hiding this comment

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

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

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

return true;

Choose a reason for hiding this comment

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

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

}
}

return false;
}

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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


return day * 24 * 60 + (h - timezone) * 60 + m;

Choose a reason for hiding this comment

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

константы

}

exports.getAppropriateMoment = function (schedule, duration, workingHours) {

Choose a reason for hiding this comment

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

ой, эта функция больше 100 строчек занимает. постарайся декомпозировать.

Choose a reason for hiding this comment

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

вообще лучше сделать так, чтобы функции занимали не больше 25 строк. так будет более читаемо и понятно. и меньше шанс допустить ошибку)

console.info(schedule, duration, workingHours);
var newSchedule = {
Danny: [],
Rusty: [],
Linus: [],
Bank: []
};

var bankTimezone = parseInt(workingHours.from.split('+')[1]);

var gangsterNames = ['Danny', 'Rusty', 'Linus'];

Choose a reason for hiding this comment

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

опять дублирование ключей словаря. а что ты будешь делать, если компаньоны соберутся поменять состав?


gangsterNames.forEach(function (gangsterName) {
schedule[gangsterName].forEach(function (time) {
newSchedule[gangsterName].push(new TimeInterval(strToIntDate(time.from) + 1 +
bankTimezone * 60, strToIntDate(time.to) - 1 + bankTimezone * 60));
});
});
newSchedule.Bank.push(new TimeInterval(strToIntDate(workingHours.from) +
bankTimezone * 60, strToIntDate(workingHours.to) + bankTimezone * 60));

Choose a reason for hiding this comment

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

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

newSchedule.Bank.push(newSchedule.Bank[0].getNextDay());
newSchedule.Bank.push(newSchedule.Bank[1].getNextDay());

function getGoodTimeIntervals() {

Choose a reason for hiding this comment

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

какая-то больно сложная функция получилась. не лучше ли вынести из неё мелкие функции наружу?

var goodTimesInt = [];
function createGoodTimesIntArray() {

var goodTimeToHack = true;

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.

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

var i = 0;
function gangsterNotBusy(gangster) {

Choose a reason for hiding this comment

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

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

if (timeInIntervals(newSchedule[gangster], i)) {
goodTimeToHack = false;

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

}
}

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

Choose a reason for hiding this comment

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

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

goodTimeToHack = true;

if (!timeInIntervals(newSchedule.Bank, i)) {
continue;
}

gangsterNames.forEach(gangsterNotBusy);

if (goodTimeToHack) {
goodTimesInt.push(i);
}
}
}

createGoodTimesIntArray();

var goodTimesIntervals = [];

for (var i = 0; i < goodTimesInt.length; i++) {
if (i === 0 || goodTimesInt[i - 1] + 1 !== goodTimesInt[i]) {
goodTimesIntervals.push(new TimeInterval(goodTimesInt[i], goodTimesInt[i]));
} else {
goodTimesIntervals[goodTimesIntervals.length - 1].end += 1;
}
}

return goodTimesIntervals;
}

var goodTimesIntervals = getGoodTimeIntervals();

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

newTimeIntervals.push(timeInterval);
}
});
goodTimesIntervals = newTimeIntervals;
}

simplifyTimesIntervals();

return {

/**
* Найдено ли время
* @returns {Boolean}
*/
exists: function () {
for (var i = 0; i < goodTimesIntervals.length; i++) {
if (goodTimesIntervals[i].getLength() >= duration) {

return true;
}
}

return false;
},

/**
* Возвращает отформатированную строку с часами для ограбления
* Например,
* "Начинаем в %HH:%MM (%DD)" -> "Начинаем в 14:59 (СР)"
* @param {String} template
* @returns {String}
*/
format: function (template) {
return template;
if (!this.exists()) {

return '';

Choose a reason for hiding this comment

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

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

}

function timeToString(time) {
if (time < 10) {

return '0' + time.toString();
}

return time.toString();
}
var startInterval = goodTimesIntervals[0];
var day = parseInt(startInterval.start / (24 * 60));
var hour = parseInt(startInterval.start / 60) - day * 24;
var minutes = startInterval.start % 60;

return template.replace('%DD', daysArray[day]).replace('%HH', timeToString(hour))
.replace('%MM', timeToString(minutes));
},

/**
* Попробовать найти часы для ограбления позже [*]
* @star
* @returns {Boolean}
*/
tryLater: function () {
if (this.exists()) {
var startInterval = goodTimesIntervals[0];
if (goodTimesIntervals.length > 1 || startInterval.getLength() >= duration + 30) {

Choose a reason for hiding this comment

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

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

startInterval.start += 30;
simplifyTimesIntervals();

return true;
}

return false;
}

return false;
}
};
Expand Down