Advanced Search

Results 1 to 5 of 5

Thread: Any suggestions on my code :)

  1. #1
    Join Date
    Feb 2007
    Location
    England
    Posts
    254
    Thanks
    0
    Thanked 5 Times in 5 Posts

    Default Any suggestions on my code :)

    Hello,

    I want to know what you think of my code, including design, functionality and coding (If you can be bothered to view my external JS, I know I'm not making this easy)

    Last time I asked it didn't work in Konqueror. I have only tested it in IE and FF.

    http://www.robertgarford.com/code/ra...1_2_strict.htm


    Many thanks to anyone who helps. Even if its not constructive!


  2. #2
    Join Date
    Jun 2005
    Location
    英国
    Posts
    11,878
    Thanks
    1
    Thanked 180 Times in 172 Posts
    Blog Entries
    2

    Default

    It works in Konqueror now. The design's nice, the functionality's good, but the code needs work.
    Code:
    //Browser detection
    var ie5=document.all&&document.getElementById;
    var ns6=document.getElementById&&!document.all;
    Argh! Don't use browser detection!
    Code:
    		document.all["radialMenu"].innerHTML = menuContent;
    innerHTML is non-standard and often produces unexpected results. Use DOM functions instead. The same applies to createContextualFragment().
    Code:
    			menuContent+="<a href=\""+tempAr[0]+"\" target=\""+tempAr[1]+"\" onmouseover=\""+rolloverTxtOn+scaleTxtOn+",goTo='"+tempAr[0]+"'\"
    You mean ; not , here.
    Code:
    <img id=\"id0\" width=\""+circle_menu_item_dimension+"px\" height=\""+circle_menu_item_dimension+"px\" src=\""+picDir+"blankbg.png\" style=\"border:0px; position:absolute; left:"+(parseInt((circle_menu_dimension/2)+(circle_menu_radius*Math.sin(y*min_angle_between_items)))-parseInt(circle_menu_item_dimension/2))+"px; top:"+(parseInt((circle_menu_dimension/2)-(circle_menu_radius*Math.cos(y*min_angle_between_items)))-parseInt(circle_menu_item_dimension/2))+"px;\" />
    You use the deprecated width and height properties (which don't take units except &#37;, by the way), even though you're using CSS anyway. You also use an XHTML-style self-closing tag, even though the script won't work on XHTML pages due to your use of innerHTML.
    Code:
    eventObj = e? e:event;
    Unnecessarily (and pointlessly, since the event will expire) global object. Could cause all sorts of problems. There are a lot of global functions in your code. You might want to consider putting the whole thing inside a namespacing object of some kind.
    Code:
    if(window.addEventListener)
    	{ // Mozilla, Netscape, Firefox
    	window.addEventListener('load', startup, false);
    	} 
    else 
    	{ // IE
    	window.attachEvent("onload", startup);
    	}
    What if neither exist?
    Code:
    //Event handlers
    function setNavType()
    	{
    	if (ie5||ns6)
    		{
    		menuobj=document.getElementById("radialMenu");	//Get menu div
    		menuobj.style.display='';							//Hide menu
    		document.oncontextmenu=function(){return false}; 	//Disable Browser Context Menus
    		
    		if(proNav)
    			{
    			document.onmousedown=showMenu;
    			document.onmouseup=function(){followLink(),hideMenu()};
    			}
    		else
    			{
    			document.onmousedown=showMenu;
    			document.onmouseup=function(){followLink()};
    			document.onclick=hideMenu;
    			}
    		}
    	}
    
    
    function startup()
    	{
    	setNavType();
    	displayEvenly();
    	hideMenu();
    	}
    So you rely on DOM events to attach to load, but overwrite any mouse events on the document anyway?
    Twey | I understand English | 日本語が分かります | mi jimpe fi le jbobau | mi esperanton komprenas | je comprends franšais | entiendo espa˝ol | t˘i Ýt hiểu tiếng Việt | ich verstehe ein bisschen Deutsch | beware XHTML | common coding mistakes | tutorials | various stuff | argh PHP!

  3. #3
    Join Date
    Feb 2007
    Location
    England
    Posts
    254
    Thanks
    0
    Thanked 5 Times in 5 Posts

    Default Hey Twey

    Firstly,
    Thankyou so much. Having someone like you look over my code is amazing.
    The next thing I was going to change was the event handling, but you are fantastic. I really didn't expect anyone to give that much constructive critism.

    Anyway enough praise. I will look at all your suggestions and make all the appropriate changes.

    I like being a regular here. If someone asked me to look at their code in a separate .js file I would just ask them to post the JS. Ah well, maybe I need to change my manner.

    Thanks again.

    Rob


  4. #4
    Join Date
    Jun 2005
    Location
    英国
    Posts
    11,878
    Thanks
    1
    Thanked 180 Times in 172 Posts
    Blog Entries
    2

    Default

    If someone asked me to look at their code in a separate .js file I would just ask them to post the JS.
    For me, at least, it's easier to debug running code. If someone pastes a JS file, I have to construct a working demo page before I can debug it thoroughly, so I prefer live demos.
    Twey | I understand English | 日本語が分かります | mi jimpe fi le jbobau | mi esperanton komprenas | je comprends franšais | entiendo espa˝ol | t˘i Ýt hiểu tiếng Việt | ich verstehe ein bisschen Deutsch | beware XHTML | common coding mistakes | tutorials | various stuff | argh PHP!

  5. #5
    Join Date
    Jul 2006
    Location
    Canada
    Posts
    2,581
    Thanks
    13
    Thanked 28 Times in 28 Posts

    Default

    I also try to use what the original script looked like, and to work around that. This will probably help the original poster see how their code was changed.
    - Mike

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •