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

Cолодовникова Екатерина #69

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KatySolo
Copy link

No description provided.

@honest-hrundel honest-hrundel changed the title Солодовникова Екатерина Cолодовникова Екатерина Oct 19, 2017
@honest-hrundel
Copy link

🍅 Пройдено тестов 9 из 16

Copy link

@mokhov mokhov left a comment

Choose a reason for hiding this comment

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

Код невозможно читать, много волшебства и не читаемых функций. Давай начнём исправления с простого – нужно сделать так, чтобы названия функций вызываемых внутри getAppropriateMoment и findPeriod были такими, чтобы можно было понять что происходит
🍅

@@ -0,0 +1,15 @@
{
// Используйте IntelliSense, чтобы узнать о возможных атрибутах.
Copy link

Choose a reason for hiding this comment

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

Это что такое?

robbery.js Outdated
exports.isStar = true;
exports.isStar = false;
const DayShift = {
'ПН': 'ВТ',
Copy link

Choose a reason for hiding this comment

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

очень не понятно почему так :)

robbery.js Outdated
@@ -4,7 +4,16 @@
* Сделано задание на звездочку
* Реализовано оба метода и tryLater
*/
exports.isStar = true;
exports.isStar = false;
const DayShift = {
Copy link

Choose a reason for hiding this comment

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

Обычно с большой буквы именуются классы, а не объекты

robbery.js Outdated
* @param {Number} timezone
* @returns {Object}
*/
function evenSchedule(schedule, timezone) {
Copy link

Choose a reason for hiding this comment

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

Название не соответствует описанию

robbery.js Outdated
* @param {Number} timezone
* @returns {Object}
*/
function changeRecord(date, timezone) {
Copy link

Choose a reason for hiding this comment

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

И тоже название не говорящее

return date;
}
let rawDateFrom = parseDate(date.from);
// ['ПН','12','00','5']
Copy link

Choose a reason for hiding this comment

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

Смотри, вот даже то что ты комментарием пишешь результат операции говорит о том, что этот код спустя неделю будет сложно отлазивать. Как можно было бы назвать функцию parseDate, чтобы не запоминать что там будет?

robbery.js Outdated
*/
function parseDate(date) {
// 'ПН 09:00+3'
let day = date.split(':')[0].split(' ')[0];
Copy link

Choose a reason for hiding this comment

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

date.split(:) можно было бы сохранить в переменную, чтобы не делать каждый раз .split

robbery.js Outdated
let newHour = hours + shift;
if (newHour > 24) {
switch (rawDate[0]) {
case 'ПН':
Copy link

Choose a reason for hiding this comment

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

Кажется для чего-то такого у тебя есть странный объект DayShift выше

robbery.js Outdated
let timezone = getTimeZone(workingHours);
schedule = evenSchedule(schedule, timezone);
schedule = cutSchedule(schedule);
schedule = convertScheduleToMinutes (schedule);
Copy link

Choose a reason for hiding this comment

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

И lint'ер не заргулася?

@honest-hrundel
Copy link

🍅 Пройдено тестов 9 из 16

@mokhov
Copy link

mokhov commented Nov 6, 2017

Пройденных тестов стало меньше

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants