Принимайте участие в зарплатном опросе! Уже собрано почти 8 000 анкет.

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

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

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

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

LinkedIn
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter

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

1. (obj.Type == 1) {
запихни в enum
блин, никак не привыкну к тому, что в javascript изначально многого нет. все нужно строить самому.
зы: Прочитал, как делают енумы, поменяю

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

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

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

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

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

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

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

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

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

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

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

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

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

ох я и написал...
короче:
(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’)

}
}())

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

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

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

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

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

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

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

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

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

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

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) Использование «классов» и нокаута — это чисто субъективный момент

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