Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Козлова Валерия#57

Open
OnaHochetNaMars wants to merge 33 commits into
urfu-2016:master from
OnaHochetNaMars:master
Open

Козлова Валерия #57
OnaHochetNaMars wants to merge 33 commits into
urfu-2016:master from
OnaHochetNaMars:master

Conversation

@OnaHochetNaMars

@OnaHochetNaMars OnaHochetNaMars commented Oct 26, 2016

Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

 

по временной зоне банка

Copy link
Copy Markdown

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

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Comment thread robbery.js Outdated
return dates[0];
}

function toFreeSchedule(schedule) {

@chipolinka chipolinka Nov 4, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Наверное, здесь функция должна называться getFreeTimes? Или можешь придумать еще понятнее?

Comment thread robbery.js
schedule.Linus.forEach(function (k) {
begin = compareDate (1, i.from, j.from, k.from);
end = compareDate (-1, i.to, j.to, k.to);
if (end > begin) {

@chipolinka chipolinka Nov 4, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • Очень большая вложенность.
  • Идет привязка с именам бандитов, что не есть хорошо. Вдруг они изменят состав?
  • Переменные begin и end инициализированы далеко от места использования, и неясно, начало и конец чего они обозначают.
  • i, j, k тоже никак себя не объясняют.

Comment thread robbery.js Outdated
for (var i = day1; i <= Math.min(day2, 3); i++) {
res.push({
from: compareDate (1, new Date (YEAR, MONTH, i, 0, 0), interval.from),
to: compareDate (-1, new Date (YEAR, MONTH, i, 23, 59), interval.to)

@chipolinka chipolinka Nov 4, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Привязка к конкретной дате

Comment thread robbery.js Outdated
};
}

function findFreeIntervals(freeTime, workingHours, res) {

@chipolinka chipolinka Nov 4, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Здесь больше подойдет глагол get. Ты же его возвращаешь?)

Comment thread robbery.js Outdated
return res;
}

function mainFunction(timeToRobbery, msInDuration) {

@chipolinka chipolinka Nov 4, 2016 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Это название функции тоже не годится)

Comment thread robbery.js Outdated
function mainFunction(timeToRobbery, msInDuration) {
var res = [];
var n = timeToRobbery.length;
var c = 0;

@chipolinka chipolinka Nov 4, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Однобуквенных переменных вообще быть не должно, только если в крайних случаях это не цикл по индексам.

Comment thread robbery.js Outdated
c++;
}
}
if (c === 0) {

@chipolinka chipolinka Nov 4, 2016 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Можно записать в 1 строку тернарными операторами)

Comment thread robbery.js Outdated
if (number < 10) {
number = '0' + number;
}
number = String(number);

@chipolinka chipolinka Nov 4, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Есть метод toString на крайний случай

Comment thread robbery.js Outdated
console.info(schedule, duration, workingHours);
var newSchedule = {};
var gmt = parseInt (workingHours.from.slice(5), 10);
var msInDuration = 60 * 1000 * duration;

@chipolinka chipolinka Nov 4, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ай-ай-ай, константы!

Copy link
Copy Markdown

Исправлены не все замечания с прошлого раза. Советую тебе перечитать гайд по оформлению кода.

Copy link
Copy Markdown

🍅

Copy link
Copy Markdown

🍅 Не пройден линтинг или базовые тесты

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

Comment thread robbery.js
var currentDay = WEEK.indexOf(dayOfWeek);
var hours = parseInt(date.slice(3, 5), 10);
var timeZone = parseInt(date.slice(8), 10);
hours = hours - timeZone + bankTimeZone;

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Это уже не просто hours

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

можно же так: hours += bankTimeZone - timeZone

Comment thread robbery.js
function maxDate() {
var dates = [].slice.call(arguments);
dates.sort(function (date1, date2) {

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Тут не нужна пустая строка

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Почему нельзя воспользоваться функцией Math.max?

@OnaHochetNaMars OnaHochetNaMars Nov 7, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

у меня не работает Math.max для дат

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

О, давай через reduce. Типа пройти по всем датам и найти такую, которая больше других. Очевидно, что sort имеет сложность больше линейной - O(log(n) * n), а reduce будет O(n)

Comment thread robbery.js
return dates[0];
}

function findFreeGangsterTime(schedule, bankTimeZone) {

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Функция должна занимать не больше 25 строк

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Кажется, что можно сначала просчитать все leadToDataType(schedule[key][i].to, bankTimeZone) для каждого i, а потом заполнить ими словарь.

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

И попробуй использовать reduce

Comment thread robbery.js

function getGangFreeTime(schedule) {
var freeTime = [];
schedule.Danny.forEach(function (interval1) {

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Привязка к именам бандитов. И слишком большая вложенность. Такого быть не должно. Нельзя ли просто по ключам пробежаться?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1

так делать совсем нельзя - а если будет сколько угодно бандитов или имена могу быть другими?

Comment thread robbery.js
}

function formateDate(date) {
var day = date.getDate();

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Не проще ли сделать так: var day = WEEK[date.getDate()]?
Почему словарь называется WEEK? Почему метод getDate возвращает день?

Comment thread robbery.js
return res;
}

function leadDateToObject(date) {

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Что значит это название?)

Comment thread robbery.js
exports.isStar = false;

var WEEK = ['', 'ПН', 'ВТ', 'СР', 'ЧТ', 'ПТ', 'СБ', 'ВС'];
var BEGIN_OF_WEEK = new Date (0, 0, 1, 0, 0);

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ты уверена, что без этих волшебных дат тут не обойтись?

Comment thread robbery.js
}

function formateFreeTime(freeTime) {
var res = [];

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Лучше названия не сокращать

Comment thread robbery.js
var begin = maxDate (interval1.from, interval2.from, interval3.from);
var end = minDate (interval1.to, interval2.to, interval3.to);
if (end > begin) {
freeTime.push({

@chipolinka chipolinka Nov 7, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ты много где пушишь объект с этими полями. Можно вынести в отдельную функцию

Copy link
Copy Markdown

🚀

@Vittly Vittly left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

в целом неплохо, но:

  • есть косяки в названиях
  • где-то слишком многословно
  • совсем не используешь reduce

надо поправить, сорри за задержку - мой косяк не увидел отбивку. В следующий раз пиши в slack если буду тормозить

🍅

Comment thread robbery.js
return freeTime;
}

function formateFreeTime(freeTime) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

судя по смыслу, аргумент лучше назвать freeIntervals

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

а почему такое название функции? formatE ? Судя по коду, мы тут интервалы пересекаем с [ПН, СР] и интервалы разбиваем по дням. Это скорее некая нормализация - normalizeIntervals

Comment thread robbery.js
for (var i = day1; i <= Math.min(day2, 3); i++) {
res.push({
from: maxDate (new Date (0, 0, i, 0, 0), interval.from),
to: minDate (new Date (0, 0, i, 23, 59), interval.to)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

а что за даты такие? Чтобы стало понятнее, нужно сделать переменные с говорящими именами

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

мы тут пытаемся разложить interval на отрезки по дням? и в итоге будут теже интервалы разложенные по дням - так? forEach, который собирает некий результат - лучше писать через reduce

Comment thread robbery.js
var res = [];
freeTime.forEach(function (interval) {
var day1 = interval.from.getDate();
var day2 = interval.to.getDate();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

хм, возможно, лучше назвать fromDay и toDay.

Comment thread robbery.js
freeTime.forEach(function (interval) {
var day1 = interval.from.getDate();
var day2 = interval.to.getDate();
for (var i = day1; i <= Math.min(day2, 3); i++) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

типа не дальше среды?

Comment thread robbery.js
function maxDate() {
var dates = [].slice.call(arguments);
dates.sort(function (date1, date2) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

О, давай через reduce. Типа пройти по всем датам и найти такую, которая больше других. Очевидно, что sort имеет сложность больше линейной - O(log(n) * n), а reduce будет O(n)

Comment thread robbery.js
to: end
});
}
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

опять по смыслу reduce = проход по массиву и получение одного результата

Comment thread robbery.js
var bank = {};
freeTime.forEach(function (interval) {
var day = interval.from.getDate();
bank.start = new Date (0, 0, day, startBankWork.hours, startBankWork.minutes);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ты часто используешь конструкцию: new Date(0, 0, x, y, z) - кмк для нее лучше завести хелпер-функцию с говорящим именем, тогда такие вызовы будут не такими таинственными

Comment thread robbery.js
freeTime.forEach(function (interval) {
var day = interval.from.getDate();
bank.start = new Date (0, 0, day, startBankWork.hours, startBankWork.minutes);
bank.end = new Date (0, 0, day, endBankWork.hours, endBankWork.minutes);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

а зачем нам объект? можно было бы сделать две переменные - bankStart и bankEnd

Comment thread robbery.js
if (timeToRobbery[i].to - timeToRobbery[i].from >= msInDuration) {
res.push(timeToRobbery[i]);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

опять reduce нужен, смотри как коротко получается:

var intervals = timeToRobbery.reduce(function(res, interval) {
 if (interval.to - interval.from >= msInDuration) {
 res.push(interval);
 }
 return res;
}, []);

и в конце можно написать тернарник: return intervals.length === 0 ? nul : intervals;

Comment thread robbery.js
exists: function () {
return false;

return (timeForRobbery !== null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

можно без скобок

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

Reviewers

@Vittly Vittly Vittly requested changes

+1 more reviewer

@chipolinka chipolinka chipolinka left review comments

Reviewers whose approvals may not affect merge requirements

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /