CODEWITHSUNDEEP
javascript
Jeff
Jeff

Reputation: 6110

My first JavaScript. Any errors?

Good Sunday morning!

After fiddling around with some JavaScript this weekend, I finally wrote my first script. It is a Tooltip generator. It works great in FF 3 + 3.5, IE 6 + 7, Safari 4, Chrome 2 + 3, and Opera 9 + 10.

Before putting my new code into production, I thought I'd ask if you see any glaring problems -- or maybe it's perfect as-is? (Lol)

JSLint.com reports:

Error:

Problem at line 10 character 15: 'e' is already defined.

    var e = window.event;

Implied global: window 10, document 19,20,22,24,33,35,41,43,49,55

Here is my code (apologies in advance if you don't like my code style):

function getmouseposition( e )
{
    var offx = 22;
    var offy = 14;
    var posx = 0;
    var posy = 0;

    if ( !e )
    {
        var e = window.event;
    }
    if ( e.pageX || e.pageY )
    {
        posx = e.pageX;
        posy = e.pageY;
    }
    else if ( e.clientX || e.clientY )
    {
        posx = e.clientX
               + document.body.scrollLeft
               + document.documentElement.scrollLeft;
        posy = e.clientY
               + document.body.scrollTop
               + document.documentElement.scrollTop;
    }
    if ( document.getElementById )
    {
        var tooltip = document.getElementById( 'tooltip' );

        tooltip.style.left = ( posx + offx ) + 'px';
        tooltip.style.top = ( posy + offy ) + 'px';
    }
}

function tooltip_on( text )
{
    if( !document.getElementById( 'tooltip' ) )
    {
        var span = document.createElement( 'span' );

        span.id = 'tooltip';
        span.style.display = 'none';
        span.innerHTML = ' ';

        document.body.appendChild( span );
    }
    var tooltip = document.getElementById( 'tooltip' );

    tooltip.innerHTML = text;
    tooltip.style.display = 'block';
    tooltip.style.position = 'absolute';

    document.onmouseover = getmouseposition;
    // document.onmousemove = getmouseposition;
}

function tooltip_off()
{
    document.getElementById( 'tooltip' ).style.display = 'none';
}

EDIT:

I'm also wondering, what does this line say (in layman's terms ) in the getmouseposition function:

if ( document.getElementById )

Upvotes: 0

Views: 470

Answers (8)

austin cheney
austin cheney

Reputation:

To solve the JSLint problem related to the implied global... document check the assume a browser option. The syntax looks good, except I would try to declare all your variables using a single var command and a comma-separated list at the top of your function. It's a bit more efficient that way and easier to maintain/read.

You are doing well for your first attempt and I hope this works out for you.

Upvotes: 0

Kyle B.
Kyle B.

Reputation: 5787

You have e as a parameter to your function and also declared via var e = inside your function. When a parameter is passed to your function it already exists as that name, and declaring a variable with the same name will cause an error. I would change either the name of the variable declared, the name of the parameter or assign it properly if that is the case.

Upvotes: 1

crunchdog
crunchdog

Reputation: 13468

if ( document.getElementById )

This check is to detect if the function getElementById is defined for the document object for the browser currently used. If the user uses a browser that does not support this function, you don't want to get an error in the browser, but rather handle the case.

Upvotes: 0

Christian C. Salvadó
Christian C. Salvadó

Reputation: 827276

In response to your edit, that if is checking if the getElementById function is available, in now days, it's safe to assume that this function is available.

Rather than checking for the function, you could check if the element was found:

var tooltip = document.getElementById('tooltip');

if (tooltip) {
  tooltip.style.left = ( posx + offx ) + 'px';
  tooltip.style.top = ( posy + offy ) + 'px';
}

Your getmouseposition function, can be simplified: 

function getmouseposition( e ) { 
  var offx = 22, 
      offy = 14, 
      posx = 0, 
      posy = 0, tooltip; 

  e = e || window.event; 
  posx = e.pageX || (e.clientX 
                    + document.body.scrollLeft 
                    + document.documentElement.scrollLeft); 

  posy = e.pageY || (e.clientY  
                    + document.body.scrollTop 
                    + document.documentElement.scrollTop); 

  if (tooltip = document.getElementById('tooltip')) { 
    tooltip.style.left = ( posx + offx ) + 'px'; 
    tooltip.style.top = ( posy + offy ) + 'px'; 
  } 
}

Upvotes: 0

Kenan Banks
Kenan Banks

Reputation: 211962

That jslint error means that the variable e is already defined within the scope of the getmouseposition function by virtue of being an argument to said function. There is no need to redeclare it, so just remove the var from that line.

Second, explicitly accessing properties of the window and document objects is pretty standard. I wouldn't change that part of your code. Not sure why jslint even mentions those lines.

Very picky style police will complain about spaces just within parentheses:

// Most do this:
function myFunction(param){
   // Statement 1
   // Statement 2
   // Statement 3
}

// Not this
function myFunction( param )
{
   // Statement 1
   // Statement 2
   // Statement 3
}

And very very picky style dorks will tell you to use a single var declaration at the top of your function:

// Do this
function getmouseposition(e){
    var offx = 22,
        offy = 14,
        posx = 0,
        posy = 0;


// Not this
function getmouseposition(e)
{
    var offx = 22;
    var offy = 14; 
    var posx = 0;
    var posy = 0;

Lastly, most style-conscious javascript programmers will use camel case for functions, so getMousePosition instead of getmouseposition

My opinion is that, besides removing the var declaration as I suggested at the top, your code is quite readable and decent style, as long as you stick to the format consistently.

Upvotes: 2

VoteyDisciple
VoteyDisciple

Reputation: 37803

Yes. There are always errors, even if nobody spots them.

However, in this particular case the report you gave is right. Where you have:

var e = window.event;

That's creating a new e variable, not replacing the e parameter you've already defined. Change it to:

e = window.event;

Upvotes: 2

ChaosPandion
ChaosPandion

Reputation: 78262

I would recommend you follow the function name convention. It makes life a lot easier for those who will maintain your code. 

function myMethodName(myParam) {
    // Code
}

Upvotes: 1

p4bl0
p4bl0

Reputation: 3906

for the error : since e is an argument of your function, it already exists, so you don't need to declare it, just assign a value to it if its null or undefined.

Just remove the "var" part of the concerned line :

if ( !e ) e = window.event;

Upvotes: 4

Related Questions