Conversation
|
🍏 Пройдено тестов 19 из 19 |
|
@dotokoto обрати внимание: решено доп. задание |
dotokoto
left a comment
dotokoto
left a comment
There was a problem hiding this comment.
Привет! Я посмотрела задачу, есть замечания по коду. Когда исправишь, напиши мне пожалуйста в Слак, чтобы я еще раз проверила.
|
|
||
| exports.isStar = true; | ||
|
|
||
| const CURRENT_YEAR = 2017; |
There was a problem hiding this comment.
Подозрительные константы. Зачем именно текущий год и месяц? Либо брать всегда текущий, не забивая его в константы. Либо взять какие-то дефолтные значения. Сейчас выглядит так, что эти значение через пару недель протухнут, и надо будет переписывать программу
| const CURRENT_MONTH = 10; | ||
| const HOUR_IN_MILLISEC = 1000 * 60 * 60; | ||
| const MINUTE_IN_MILLISEC = 1000 * 60; | ||
| const HALF_HOUR = 30; |
There was a problem hiding this comment.
По названию константы непонятно, что в ней хранятся минуты
| * @param {Number} minutes | ||
| * @returns {String} | ||
| */ | ||
| function getCorrectFormat(string, day, hours, minutes) { |
There was a problem hiding this comment.
У тебя много функций, связанных с работой с датами и временем. Можно было бы вынести их в отдельный файл-библиотеку
| * @returns {Number} | ||
| */ | ||
| function getNumberOfDay(day) { | ||
| return ROBBERY_DAYS.indexOf(getDay(day)) + 1; |
There was a problem hiding this comment.
эта функция точно всегда будет правильно работать? Дней 7, а в ROBBERY_DAYS их только 3. При этом в расписании может попасться и четверг, и пятница, и тд
| * @returns {Number} | ||
| */ | ||
| function getTimeZone(time) { | ||
| return time.substr(-1); |
| if (schedule.length !== 0) { | ||
| schedule.reduceRight((previousValue, currentValue, index) => { | ||
| if (currentValue.from === previousValue.from && currentValue.to === previousValue.to) { | ||
| schedule.splice(index, 1); |
There was a problem hiding this comment.
Я бы не рекомендовала делать splice на массиве, по которому проходит цикл. Очень легко ошибиться и сломать что-нибудь. Лучше создавай новый массив, тогда и reduceRight можно будет заменить на что-нибудь другое
| let freeTimes = []; | ||
| let range = { | ||
| from: Date.UTC(CURRENT_YEAR, CURRENT_MONTH, 1, 0, 0) - timezone, | ||
| to: Date.UTC(CURRENT_YEAR, CURRENT_MONTH, 3, 23, 59) - timezone }; |
There was a problem hiding this comment.
3 опять похоже на захардкоженную константу
| let range = { | ||
| from: Date.UTC(CURRENT_YEAR, CURRENT_MONTH, 1, 0, 0) - timezone, | ||
| to: Date.UTC(CURRENT_YEAR, CURRENT_MONTH, 3, 23, 59) - timezone }; | ||
| bankSchedule.forEach(day => { |
There was a problem hiding this comment.
Этот шаг не относится к reverseSchedule, лучше вынести его в отдельную функцию
| * @param {Number} duration | ||
| * @returns {Array} | ||
| */ | ||
| function reverseSchedule(commonSchedule, bankSchedule, timezone, duration) { |
There was a problem hiding this comment.
И опять же, можно переименовать функцию. Так как по факту, она возвращает доступное для ограбления время
| const CURRENT_MONTH = 10; | ||
| const HOUR_IN_MILLISEC = 1000 * 60 * 60; | ||
| const MINUTE_IN_MILLISEC = 1000 * 60; | ||
| const HALF_HOUR = 30; |
There was a problem hiding this comment.
И я бы переименовала эту константу. Так как полчаса - на самом деле тоже параметр задачи, может поменяться и на 40 минут, и на 10.
|
🍅 |
3 participants
No description provided.