Кошара Павел #4
Кошара Павел #4
Conversation
|
🍅 Пройдено тестов 15 из 19 |
|
🍅 Пройдено тестов 18 из 19 |
|
🍏 Пройдено тестов 19 из 19 |
|
@mironov-alexey обрати внимание решено доп. задание |
robbery.js
Outdated
| exports.getAppropriateMoment = function (schedule, duration, workingHours) { | ||
| console.info(schedule, duration, workingHours); | ||
| schedule = normalizeSchedule(schedule); | ||
| duration *= 60; |
There was a problem hiding this comment.
тут и в предыдущей строке изменяется то, что пришло в параметрах функции. Лучше так не делать и создавать новые переменные, названия которых будет лучше отображать то, что в них находится. Например, потом можно легко забыть, что в schedule уже лежит не просто schedule, а normalizedSchedule, аналогично с duration
robbery.js
Outdated
| schedule = normalizeSchedule(schedule); | ||
| duration *= 60; | ||
| var workingDays = getWorkingDays(workingHours); | ||
| var bankTimezone = parseInt(/\+(\d+)$/.exec(workingHours.from)[1]); |
There was a problem hiding this comment.
В parseInt нужно вторым аргументом указывать основание системы счисления. Чаще всего это, конечно 10. Так исторически сложилось, что в некоторых средах, в которых исполняется js, дефолтное значение этой системы не 10, а 8. Т.е. например, parseInt("0302"); вернёт 194.
robbery.js
Outdated
| duration *= 60; | ||
| var workingDays = getWorkingDays(workingHours); | ||
| var bankTimezone = parseInt(/\+(\d+)$/.exec(workingHours.from)[1]); | ||
| var weekStart = Date.parse('1 1 1970 00:00 GMT+' + bankTimezone) / 1000; |
There was a problem hiding this comment.
На первый взгляд не очень понятно, что это за magic-constant строки. В частности, почему именно 1970 год? :)
There was a problem hiding this comment.
как можно объяснить, что это число было выбрано путем 4 бросков десятистороннего кубика?
robbery.js
Outdated
| var deadline = Date.parse('1 3 1970 23:59 GMT+' + bankTimezone) / 1000; | ||
| var spareTime = getSpareTimes(schedule, weekStart, deadline); | ||
|
|
||
| var robberyTime = []; |
There was a problem hiding this comment.
Может лучше robberyTimes? Это же список.
robbery.js
Outdated
| var bankTimezone = parseInt(/\+(\d+)$/.exec(workingHours.from)[1]); | ||
| var weekStart = Date.parse('1 1 1970 00:00 GMT+' + bankTimezone) / 1000; | ||
| var deadline = Date.parse('1 3 1970 23:59 GMT+' + bankTimezone) / 1000; | ||
| var spareTime = getSpareTimes(schedule, weekStart, deadline); |
There was a problem hiding this comment.
Тут кстати то же, что в комментарии ниже :) Функция называется getSpareTimes и по всей видимости возвращает список отрезков, а переменная в единственном числе названа.
robbery.js
Outdated
| var from = DAYS[i] + ' ' + workingHours.from; | ||
| var to = DAYS[i] + ' ' + workingHours.to; | ||
| workingDays.push({ | ||
| 'from': dateToTimestamp(from), |
There was a problem hiding this comment.
Можно, кстати, не писать 'from' в кавычках. Без них тоже ок будет:
{
from: 123
}
robbery.js
Outdated
| current = current < schedule[i].to ? schedule[i].to : current; | ||
| continue; | ||
| } | ||
| spareTime.push({ 'from': current, 'to': schedule[i].from }); |
There was a problem hiding this comment.
И тут можно без кавычек. А ещё ты где-то пишешь подобные объявления в одну строчку
{ 'from': current, 'to': schedule[i].from }а где-то в несколько
workingDays.push({
'from': dateToTimestamp(from),
'to': dateToTimestamp(to)
});Вряд ли есть какой-то правильный стиль, но лучше делать везде однообразно
robbery.js
Outdated
| if (one.from < another.from) { | ||
| return -1; | ||
| } | ||
| if (one.from > another.from) { |
robbery.js
Outdated
| var busy = schedule[thug].map(toTimestampInterval); | ||
| busyIntervals = busyIntervals.concat(busy); | ||
| }); | ||
| busyIntervals = busyIntervals.sort(function (one, another) { |
There was a problem hiding this comment.
sort изменяет исходный массив (но и возвращает его, да) так что это присвоение бессмысленное по двум причинам :) Можно просто return busyIntervals.sort...
robbery.js
Outdated
| } | ||
| str = str.replace(/(\d) (\d{2}:\d{2})(\+\d+)?/, '1 $1 1970 $2 GMT$3'); | ||
|
|
||
| return Date.parse(str) / 1000; |
There was a problem hiding this comment.
Прост интересно, а зачем ты везде избавляешься от миллисекунд?
|
🍏 Пройдено тестов 19 из 19 |
|
@mironov-alexey обрати внимание решено доп. задание |
robbery.js
Outdated
| } | ||
|
|
||
| function getWorkingDays(workingHours) { | ||
| return DAYS.reduce(function (acc, day) { |
robbery.js
Outdated
| return true; | ||
| } | ||
| if (robberyIntervals.length > 1) { | ||
| robberyIntervals.shift(); |
There was a problem hiding this comment.
Я бы кстати написал так
return !!robberyIntervals.shift();но ментор наверн будет ругаться. Так что лучше оставить так 😆
|
🚀 |
|
🍏 Пройдено тестов 19 из 19 |
robbery.js
Outdated
| */ | ||
| exports.getAppropriateMoment = function (schedule, duration, workingHours) { | ||
| console.info(schedule, duration, workingHours); | ||
| var bankTimezone = parseInt(/\+(\d+)$/.exec(workingHours.from)[1], 10); |
There was a problem hiding this comment.
капец, как сложно:
- создали регвыр
- применили регвыр
- взяли капчер
- привели к числу через
parseInt
все про регвыр лучше унести в хелпер - можно надежнее написать:
function getTimezoneFrom(stamp) {
var match = /\+(\d+)$/.exec(stamp);
// match-а ведь может и не быть
return match && Number(match[1]); // parseInt нужен, если мы уверены только в том, что строка начинается с цифр, а дальше могут быть буквы - тут стопудов у нас только цифры
}
|
🍏 Пройдено тестов 19 из 19 |
|
@Vittly обрати внимание решено доп. задание |
|
🍅 |
robbery.js
Outdated
|
|
||
| /* Будем считать, что понедельник - 1 января 1970 года. | ||
| соответственно банк меняет сигнализацию 4 января 1970 года. */ | ||
| isoDate = isoDate.replace(/(\d) (\d{2}:\d{2})(\+\d+)?/, '1 $1 1970 $2 GMT$3'); |
There was a problem hiding this comment.
еще правильнее писать полный регесп - ^(\d) (\d{2}:\d{2})(\+\d+)?$ - типа добавить крышку и доллар. Кстати, а почему таймзоны может не быть? Ты в курсе, что тогда возьмется локальная?
There was a problem hiding this comment.
Раз ниже используешь getUTCMinutes и getUTCHours - может тут будем тоже UTC, а не GMT писать - для однородности
There was a problem hiding this comment.
Если UTC или GMT(которые судя по всему на практике идентичны) указаны, то местное время точно не возьмется, а возьмется UTC+0
There was a problem hiding this comment.
собственно GMT добавлено для того чтобы не было проблем с машинами в разных временных зонах
There was a problem hiding this comment.
а возьмется UTC+0
нет, у меня на машине, например, не так
There was a problem hiding this comment.
> Date.parse('1 1 1970 1:00')
-14400000
> Date.parse('1 1 1970 1:00 UTC')
3600000
то есть я неправ если утверждаю что второй вариант везде вернет 3600000? добавить +0 не выглядит решением и получается что задача взятия времени по Гривичу становится нерешаемой
There was a problem hiding this comment.
ой, теперь понял, в чем проблема. Да, прав, что второй вариант хороший. Я тут предлагаю вместо GMT писать UTC (так как вроде одно и тоже и ниже ты используешь UTC-методы) и еще прошу объяснить мне, вот это:
(+\d+)?/
когда мы в задаче парсим время без зоны?
There was a problem hiding this comment.
не очень внимательно читал задание
| var isoDate = date; | ||
| DAYS.forEach(function (day, i) { | ||
| isoDate = isoDate.replace(day, i + 1); | ||
| }); |
There was a problem hiding this comment.
можно еще так:
isoDate = date.replace(/ПН|ВТ|СР/, function (day) {
return DAYS.indexOf(day) + 1;
});There was a problem hiding this comment.
Мне кажется, что Ваш вариант медленнее
There was a problem hiding this comment.
Ваш - твой, пожалуйста) Согласен, насколько медленнее? Из явных плюсов - в моем варианте семантика кода ближе к задаче - код написан ровно так как от него требуется
There was a problem hiding this comment.
из минусов - при добавлении нового дня придется и регулярку менять вместе с массивом дат
про скорость я пожалуй возьму свои слова назад, уже не так уверен
There was a problem hiding this comment.
да, регвыр придется синхронизировать - убедил, тут больше не пристаю
robbery.js
Outdated
| } | ||
|
|
||
| function getBusyIntervals(schedule) { | ||
| return Object.keys(schedule).reduce(function (acc, thug) { |
There was a problem hiding this comment.
acc -> intervals или result
thug -> thugName?
robbery.js
Outdated
| function getBusyIntervals(schedule) { | ||
| return Object.keys(schedule).reduce(function (acc, thug) { | ||
| var busy = schedule[thug].map(toTimestampInterval); | ||
| acc = acc.concat(busy); |
There was a problem hiding this comment.
я бы так написал (не знаю, пустит ли линтер):
return acc.concat(
schedule[thug].map(toTimestampInterval)
);| console.info(schedule, duration, workingHours); | ||
| var bankTimezone = getTimezone(workingHours.from); | ||
| var weekStart = dateToTimestamp('ПН 00:00+' + bankTimezone); | ||
| var deadline = dateToTimestamp('СР 23:59+' + bankTimezone); |
There was a problem hiding this comment.
пока просто мысли вслух: у тебя есть хеплер getWorkingDays - который уже внутри содержит знание о том, что на ограбление есть пн-ср (кстати, тогда название должно об этом говорить - м.б. getAppropriateIntervals) - поэтому weekStart и deadline уже не нужны
There was a problem hiding this comment.
Это и padNumberLeft: да, в кодстайле сказано не писать лишние вещи. Но курс ООП научил нас такой штуке как "переиспользование кода".
Мне кажется, что эти функции содержат не очень много лишнего функционала, а их направленность на кокретную задачу противоречит ООП.
There was a problem hiding this comment.
Если бы я был грабителем, то я учел бы что следующий банк может не менять сигнализацию вообще, а проблема выбора подходящего времени все равно есть)
There was a problem hiding this comment.
Все правильно говоришь, но тут есть несколько "но". Переиспользование должно быть востребовано.
Примеры:
- Есть команда, которая делает сложную задачу. Задача обсуждается, выделяются этапы - подзадачи, работа распределяется. Таким образом, делая этап №1, команда не просто делает конкретные задачи, а делает их так, чтобы потом было удобно выполнить этапы №2,3 ... Тут как раз надо делать "чуть больше", чем нужно прямо сейчас, потому что мы понимаем, зачем мы это делаем.
- Есть конкретная задача, понимание, что это не последняя задача по заданной теме. Какие задачи потом будут, я не знаю, но знаю что они будут. Тогда на основе опыта нужно "предугадать", как лучше писать - где делать узко, где немного шире.
В данном случае ни один из вариантов не подходит, поэтому я предлагаю упростить. Тем более мое упрощение не убьет напрочь твою задумку - ее легко будет вернуть простым рефакторингом
There was a problem hiding this comment.
Если бы я был грабителем, то я учел бы что следующий банк
принято)
robbery.js
Outdated
| spareIntervals.forEach(function (interval) { | ||
| workingDays.forEach(function (day) { | ||
| var minTo = Math.min(day.to, interval.to); | ||
| var maxFrom = Math.max(day.from, interval.from); |
There was a problem hiding this comment.
я бы назвал переменные иначе - так как min и max описывают способ их вычисления, а не их смысл. Мое предложение: attemptFrom и attemptTo
| */ | ||
| format: function (template) { | ||
| return template; | ||
| if (!this.exists()) { |
There was a problem hiding this comment.
можно и так:
var robbery = robberyIntervals[0];
if (!robbery) {
return '';
}
// ...
robbery.js
Outdated
| } | ||
|
|
||
| var hourInMilliseconds = 60 * 60 * 1000; | ||
| var date = new Date(robberyIntervals[0].from + bankTimezone * hourInMilliseconds); |
There was a problem hiding this comment.
воу-воу, а ограбление разве не в часовом поясе банка уже? наверное тут, дабы утащить время в UTC0, надо делать - bankTimezone * hourInMilliseconds? Или я неправ?
There was a problem hiding this comment.
Тесты для выявления таких ошибок и нужны. Все интервалы в коде в UTC.
There was a problem hiding this comment.
тогда прошу мне объяснить на примере, где я неправ в своих рассуждениях
There was a problem hiding this comment.
Date.parse возвращает таймстамп, который по определению в UTC. Все интервалы состоят из этих таймстампов. Когда в дате пишется 10:00+5, имеется в виду сдвиг на 5 часов вперед относительно UTC. Этот сдвиг я к имеющемуся таймстампу и добавляю, дабы получить дату в часовом поясе банка
There was a problem hiding this comment.
это я все понимаю) просто robberyIntervals уже посчитаны в зоне банка, то есть сдвиг уже зашит - а ты еще раз добавляешь, поэтому спрашиваю
There was a problem hiding this comment.
вообще все интервалы у меня в UTC, включая robberyIntervals
There was a problem hiding this comment.
аааа, допер - типа внутри robberyIntervals уже есть сдвиг (за счет isoDate.parse('... GMT$3')) - но тут мы хотим получить "чистое время" - типа без сдвига, а тогда мы должны еще плюсануть сдвиг времени банка, как-то так:
format = UTC((time + zone) + zone) // вызов UTC внешний zone отбросит
|
|
||
| function padNumberLeft(number, length) { | ||
| return number.toLocaleString(undefined, { 'minimumIntegerDigits': length }); | ||
| } |
There was a problem hiding this comment.
но ты его кроме как с двойкой не используешь, поэтому можно переименовать в twoDigitsFormat(number)
| robberyIntervals.shift(); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
нет, не согласен.
По первой части: ты пробуешь попробовать ограбить в тот же интервал с задержкой, но оставляешь сайдэффект, о котором никто в задаче не просит - правишь robberyIntervals - это лишнее - просто верни true.
По второй части: ты снова изменяешь robberyIntervals - зачем? Плюс, ты считаешь, что второй интервал обязательно подойдет - но почему? Откуда уверенность что robberyIntervals[1].from - robberyIntervals[0].from > halfHourInMilliseconds? Это надо проверять
There was a problem hiding this comment.
- Следующий вызов format уже должен вернуть новое время. После вызова tryLater уже нет возможности вернуться на старое время. Да и как format без сайдэффектов потом узнает, что нужно вернуть не самое ранее время?
- robberyIntervals состоит исключительно из благоприятных для ограбления промежутков по построению. Про сайдэффекты то же самое
There was a problem hiding this comment.
- Спросил ребят, да, похоже, требование такое - сайдэффекты ок
There was a problem hiding this comment.
2- Это не ответ - robberyIntervals[1].from - robberyIntervals[0].from > halfHourInMilliseconds - это же не обязательно
There was a problem hiding this comment.
уточню - duration может быть меньше получаса, например - 5 минут. Тогда следующий период может быть после первого - через 10 минут, а не через 30
There was a problem hiding this comment.
ах, с трудом понял. плохие тесты, видимо
|
🍏 Пройдено тестов 19 из 19 |
|
@Vittly обрати внимание решено доп. задание |
|
🍏 |
|
🍏 Пройдено тестов 19 из 19 |
|
@Vittly обрати внимание решено доп. задание |
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Как-то сложно получилось, и, главное, кажется с логикой проблемы. Чего мы хотим? Нам нужно:
- либо интервал, который начинается позже
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;
}Так что пока еще 🍅
|
🍏 Пройдено тестов 19 из 19 |
|
@Vittly обрати внимание решено доп. задание |
|
🍏 |
| currentRobberyIndex = i; | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
так, а чего мы slice не сделали? типа: robberyIntervals = robberyIntervals.slice(i) ? Вон в строчке №181 мы же оставляем сайдэффект - правим границу интервала, но элементы остаются не местах. Как ты мне сам объяснял, должен работать следующий tryLater - как еще на полчаса дальше
There was a problem hiding this comment.
Теперь слайсы не нужны. Я решил хранить указатель на текущий самый ранний(с учетом попыток начать позже) интервал для ограбления
| currentRobberyIndex = i; | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
мм, понял тебя. Только не понятно, как это согласуется с работой метода exists - он же тоже должен учитывать currentRobberyIndex - а то, например, сломается format (что наводит нас на мысль, что все таки стоит делать slice)
но это уже придирки, с задачей ты разобрался
There was a problem hiding this comment.
Тогда можно закрыть, наверное?)
There was a problem hiding this comment.
И ничего не сломается, т.к. currentRobberyIndex в exists не используется
There was a problem hiding this comment.
Точнее, robberyIndex не превысит длину robberyIntervals(если robberyIntervals не пуст)
There was a problem hiding this comment.
еще раз, сломается, я же написал
if (!this.exists()) { // не посмотрели на currentRobberyIndex, а он, может быть, уже больше длины
return '';
}
var hourInMilliseconds = 60 * 60 * 1000;
var date = new Date(
robberyIntervals[currentRobberyIndex].from + // бдыжь, бах, бум! все взорволось, undefined is not an objectне торопись так в выводах, читай внимательнее набросы, я же их пишу, чтоб чему-то научить
There was a problem hiding this comment.
robberyIndex не превысит длину robberyIntervals
это другой разговор:)
There was a problem hiding this comment.
Что-то я запутался. Если 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.
There was a problem hiding this comment.
дело в том, что комменты:
Точнее, robberyIndex не превысит длину...
и
еще раз, сломается, я же написал...
мы написали почти одновременно, и когда я писал свой - твоего еще не было. Я не заметил, что robberyIntervals меняясь не убежит за длину. Потом я запостил свой коммент и тут же увидел твой и все понял. А в последнем комменте ты еще раз все проговорил, спасибо
|
@melanholy, Молодец! |
4 participants
No description provided.