10.03.2013, 10:59
|
 |
Новичок на форуме
|
|
Регистрация: 15.10.2012
Сообщений: 5
|
|
|
Оцените пожалуйста код.
Здравствуйте, уважаемые.
Недавно начал изучать 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...
|
|
10.03.2013, 13:04
|
 |
Профессор
|
|
Регистрация: 05.06.2009
Сообщений: 1,703
|
|
var element = this.element;
В чём профит от этой переменной?
Я бы понял если бы она использовалась в каком нибудь колбэке (т.е. чтобы в правильном контексте) или путь был бы слишком длинный (this.element.element2.element3.element4). А тут то зачем? Не существенно конечно, но лично меня это раздражает в коде.
p.s.: смотрел только с 62 по 68 строку.
__________________
Лучше установить FreeBSD, чем потратить 30 лет на Linux'ы и выяснить какой из них хуже.
Самые главные в жизни вещи - не вещи! (было написано на одном гараже =)
Последний раз редактировалось Tim, 10.03.2013 в 13:08.
|
|
12.03.2013, 12:05
|
|
Профессор
|
|
Регистрация: 14.03.2012
Сообщений: 1,808
|
|
newbie7,
хороший, понятный код! Только выделяйте блоки с переменными и функции переносом строки, а то иногда кашеца получается)
|
Цитата:
|
var element = this.element;
В чём профит от этой переменной?
|
в понятности и сокращении кода, ему что там this.element 5 раз писать?)
|
Цитата:
|
|
Буду также признателен за советы из области как отрастить скил в JS...
|
Просто побольше пиши, я вот уже неделю не пишу и чувствую что деградирую, хотя и без того балбес)
__________________
Научу себя плохому 
Последний раз редактировалось l-liava-l, 12.03.2013 в 12:08.
|
|
12.03.2013, 13:05
|
|
х.з
|
|
Регистрация: 21.11.2010
Сообщений: 4,588
|
|
|
Сообщение от newbie7
|
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;
};
|
Ужасный участок .
1 coords = this.getBoundingClientRect() вычислять нужно 1 раз при заходе на элемент , а не всякий раз при движении мыши -данные статичны при всяком вычислении.
2 (eY - coords.top)*4+100 (eY - coords.top)*400
3 при каждом мове не стоит "перерисовывать" лупу . Оптимальная частота 30 -60 кадров в секунду . События движения мыши вызовутся гораздо в большем количестве чем эта частота , и при соответствующих условиях будет явно виден эффект торможения.
|
Сообщение от newbie7
|
|
Недавно начал изучать JS. В программировании опыта нет совершенно
|
и конструкторы и прототипы и Dom + фишки  Либо практически чистый копипаст, либо гений ,либо автор топика лукавит
Последний раз редактировалось dmitriymar, 12.03.2013 в 13:08.
|
|
12.03.2013, 13:29
|
|
Профессор
|
|
Регистрация: 05.06.2009
Сообщений: 1,703
|
|
|
l-liava-l,
по объёму кода не больше будет. минус одна лишняя строчка. я бы понял если бы он "e" вместо "element " написал.
а так заменили:
this.element
на
my_mega_element
чем лучше то? точки нет?
p.s.: 4 раза
__________________
Лучше установить FreeBSD, чем потратить 30 лет на Linux'ы и выяснить какой из них хуже.
Самые главные в жизни вещи - не вещи! (было написано на одном гараже =)
|
|
12.03.2013, 14:27
|
|
Профессор
|
|
Регистрация: 14.03.2012
Сообщений: 1,808
|
|
|
Цитата:
|
по объёму кода не больше будет. минус одна лишняя строчка. я бы понял если бы он "e" вместо "element " написал.
а так заменили:
this.element
на
my_mega_element
чем лучше то? точки нет?
p.s.: 4 раза
|
угу, лучше бы e или el
__________________
Научу себя плохому
|
|
12.03.2013, 14:51
|
|
Профессор
|
|
Регистрация: 05.06.2009
Сообщений: 1,703
|
|
|
el. e на объект события похоже. я по крайней мере так его всегда именую
__________________
Лучше установить FreeBSD, чем потратить 30 лет на Linux'ы и выяснить какой из них хуже.
Самые главные в жизни вещи - не вещи! (было написано на одном гараже =)
|
|
12.03.2013, 14:58
|
|
Профессор
|
|
Регистрация: 05.06.2009
Сообщений: 1,703
|
|
вообще если понятность не очень страдает то я экономлю место по вертикали и не создаю переменные которые используются 1, 2 раза. лучше чтобы код по вертикали был компактнее.
if(!(this instanceof Imgt)) {
return new Imgt(id);
}
this.element = document.getElementById(id);
тут явно строка пустая не нужна например. сверху от условия да, чтобы не сливалось можно пропустить.
из-за таких лишних строк кроме одной ф-й ни чего на экране не видно, не видно как она связана с другим кодом.
p.s.: хз, многим покажется мелочью и мб они будут правы, но мне глаз режет как то это всё последнее время.
p.s.s: документационные комментарии все мои усилия сводят на нет
__________________
Лучше установить FreeBSD, чем потратить 30 лет на Linux'ы и выяснить какой из них хуже.
Самые главные в жизни вещи - не вещи! (было написано на одном гараже =)
|
|
12.03.2013, 23:22
|
|
Новичок на форуме
|
|
Регистрация: 15.10.2012
Сообщений: 5
|
|
Спасибо всем, кто откликнулся.
|
Сообщение от dmitriymar
|
Ужасный участок .
1 coords = this.getBoundingClientRect() вычислять нужно 1 раз при заходе на элемент , а не всякий раз при движении мыши -данные статичны при всяком вычислении.
2 (eY - coords.top)*4+100 (eY - coords.top)*400
3 при каждом мове не стоит "перерисовывать" лупу . Оптимальная частота 30 -60 кадров в секунду . События движения мыши вызовутся гораздо в большем количестве чем эта частота , и при соответствующих условиях будет явно виден эффект торможения.
|
Первый пункт исправил. Сам объект coords поместил в конструктор.
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)});
}
};
По второму пункту мне не совсем понятно. Пробовал исправлять и виджет просто перестаёт работать.
А с третьим так и вовсе тьма  По идее нужно замедлить сам обработчик события mousemove, чтобы не так часто выполнялась функция Imgt.shower, но как это сделать пока не знаю.
|
Сообщение от dmitriymar
|
|
и конструкторы и прототипы и Dom + фишки Либо практически чистый копипаст, либо гений ,либо автор топика лукавит
|
В JS начал разбираться примерно в октябре 2012, но после того как чуть с ума не сошёл от рекурсии в функции вычисления факториала закинул. Вторая попытка началась с середины декабря, когда пришла книга Флэнэгана и пока продолжается (надеюсь и не закончится  ). Возможно для опытного программиста, которому достаточно спецификацию прочитать, это и вечность, но для меня, с дипломом инженера-гидротехника, это недавно.
Копипаста здесь нет. Я как и, наверно, любой другой новичок хотел найти на форумах кого-нибудь вроде куратора, но прочитав не одну такую тему понял что бесполезно. И решил попробовать просто выложить код на обсуждение.
И до гения мне как до луны, потому как после некоторых тем просто мозги наружу 
|
|
13.03.2013, 00:11
|
|
х.з
|
|
Регистрация: 21.11.2010
Сообщений: 4,588
|
|
пункт 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/
некоторые его высказывания можно назвать спорными, но мозги в нужную строну поворачивает. Примеры там есть .
А в обще, довольно странно, как вы всё смешали .
Создаёте объекты, а обращаетесь к методам и свойствам функции из него переназначая их всякий раз при создании. А не к методам и свойствам объекта ....
Подводя итог: читаемость -да
форматирование -да
архитектура -нет
говнокод -да
базовые оптимизации -нет
читаемость и форматирование ничто в сравнении с неправильной архитектурой и говнокодом. Оптимизации не беру в расчёт
Склоняюсь к жесткому копипасту -копирование правильных участков кода из литературы , без понимания архитектуры , взаимодействий и базовых знаний о том , что и как работает
А вобще всё не так бы делал.
собрал бы всё в модуль
накинул бы наблюдатель ,и при заходе на изображение помеченное, классом , атрибутом ... включал бы лупу
а не создавал бы явно для каждого изображения объект
Последний раз редактировалось dmitriymar, 13.03.2013 в 12:14.
|
|
|
|
