Оцените пожалуйста код.
Здравствуйте, уважаемые.
Недавно начал изучать JS. В программировании опыта нет совершенно, поэтому некоторые вещи очень здорово перегревали мозг. Но самой большой своей проблемой я считаю отсутствие возможности пообщаться оффлайн с практикующим профессионалом, показать свой код. Поэтому обращаюсь к знающим людям этого форума. Помогите, ткните носом в неточности, возможно я что-то делаю не так как было бы правильнее. В общем буду благодарен за любой совет. p.s. В одной из тем нашёл ссылку на jquery плагин (http://chrisiufer.com/loupe/), который советовали сделать на обычном JS. У меня как-бы получилось, но оценить качество кода возможности не имею. На CSS неточности просьба не обращать внимание, интересует только оценка реализации на JS. <!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=UTF-8">
<title>Прикольный виджет</title>
<style type="text/css">
.lupe {
border: gray 4px solid;
width: 200px;
height: 200px;
position: absolute;
background-repeat: no-repeat;
background-color: white;
border-radius: 200px;
}
</style>
</head>
<body>
<img id="image" src="http://4.bp.blogspot.com/-PaPJ6ulqi9c/Tf1-QMenB1I/AAAAAAAAADs/U_31Ap2LIjA/s1600/novye-interesnye-fotografii-ot-national-geographic-4.jpg" alt="" width="400">
<img id="image2" src="http://2.bp.blogspot.com/-Edrn-TiDZlw/UPha3ogDR1I/AAAAAAAAA9s/esDb0tWcCdA/s1600/DSC06442.JPG" alt="" width="400">
<script type="text/javascript">
var Imgt = function (id) {
if(!(this instanceof Imgt)) {
return new Imgt(id);
}
this.element = document.getElementById(id);
this.addEvent('mouseover', Imgt.createLupe);
this.addEvent('mouseout', Imgt.deleteLupe);
this.addEvent('mousemove', Imgt.shower);
};
Imgt.lupe = document.createElement('div');
Imgt.createLupe = function () {
var lupe = Imgt.lupe;
lupe.className = 'lupe';
document.getElementsByTagName('body')[0].appendChild(lupe);
lupe.style.backgroundImage = 'url('+ this.src + ')';
};
Imgt.deleteLupe = function () {
var lupe = Imgt.lupe;
lupe.parentNode.removeChild(lupe);
};
Imgt.shower = function (e) {
var lupe = Imgt.lupe,
event = e || window.event,
eX = event.x || event.clientX,
eY = event.y || event.clientY,
coords = this.getBoundingClientRect(),
y = -(eY - coords.top)*4+100 + 'px',
x = -(eX - coords.left)*4+100 + 'px';
lupe.style.top = 10 + eY + 'px';
lupe.style.left = 10 + eX + 'px';
lupe.style.backgroundPosition = x + ' ' + y;
};
Imgt.prototype.addEvent = function (event, handler) {
var element = this.element;
if(element.addEventListener) {
return element.addEventListener(event, handler);
}else if(element.attachEvent) {
return element.attachEvent('on' + event, handler);
}
};
var i = new Imgt('image');
var i2 = Imgt('image2');
</script>
</body>
</html>
Буду также признателен за советы из области как отрастить скил в JS... |
var element = this.element; В чём профит от этой переменной? Я бы понял если бы она использовалась в каком нибудь колбэке (т.е. чтобы в правильном контексте) или путь был бы слишком длинный (this.element.element2.element3.element4). А тут то зачем? Не существенно конечно, но лично меня это раздражает в коде. p.s.: смотрел только с 62 по 68 строку. |
newbie7,
хороший, понятный код! Только выделяйте блоки с переменными и функции переносом строки, а то иногда кашеца получается) Цитата:
Цитата:
|
Цитата:
1 coords = this.getBoundingClientRect() вычислять нужно 1 раз при заходе на элемент , а не всякий раз при движении мыши -данные статичны при всяком вычислении. 2 3 при каждом мове не стоит "перерисовывать" лупу . Оптимальная частота 30 -60 кадров в секунду . События движения мыши вызовутся гораздо в большем количестве чем эта частота , и при соответствующих условиях будет явно виден эффект торможения. Цитата:
|
l-liava-l,
по объёму кода не больше будет. минус одна лишняя строчка. я бы понял если бы он "e" вместо "element " написал. а так заменили: this.element на my_mega_element чем лучше то? точки нет? p.s.: 4 раза |
Цитата:
|
el. e на объект события похоже. я по крайней мере так его всегда именую
|
вообще если понятность не очень страдает то я экономлю место по вертикали и не создаю переменные которые используются 1, 2 раза. лучше чтобы код по вертикали был компактнее.
if(!(this instanceof Imgt)) {
return new Imgt(id);
}
this.element = document.getElementById(id);
тут явно строка пустая не нужна например. сверху от условия да, чтобы не сливалось можно пропустить. из-за таких лишних строк кроме одной ф-й ни чего на экране не видно, не видно как она связана с другим кодом. p.s.: хз, многим покажется мелочью и мб они будут правы, но мне глаз режет как то это всё последнее время. p.s.s: документационные комментарии все мои усилия сводят на нет |
Спасибо всем, кто откликнулся.
Цитата:
Imgt.shower = function (e,obj) {
var lupe = Imgt.lupe,
event = e || window.event,
eX = event.x || event.clientX,
eY = event.y || event.clientY,
y = -(eY - obj.coords.top)*4+100 + 'px',
x = -(eX - obj.coords.left)*4+100 + 'px';
lupe.style.top = 10 + eY + 'px';
lupe.style.left = 10 + eX + 'px';
lupe.style.backgroundPosition = x + ' ' + y;
};
Imgt.prototype.addEvent = function (event, handler) {
var self = this;
if(this.element.addEventListener) {
return this.element.addEventListener(event, function(e) {handler(e,self)});
}else if(this.element.attachEvent) {
return this.element.attachEvent('on' + event, function(e) {handler(e,self)});
}
};
По второму пункту мне не совсем понятно. Пробовал исправлять и виджет просто перестаёт работать. А с третьим так и вовсе тьма :help: По идее нужно замедлить сам обработчик события mousemove, чтобы не так часто выполнялась функция Imgt.shower, но как это сделать пока не знаю. Цитата:
Копипаста здесь нет. Я как и, наверно, любой другой новичок хотел найти на форумах кого-нибудь вроде куратора, но прочитав не одну такую тему понял что бесполезно. И решил попробовать просто выложить код на обсуждение. И до гения мне как до луны, потому как после некоторых тем просто мозги наружу :cray: |
пункт 2 .
я загнал -не глянул на очерёдность вычислений в коде. в любом случае там можно оптимизировать . Раскройте скобки -сведите всё к двум операциям. Здесь конечно прирост будет не заметен , но на сложных вычислениях результат будет заметен eY *4 и eX*4 -по сути дела изменяемые величины. требующие вычисления всё остальное в выражениях константы : obj.coords.top*4+100, obj.coords.left*4+100 да и obj -можно сделать закрытым свойством и обращаться к нему (либо просто свойством), а не предавать в обработчик лишним аргументом пункт 3 http://www.ozon.ru/context/detail/id/19106391/ некоторые его высказывания можно назвать спорными, но мозги в нужную строну поворачивает. Примеры там есть . А в обще, довольно странно, как вы всё смешали . Создаёте объекты, а обращаетесь к методам и свойствам функции из него переназначая их всякий раз при создании. А не к методам и свойствам объекта .... Подводя итог: читаемость -да форматирование -да архитектура -нет говнокод -да базовые оптимизации -нет читаемость и форматирование ничто в сравнении с неправильной архитектурой и говнокодом. Оптимизации не беру в расчёт Склоняюсь к жесткому копипасту -копирование правильных участков кода из литературы , без понимания архитектуры , взаимодействий и базовых знаний о том , что и как работает А вобще всё не так бы делал. собрал бы всё в модуль накинул бы наблюдатель ,и при заходе на изображение помеченное, классом , атрибутом ... включал бы лупу а не создавал бы явно для каждого изображения объект |
| Часовой пояс GMT +3, время: 10:21. |