Есть крутые JavaScript девелоперы? Сделайте код ревью
Немного о проекте.
Интерактивная карта свободных парковочных мест. Водители добавляют на карту различные метки, помогаю друг другу припарковаться.
Ссылка на страничку со скриптом: parkoomba.com/map
Подробнее о сервисе: dou.ua/forums/topic/8726
Технологии: knockoutjs, jquery, bootstrap, OpenStreetMaps, leafletjs, fontawesome
Отмазка на случай «плохого» кода.
С фронтендом работаю совсем немного. Это первый более-менее опыт работы с javascript, knockoutjs и т.д. C удовольствием выслушаю абсолютно любые замечание, даже незначительные.
Давайте писать каждое замечание отдельной веткой. Так будет легче трекать/обсуждать.
Заранее спасибо
19 коментарів
Додати коментар Підписатись на коментаріВідписатись від коментарівдодам ще
1. (obj.Type == 1) {
запихни в enum
2. htmlPopup = «Свободное парковочное место»
не фешн. заюзай лібу для глобалізації/локалізаіцї
3. if (obj.WantedOffset < 30) {
меджик намберс
зы: Прочитал, как делают енумы, поменяю
Меня очень смутили <link rel="stylesheet" внутри <body
Комментарии типа // deg2rad below — бесполезно, ведь в блокноте не программируем :)
В функции caclulateRadius идет какое-то мудреное вычисление, со странным форматирование.
Старайтесь все же названия переменных делать от 5 символов, используя camelCase или use_underscore
хм. но ведь в конце боди то. asp.net mvc из коробки предлагает рендерить именно там.
ой, это все какой-то бородатый копипаст:)ну ок, допустим. рендерить в футере?
с именованием согласен, надо навести порядок. Богдан тоде про это писал
стили надо бы все в 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.атрибут = .. и обработка асинхронных реджектов.
это все понятно, но как бы глядя на исходный код различных либ, я не могу утверждать что это общепринятая практика
там только точку с запятой в конце могут не ставить, что не позволяет их в бандлы собирать. Любая минифицированная либа без ; работать не будет.
А зачем в прототипы записывать атрибуты которые не есть функциями? Они же, в большинстве случаев, должны быть уникальными для каждого отдельного объекта
если это не методы, и не свойства наследуемые всеми экземплярами, то в этом ни смысла ни логики нету.
1) Выложите код куда-то.
2) Скрипт в глобальном скоупе (см module pattern, amd javascript)
3) _function createPlaces_ Можно было использовать $.get вместо $.ajax
и вместо
.done(function(data) { doneFunc(data); })вот такой код4) Нет обработки ошибок (того же ajax)
5) Большая функция createMarkerInfo (60 строк)
Дальше мелочи:
6) markers = new Array(); предпочтительнее использовать литералы
7) data.forEach — не будет работать в ИЕ8
8) Не стандартное именование (С большой буквы называют «классы»)
9) Надо бы почитать про «good parts javascript» и про «jshint/jslint»
10) Использование «классов» и нокаута — это чисто субъективный момент