Skip to content
Closed
Changes from all commits
Commits
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
154 changes: 151 additions & 3 deletions robbery.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,101 @@
*/
exports.isStar = true;

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

function dateToTimestamp(date) {
var isoDate = date;
DAYS.forEach(function (day, i) {
isoDate = isoDate.replace(day, i + 1);
});
Copy link

Choose a reason for hiding this comment

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

можно еще так:

isoDate = date.replace(/ПН|ВТ|СР/, function (day) {
    return DAYS.indexOf(day) + 1;
});

Copy link
Author

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.

Ваш - твой, пожалуйста) Согласен, насколько медленнее? Из явных плюсов - в моем варианте семантика кода ближе к задаче - код написан ровно так как от него требуется

Copy link
Author

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 января 1970 года.
соответственно банк меняет сигнализацию 4 января 1970 года. */
isoDate = isoDate.replace(/^(\d) (\d{2}:\d{2})(\+\d+)$/, '1 $1 1970 $2 UTC$3');

return Date.parse(isoDate);
}

function getTimezone(time) {
var match = /^\d{2}:\d{2}\+(\d+)$/.exec(time);

return match && Number(match[1]);
}

function toTimestampInterval(interval) {
return {
from: dateToTimestamp(interval.from),
to: dateToTimestamp(interval.to)
};
}

function getBusyIntervals(schedule) {
return Object.keys(schedule).reduce(function (busyIntervals, thugName) {
return busyIntervals.concat(
schedule[thugName].map(toTimestampInterval)
);
}, []);
}

function getWorkingDays(workingHours) {
return DAYS.map(function (day) {
var from = day + ' ' + workingHours.from;
var to = day + ' ' + workingHours.to;

return {
from: dateToTimestamp(from),
to: dateToTimestamp(to)
};
});
}

function invertBusyIntervals(busyIntervals, weekStart, deadline) {
var sortedBusyIntervals = busyIntervals.slice().sort(function (one, another) {
return one.from - another.from;
});
var spareIntervals = [];
var current = weekStart;
sortedBusyIntervals.forEach(function (interval) {
if (current < interval.from) {
spareIntervals.push({
from: current,
to: interval.from
});
}
current = Math.max(interval.to, current);
});
if (current < deadline) {
spareIntervals.push({
from: current,
to: deadline
});
}

return spareIntervals;
}

function getRobberyIntervals(spareIntervals, workingDays, durationInMilliseconds) {
var robberyIntervals = [];
spareIntervals.forEach(function (interval) {
workingDays.forEach(function (day) {
var attemptTo = Math.min(day.to, interval.to);
var attemptFrom = Math.max(day.from, interval.from);
if (attemptTo - attemptFrom >= durationInMilliseconds) {
robberyIntervals.push({
from: attemptFrom,
to: attemptTo
});
}
});
});

return robberyIntervals;
}

function twoDigitsFormat(number) {
return number.toLocaleString(undefined, { 'minimumIntegerDigits': 2 });
}
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.

но ты его кроме как с двойкой не используешь, поэтому можно переименовать в twoDigitsFormat(number)


/**
* @param {Object} schedule – Расписание Банды
* @param {Number} duration - Время на ограбление в минутах
Expand All @@ -15,7 +110,21 @@ exports.isStar = true;
* @returns {Object}
*/
exports.getAppropriateMoment = function (schedule, duration, workingHours) {
console.info(schedule, duration, workingHours);
var bankTimezone = getTimezone(workingHours.from);
var weekStart = dateToTimestamp('ПН 00:00+' + bankTimezone);
var deadline = dateToTimestamp('СР 23:59+' + bankTimezone);
Copy link

Choose a reason for hiding this comment

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

пока просто мысли вслух: у тебя есть хеплер getWorkingDays - который уже внутри содержит знание о том, что на ограбление есть пн-ср (кстати, тогда название должно об этом говорить - м.б. getAppropriateIntervals) - поэтому weekStart и deadline уже не нужны

Copy link
Author

Choose a reason for hiding this comment

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

Это и padNumberLeft: да, в кодстайле сказано не писать лишние вещи. Но курс ООП научил нас такой штуке как "переиспользование кода".
Мне кажется, что эти функции содержат не очень много лишнего функционала, а их направленность на кокретную задачу противоречит ООП.

Copy link
Author

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. Есть команда, которая делает сложную задачу. Задача обсуждается, выделяются этапы - подзадачи, работа распределяется. Таким образом, делая этап №1, команда не просто делает конкретные задачи, а делает их так, чтобы потом было удобно выполнить этапы №2,3 ... Тут как раз надо делать "чуть больше", чем нужно прямо сейчас, потому что мы понимаем, зачем мы это делаем.
  2. Есть конкретная задача, понимание, что это не последняя задача по заданной теме. Какие задачи потом будут, я не знаю, но знаю что они будут. Тогда на основе опыта нужно "предугадать", как лучше писать - где делать узко, где немного шире.

В данном случае ни один из вариантов не подходит, поэтому я предлагаю упростить. Тем более мое упрощение не убьет напрочь твою задумку - ее легко будет вернуть простым рефакторингом

Copy link

@Vittly Vittly Oct 24, 2016

Choose a reason for hiding this comment

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

Если бы я был грабителем, то я учел бы что следующий банк

принято)


