2
\$\begingroup\$

I have code for googly eyes that is too long. Basically I have variables for each element pupils (one for right -other for left), eyesockets (left, right). I wish I could compress my code so I can call a function animateEye (eye).

  function animateEye(eye) {
          console.log(eye);
          var pupille = eye.find("div");

For now this is what I have and I have no idea how to continue.

This is the actual code:

var pupilleLinks = $('#pupilleLinks');
var pupilleRechts = $('#pupilleRechts');

//			var abstandH =  pupille.offset().left- e.pageX + pupille.height()/2;

$(document).on('mousemove', function(e){
    var abstandV1 = pupilleLinks.offset().top- e.pageY +pupilleLinks.height()/2;

    var pupilleL1 = pupilleLinks.position().top - abstandV1 ; // 10 = Breite Pupille / 2
    var augeLinks = $("#augeLinks")
    var distanceX=augeLinks.width()-pupilleLinks.width();
    var distanceY=augeLinks.height()-pupilleLinks.height();
    if(pupilleL1 > distanceY) { 
      (pupilleL1 = distanceY);
               }
    if(pupilleL1 < 0) { 
      (pupilleL1 = 0);
               }
    var abstandH1 = pupilleLinks.offset().left- e.pageX + pupilleLinks.width()/2;
    var pupilleL2 = pupilleLinks.position().left - abstandH1 ;
    if (pupilleL2 < 0) {
       (pupilleL2 = 0);
               }
    if(pupilleL2 > distanceX) {
      (pupilleL2 =distanceX);
               }
    pupilleLinks.css({
        left: pupilleL2+"px",
        top: pupilleL1+"px"
               });

    var abstandH2 = pupilleRechts.offset().left- e.pageX + pupilleRechts.width()/2;
    var pupilleR1 = pupilleRechts.position().left - abstandH2;
    var augeRechts = $("#augeRechts")
    var distanceX2=augeRechts.width()-pupilleRechts.width();
    var distanceY2=augeRechts.height()-pupilleRechts.height();
    if(pupilleR1 < 0) {
      (pupilleR1 = 0);
               }
    if(pupilleR1 > distanceX2) {
      (pupilleR1 = distanceX2);
               }
    var abstandV2 = pupilleRechts.offset().top- e.pageY + pupilleRechts.width()/2;
    var pupilleR2 = pupilleRechts.position().top - abstandV2;
    if(pupilleR2 < 0) {
      (pupilleR2 = 0);
               }
    if(pupilleR2 > distanceY2) {
      (pupilleR2 =distanceY2);
               }
    pupilleRechts.css({
            left: pupilleR1+"px",
            top: pupilleR2+"px"
               });

    });
.auge {
  position: relative;
  display: inline-block;
  width: 150px;
  height: 150px;
  border: 10px solid black;
}
.pupille {
  position: absolute;
  top: 50%;
  left: 50%;
  width: 30px;
  height: 30px;
  background-color: blue;
  border-radius: 15px;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<div id="augeLinks" class="auge">
  <div id="pupilleLinks" class="pupille"></div>
</div>
<div id="augeRechts" class="auge">
  <div id="pupilleRechts" class="pupille"></div>
</div>

\$\endgroup\$
  • \$\begingroup\$ Where does pupilleLinks get defined? \$\endgroup\$ – Gary Storey Jun 9 '15 at 15:31
  • \$\begingroup\$ What is eye? Is that a DOM element that you want to contain the "eyes"? Does pupileLinks ever move from the current position? \$\endgroup\$ – Gary Storey Jun 9 '15 at 16:13
  • \$\begingroup\$ pupileLinks and pupileRechts are never defined in your code. What are they? \$\endgroup\$ – Gary Storey Jun 9 '15 at 16:40
  • \$\begingroup\$ Your code actually wasn't far off. There was a stray } in var abstandV2 = pupilleRechts.offset().top- e.pageY + pupilleRechts.wid}th()/2; that broke the code — probably a pasting error. It's unusual for us to do this, but I've added a bit of HTML and CSS to make the demo — please adjust it to match your code. Welcome to Code Review. \$\endgroup\$ – 200_success Jun 9 '15 at 19:38
3
\$\begingroup\$

Programming in German results in an awkward mix of English and German identifiers, with conflicting capitalization conventions. I suggest sticking to English, though I will make an attempt to accommodate your language preference here.

The code is duplicated not only for each eye, but also for each axis, resulting in four copies. The copy-and-pasted code is hard to follow because you have pasted them in an order that is not quite parallel:

var abstandV1 = …;
var pupilleL1 = …;
var augeLinks = $("#augeLinks")
var distanceX=…;
var distanceY=…;
if …
var abstandH1 = …;
var pupilleL2 = …;
if …

A more logical order would be:

var augeLinks = $("#augeLinks");
var abstandLY = …;
var pupilleLY = …;
var distanceLY = …;
if …
var abstandLX = …;
var pupilleLX = …;
var distanceLX = …;
if …

Instead of using if-statements the enforce the lower and upper bounds, I suggest using Math.min() and Math.max().


Using the same code for the left and right eyes is easy: just make smarter use of jQuery selectors:

$('.pupille').each(function() {
    var $pupille = $(this);
    var $auge = $(this).closest('.auge');
    …
});

Writing one version of the code for both the horizontal and vertical axes is tricker. I suggest defining a horizontal and a vertikal object to extract the relevant attributes.

var horizontal = {
    groesse:  function($el) { return $el.width(); },
    position: function($el) { return $el.position().left; },
    offset:   function($el) { return $el.offset().left; },
    cursor:   function(event) { return event.pageX; },
};
var vertikal = {
    groesse:  function($el) { return $el.height(); },
    position: function($el) { return $el.position().top; },
    offset:   function($el) { return $el.offset().top; },
    cursor:   function(event) { return event.pageY; },
};

function koordinate($pupille, $auge, cursor, achse) {
    var augeGroesse = achse.groesse($auge) - achse.groesse($pupille);
    var abstand = achse.cursor(cursor) +
                  achse.position($pupille) -
                  achse.offset($pupille) -
                  achse.groesse($pupille) / 2;
    return Math.max(0, Math.min(augeGroesse, abstand));
}

$(document).on('mousemove', function(event) {
    $('.pupille').each(function() {
        var $pupille = $(this);
        var $auge = $(this).closest('.auge');
        $pupille.css({
            left: koordinate($pupille, $auge, event, horizontal) + 'px',
            top:  koordinate($pupille, $auge, event, vertikal) + 'px',
        });
    });
});
.auge {
  position: relative;
  display: inline-block;
  width: 150px;
  height: 150px;
  border: 10px solid black;
}
.pupille {
  position: absolute;
  top: 50%;
  left: 50%;
  width: 30px;
  height: 30px;
  background-color: blue;
  border-radius: 15px;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<div id="augeLinks" class="auge">
  <div id="pupilleLinks" class="pupille"></div>
</div>
<div id="augeRechts" class="auge">
  <div id="pupilleRechts" class="pupille"></div>
</div>

\$\endgroup\$
  • \$\begingroup\$ Oh my Flying spagetty monster!!! I readjusted my code as you posted and it works like heaven. I am forever grateful to you, I really am. \$\endgroup\$ – Daria M Jun 10 '15 at 6:11
  • \$\begingroup\$ 200_success, I know I said thank you before but really, you are a true mentor. I just got started with programming and the explanations I gor just made me more confused. Your answer was so logic and so easy to understand, it just made me not give up with programming(I was on the verge of doing so). Just want to let you know I am grateful people like you exist \$\endgroup\$ – Daria M Jun 10 '15 at 8:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.