Skip to content

Conversation

@Naturin
Copy link

@Naturin Naturin commented Oct 18, 2017

No description provided.

@honest-hrundel honest-hrundel changed the title v.1 Титова Наталья Oct 18, 2017
@honest-hrundel
Copy link

🍅 Пройдено тестов 6 из 19

@honest-hrundel
Copy link

🍅 Пройдено тестов 6 из 19

@honest-hrundel
Copy link

🍅 Пройдено тестов 12 из 19

@honest-hrundel
Copy link

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

Copy link

@savichev-igor savichev-igor left a comment

Choose a reason for hiding this comment

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

Читать код сложно, постарайся внести побольше семантики и избавиться от этих индексов, посмотри в сторону использования объектов и свойств

Добавь где-то JSDoc или лаконичные комментарии, чтобы объяснить что происходит

🍅

var MINUTES_IN_HOUR = 60;
var HOUR_IN_DAY = 24;
var TOTAL_TIME_LINE = WEEK_DAYS.length * HOUR_IN_DAY * MINUTES_IN_HOUR;
// var TRY_LATER_MINUTES = 30;

Choose a reason for hiding this comment

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

Такого рода комментарии лучше удалять

.replace('%MM', (minutes < 10 ? '0' : '') + minutes);
}

// /**

Choose a reason for hiding this comment

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

???


function calcGangPartyFreeTime(scheduleArr) {
var result = [];
for (let personName in scheduleArr) {

Choose a reason for hiding this comment

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

forEach?

continue;
}
var personResult = [];
for (let i = 0; i < scheduleArr[personName].length; i++) {

Choose a reason for hiding this comment

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

Используешь где-то let, но при этом не используешь const для констант

return hours * MINUTES_IN_HOUR + Number(format[3]);
}

function busyTimeToFreeTime(timeArr) {

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.

За 2 минуты так и не смог понять, что делает этот метод

function busyTimeToFreeTime(timeArr) {
var result = [];
for (let i = 0; i <= timeArr.length; i++) {
if (i === 0) {

Choose a reason for hiding this comment

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

Почему такое исключение для первой итерации?

function getBankWorkingTime(timeArr) {
var result = [];
for (let i = 0; i < WEEK_DAYS.length; i++) {
result.push([stringToInt(WEEK_DAYS[i] + ' ' + timeArr.from),

Choose a reason for hiding this comment

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

Почему бы здесь не использовать интерполяцию строк?


function getBankWorkingTime(timeArr) {
var result = [];
for (let i = 0; i < WEEK_DAYS.length; i++) {

Choose a reason for hiding this comment

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

Только не for)

var arr2 = compare ? arrays[0] : arrays[1];
for (let i = 0; i < arr1.length; i++) {
for (let k = 0; k < arr2.length; k++) {
// if (arr1[i][0] >= arr2[k][1] || arr1[i][1] <= arr2[k][0]) {

Choose a reason for hiding this comment

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

Удалить

}

function checkTimeInterval(arr1, arr2, newArr) {
if (arr1[0] >= arr2[1] || arr1[1] <= arr2[0]) {

Choose a reason for hiding this comment

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

Индексы - сложно и непонятно

@savichev-igor savichev-igor dismissed their stale review October 19, 2017 10:31

Поторопился

@savichev-igor
Copy link

savichev-igor commented Oct 19, 2017

Почему-то в почту стали падать уведомления про твой PR, думал что уже можно проверять

Поздно увидел, что тесты ещё не все пройдены

Можешь уже опираться на моё частичное ревью)

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