var busyIntervals = getBusyIntervals(schedule);
var spareIntervals = invertBusyIntervals(busyIntervals, weekStart, deadline);

var durationInMilliseconds = duration * 60 * 1000;
var workingDays = getWorkingDays(workingHours);
var robberyIntervals = getRobberyIntervals(
spareIntervals,
workingDays,
durationInMilliseconds
);
var currentRobberyIndex = 0;

return {

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

/**
Expand All @@ -35,7 +144,23 @@ exports.getAppropriateMoment = function (schedule, duration, workingHours) {
* @returns {String}
*/
format: function (template) {
return template;
if (!this.exists()) {
Copy link

Choose a reason for hiding this comment

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

можно и так:

var robbery = robberyIntervals[0];

if (!robbery) {
    return '';
}

// ...

return '';
}

var hourInMilliseconds = 60 * 60 * 1000;
var date = new Date(
robberyIntervals[currentRobberyIndex].from +
bankTimezone * hourInMilliseconds
);
var minutes = twoDigitsFormat(date.getUTCMinutes());
var hours = twoDigitsFormat(date.getUTCHours());
var day = DAYS[date.getUTCDate() - 1];

return template
.replace('%HH', hours)
.replace('%MM', minutes)
.replace('%DD', day);
},

/**
Expand All @@ -44,6 +169,29 @@ exports.getAppropriateMoment = function (schedule, duration, workingHours) {
* @returns {Boolean}
*/
tryLater: function () {
if (!this.exists()) {
return false;
}

var halfHourInMilliseconds = 30 * 60 * 1000;
var i = currentRobberyIndex;
var delayedRobberyStart = robberyIntervals[i].from + halfHourInMilliseconds;
while (i < robberyIntervals.length && robberyIntervals[i].from < delayedRobberyStart) {
if (robberyIntervals[i].to - delayedRobberyStart >= durationInMilliseconds) {
robberyIntervals[i].from = delayedRobberyStart;
currentRobberyIndex = i;

return true;
}
Copy link

Choose a reason for hiding this comment

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

нет, не согласен.

По первой части: ты пробуешь попробовать ограбить в тот же интервал с задержкой, но оставляешь сайдэффект, о котором никто в задаче не просит - правишь robberyIntervals - это лишнее - просто верни true.

По второй части: ты снова изменяешь robberyIntervals - зачем? Плюс, ты считаешь, что второй интервал обязательно подойдет - но почему? Откуда уверенность что robberyIntervals[1].from - robberyIntervals[0].from > halfHourInMilliseconds? Это надо проверять

Copy link
Author

Choose a reason for hiding this comment

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

  1. Следующий вызов format уже должен вернуть новое время. После вызова tryLater уже нет возможности вернуться на старое время. Да и как format без сайдэффектов потом узнает, что нужно вернуть не самое ранее время?
  2. robberyIntervals состоит исключительно из благоприятных для ограбления промежутков по построению. Про сайдэффекты то же самое

Copy link

Choose a reason for hiding this comment

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

  1. Спросил ребят, да, похоже, требование такое - сайдэффекты ок

Copy link

@Vittly Vittly Oct 24, 2016

Choose a reason for hiding this comment

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

2- Это не ответ - robberyIntervals[1].from - robberyIntervals[0].from > halfHourInMilliseconds - это же не обязательно

Copy link

Choose a reason for hiding this comment

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

уточню - duration может быть меньше получаса, например - 5 минут. Тогда следующий период может быть после первого - через 10 минут, а не через 30

Copy link
Author

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.

да, видимо тесты не покрывают

i++;
}

if (i < robberyIntervals.length) {
currentRobberyIndex = i;

return true;
}
Copy link

Choose a reason for hiding this comment

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

Как-то сложно получилось, и, главное, кажется с логикой проблемы. Чего мы хотим? Нам нужно:

  • либо интервал, который начинается позже delayedRobberyStart
  • либо интервал, который начинается раньше, но его время, отсчитанное от delayedRobberyStart было больше duration

Мне кажется, не надо два кейса мешать в один. Лучше написать так:

var i = 0;

// 1ый кейс
while (i < robberyIntervals.length && robberyIntervals[i].from < delayedRobberyStart) {
    if (robberyIntervals[i].to - delayedRobberyStart >= durationInMilliseconds) {
        robberyIntervals[i].from = delayedRobberyStart;
        robberyIntervals = robberyIntervals.slice(i);

        return true;
    }
    i++;
}

// 2ой кейс
if (i < robberyIntervals.length) {
    robberyIntervals = robberyIntervals.slice(i);
    return true;
}

Так что пока еще 🍅

Copy link

Choose a reason for hiding this comment

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

так, а чего мы slice не сделали? типа: robberyIntervals = robberyIntervals.slice(i) ? Вон в строчке №181 мы же оставляем сайдэффект - правим границу интервала, но элементы остаются не местах. Как ты мне сам объяснял, должен работать следующий tryLater - как еще на полчаса дальше

Copy link
Author

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.

мм, понял тебя. Только не понятно, как это согласуется с работой метода exists - он же тоже должен учитывать currentRobberyIndex - а то, например, сломается format (что наводит нас на мысль, что все таки стоит делать slice)

но это уже придирки, с задачей ты разобрался

Copy link
Author

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.

И ничего не сломается, т.к. currentRobberyIndex в exists не используется

Copy link
Author

Choose a reason for hiding this comment

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

Точнее, robberyIndex не превысит длину robberyIntervals(если robberyIntervals не пуст)

Copy link

@Vittly Vittly Oct 26, 2016

Choose a reason for hiding this comment

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

еще раз, сломается, я же написал

if (!this.exists()) { // не посмотрели на currentRobberyIndex, а он, может быть, уже больше длины
    return '';
}
var hourInMilliseconds = 60 * 60 * 1000;
var date = new Date(
    robberyIntervals[currentRobberyIndex].from + // бдыжь, бах, бум! все взорволось, undefined is not an object

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

Copy link

@Vittly Vittly Oct 26, 2016

Choose a reason for hiding this comment

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

robberyIndex не превысит длину robberyIntervals

это другой разговор:)

Copy link
Author

Choose a reason for hiding this comment

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

Что-то я запутался. Если currentRobberyIndex равен 0 и exists вернет true, то нулевой элемент в robberyIntervals есть. Если currentRobberyIndex больше 0, то смотрим где он меняется:

            while (i < robberyIntervals.length && robberyIntervals[i].from < delayedRobberyStart) {
                if (robberyIntervals[i].to - delayedRobberyStart >= durationInMilliseconds) {
                    robberyIntervals[i].from = delayedRobberyStart;
                    currentRobberyIndex = i;

                    return true;
                }
                i++;
            }

            if (i < robberyIntervals.length) {
                currentRobberyIndex = i;

                return true;
            }

В обоих случаях присвоение произойдет только тогда, когда присваимое значение(i) меньше длины robberyIntervals, то есть если exists вернет true, то robberyInterval[currentRobberyInterval] != undefined.

Copy link

Choose a reason for hiding this comment

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

дело в том, что комменты:

Точнее, robberyIndex не превысит длину...

и

еще раз, сломается, я же написал...

мы написали почти одновременно, и когда я писал свой - твоего еще не было. Я не заметил, что robberyIntervals меняясь не убежит за длину. Потом я запостил свой коммент и тут же увидел твой и все понял. А в последнем комменте ты еще раз все проговорил, спасибо


return false;
}
};
Expand Down