Розробка · 12 грудня 2013, 12:43 1698

Oleksandr Gamaniuk, CEO в BOTMAKERS LLC

Есть крутые JavaScript девелоперы? Сделайте код ревью

Немного о проекте.
Интерактивная карта свободных парковочных мест. Водители добавляют на карту различные метки, помогаю друг другу припарковаться.
Ссылка на страничку со скриптом: parkoomba.com/map
Подробнее о сервисе: dou.ua/forums/topic/8726
Технологии: knockoutjs, jquery, bootstrap, OpenStreetMaps, leafletjs, fontawesome

Отмазка на случай «плохого» кода.
С фронтендом работаю совсем немного. Это первый более-менее опыт работы с javascript, knockoutjs и т.д. C удовольствием выслушаю абсолютно любые замечание, даже незначительные.

Давайте писать каждое замечание отдельной веткой. Так будет легче трекать/обсуждать.
Заранее спасибо

Теми: code review, HTML, JavaScript, програмування

👍ПодобаєтьсяСподобалось0

До обраногоВ обраному0

Facebook

Twitter

LinkedIn

Ctrl + Enter

Ctrl + Enter

YG SE 13.12.2013 14:44

додам ще
1. (obj.Type == 1) {
запихни в enum
2. htmlPopup = «Свободное парковочное место»
не фешн. заюзай лібу для глобалізації/локалізаіцї
3. if (obj.WantedOffset < 30) {
меджик намберс

Відповісти

Підтримати

Oleksandr Gamaniuk CEO в BOTMAKERS LLC 13.12.2013 15:52

1. (obj.Type == 1) {
запихни в enum

блин, никак не привыкну к тому, что в javascript изначально многого нет. все нужно строить самому.
зы: Прочитал, как делают енумы, поменяю

Відповісти

Підтримати

YG

Andrey Tkachenko Senior Backend Developer в Spryker Systems GmbH 13.12.2013 12:41

Меня очень смутили <link rel="stylesheet" внутри <body
Комментарии типа // deg2rad below — бесполезно, ведь в блокноте не программируем :)

В функции caclulateRadius идет какое-то мудреное вычисление, со странным форматирование.
Старайтесь все же названия переменных делать от 5 символов, используя camelCase или use_underscore

Відповісти

Підтримати

Oleksandr Gamaniuk CEO в BOTMAKERS LLC 13.12.2013 15:23

Меня очень смутили link rel="stylesheet" внутри body

хм. но ведь в конце боди то. asp.net mvc из коробки предлагает рендерить именно там.
ну ок, допустим. рендерить в футере?

Комментарии типа // deg2rad below — бесполезно, ведь в блокноте не программируем :)
В функции caclulateRadius идет какое-то мудреное вычисление, со странным форматирование.

ой, это все какой-то бородатый копипаст:)

с именованием согласен, надо навести порядок. Богдан тоде про это писал

Відповісти

Підтримати

Andrey Tkachenko

Andrey Tkachenko Senior Backend Developer в Spryker Systems GmbH 13.12.2013 15:29

стили надо бы все в head, иначе они будут грузиться после отображения страницы, меняя ее налету.

Відповісти

Підтримати

Oleksandr Gamaniuk

Oleksandr Gamaniuk CEO в BOTMAKERS LLC 13.12.2013 15:35

да, да. поленился видно когда-то. они используются только на это странице, поэтому немного сложнее рендерить

Відповісти

Підтримати

Andrey Tkachenko

anonymous 12.12.2013 22:55

начало не обнадежило:
var isFirstClick = true; function AddMarkersModel
в глобал скоуп...
оберните хотя бы в самовызывающеюся функцию, но лучше AMD посмотрите require.js
хорошей практикой считается установка use strict но это совсем не обязательно
разбейте на модули, а то как все поскидали «в кучу»
объявление переменных должно быть в начале функций, а инициализация может быть и позже

+ 1 ко всему что написал автор ниже

Відповісти

Підтримати

Oleksandr Gamaniuk CEO в BOTMAKERS LLC 13.12.2013 15:27

начну с простого, про остальное нужно читать, не совсем понимаю, что имлось ввиду.

объявление переменных должно быть в начале функций, а инициализация может быть и позже

Почему объявление вначале? Это так принято в javascript?

Відповісти

Підтримати

anonymous 13.12.2013 15:36

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

Відповісти

Підтримати

Oleksandr Gamaniuk

anonymous 13.12.2013 15:42

ох я и написал...
короче:
(function () {
var c = 1;

if (a === undefined) {
console.log(’var is existing!! but its equal to undefined’)
}

var a = 1;
if (a === c) {
console.log(’value was set for var’)
}

//this will throw exception
if (notExist === undefined) {
console.log(’var is existing!! but its equal to undefined’)

}
}())

Відповісти

Підтримати

Oleksandr Gamaniuk CEO в BOTMAKERS LLC 13.12.2013 15:49

Нет, нет, все в порядке. Я знаю про это.
Тут смысл в другом. Обычно объявляю переменные ближе к тому месту, где они используются. Так легче понимать и сапортить код.

Поэтому вопрос: это общепринятая практика или личные предпочтения?

Відповісти

Підтримати

anonymous 13.12.2013 16:05

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

Відповісти

Підтримати

Oleksandr Gamaniuk

Дмитро 13.12.2013 16:14

этот как точка запятая в джс рекоммендуется ее всегда ставить одними а другие выступают против этого

во времена минификаций всего и вся это уже общепринятая практика.

а еще хороший тон — экономия памяти браузера и и использование прототипов(prototype), для функций конструкторов, вместо this.атрибут = .. и обработка асинхронных реджектов.

Відповісти

Підтримати

anonymous 13.12.2013 16:18

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

Відповісти

Підтримати

Дмитро 13.12.2013 16:20

там только точку с запятой в конце могут не ставить, что не позволяет их в бандлы собирать. Любая минифицированная либа без ; работать не будет.

Відповісти

Підтримати

Vasyl Ruban 25.03.2014 22:52

А зачем в прототипы записывать атрибуты которые не есть функциями? Они же, в большинстве случаев, должны быть уникальными для каждого отдельного объекта

Відповісти

Підтримати

Дмитро 26.03.2014 08:53

если это не методы, и не свойства наследуемые всеми экземплярами, то в этом ни смысла ни логики нету.

Відповісти

Підтримати

Дмитро Сидоренко CTO в Jiffsy 14.12.2013 18:30

Почему объявление вначале? Это так принято в javascript?

Это тема холивара. Делайте так, как вам больше нравится. Нет единственно правильного подхода. Я склоняюсь к объявлению переменных как можно ближе к месту использования.

Відповісти

Підтримати

Oleksandr Gamaniuk

Bogdan Shyiak SE or Dev 12.12.2013 13:12

1) Выложите код куда-то.
2) Скрипт в глобальном скоупе (см module pattern, amd javascript)
3) _function createPlaces_ Можно было использовать $.get вместо $.ajax
и вместо

.done(function(data) {
        doneFunc(data);
    })

вот такой код

.done(doneFunc)

4) Нет обработки ошибок (того же ajax)
5) Большая функция createMarkerInfo (60 строк)
Дальше мелочи:
6) markers = new Array(); предпочтительнее использовать литералы
7) data.forEach — не будет работать в ИЕ8
8) Не стандартное именование (С большой буквы называют «классы»)
9) Надо бы почитать про «good parts javascript» и про «jshint/jslint»
10) Использование «классов» и нокаута — это чисто субъективный момент

Відповісти

Підтримати

Підписатись на коментарі

Ваша пошта

Не підписуватись