Is this good or bad code?

  • (3 Pages)
  • +
  • 1
  • 2
  • 3

30 Replies - 4632 Views - Last Post: 22 January 2013 - 05:30 AM

#16 Dormilich   User is offline

  • 痛覚残留
  • member icon

Reputation: 4303
  • View blog
  • Posts: 13,677
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 17 January 2013 - 08:28 AM

though, you can still use window and gain nothing (while the original uses the faster locals no matter what).
Was This Post Helpful? 0
  • +
  • -

#17 andrewsw   User is offline

  • no more Mr Potato Head
  • member icon

Reputation: 6957
  • View blog
  • Posts: 28,696
  • Joined: 12-December 12

Re: Is this good or bad code?

Posted 17 January 2013 - 08:56 AM

View PostDormilich, on 17 January 2013 - 08:28 AM, said:

though, you can still use window and gain nothing (while the original uses the faster locals no matter what).


I thought that was what you were hinting at :) - it's easy to type window by mistake.
Was This Post Helpful? 0
  • +
  • -

#18 quim   User is offline

  • D.I.C Head
  • member icon

Reputation: 18
  • View blog
  • Posts: 182
  • Joined: 11-December 05

Re: Is this good or bad code?

Posted 17 January 2013 - 12:13 PM

Quote

The problem caused by scripts is that they block parallel downloads. The HTTP/1.1 specification suggests that browsers download no more than two components in parallel per hostname. If you serve your images from multiple hostnames, you can get more than two downloads to occur in parallel. While a script is downloading, however, the browser won't start any other downloads, even on different hostnames.
In some situations it's not easy to move scripts to the bottom. If, for example, the script uses document.write to insert part of the page's content, it can't be moved lower in the page. There might also be scoping issues. In many cases, there are ways to workaround these situations.
An alternative suggestion that often comes up is to use deferred scripts. The DEFER attribute indicates that the script does not contain document.write, and is a clue to browsers that they can continue rendering. Unfortunately, Firefox doesn't support the DEFER attribute. In Internet Explorer, the script may be deferred, but not as much as desired. If a script can be deferred, it can also be moved to the bottom of the page. That will make your web pages load faster.


Source: http://developer.yah....html#js_bottom


@Dormilich – Answer to your points.

Quite debatable?
* There usually is no unneeded script (unless the developer screwed up)
WRONG: yes, there are unneeded scripts at the beginning.
Most of the times we don’t need jquery or other script before the DOM is loaded.
This is exactly why we use

$(document).ready(function(){
// Please do something when the DOM is ready, because I am completely useless otherwise.
});


But wouldn’t you want to avoid using a function if you don’t need it?

This is an example of a script you need at the TOP of the page:
If you are using HTML5 and you want to detect if the browser has implemented a feature you will want to use.
Html5shiv or Modernizr


* Browser caching
FINE: Browser sees the URL and checks the cache and says on wait I don’t have to do this request. This is still valid if you put the script at the end of the page, only this time the browser will not do this check until the page is done loading thus saving some millisecond from the startup time.

* If your script needs some seconds to load, you have other problems (or in other words, scripts usually load way faster than images)
ANSWER: No matter what the loading time of any scrip or any file at all will be > than 0ms (millisecond), even if the scrip is empty.
* Modern browsers can download in parallel
YES, TRUE notices how you said modern browsers.
1. No everyone uses modern browsers
2. Still for us that use modern browsers, there is a limit of how many parallel request a browser can perform.


* transfer compression (i.e. gzip deflate)
ANSWER: Still holds true if you put the scrip at the button of the page.

All in all, you don’t have to take my word for this.

Do this test:

Clean the cache of your browser
Make a simple website with the script (jQuery) on top and run it check the loading time

Clean the cache again, using the same website now with the script at the button run it and check the loading time.

*Uses the browser tools to check the loading time
Was This Post Helpful? 0
  • +
  • -

#19 JD.CoolPenguin   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 05-May 12

Re: Is this good or bad code?

Posted 18 January 2013 - 03:07 PM

Hello again guys,

I was wandering if anyone could enlighten me a bit further about something Dormilich said

Quote

Quote

Sometimes global variables must be necessary though? For example the strt variable can't be declared inside playSlide() because that function will be accessed multiple times and I want the value of strt to increment +1 each time.


5)Sometimes they are convenient, but they are never necessary. and should the use of globals be not avoidable, use as less as possible (i.e. one (!) global, cf. "namespace"). other keywords here are Objects and Closures.


First of all, I see no way that strt can be declared locally as apposed to globally, but I'm very new at this, so is there a way I do not know of?
Secondly what does he mean by

Quote

one (!) global, cf. "namespace"
?
Thirdly, does anyone have a link (or can explain themselves) to something that can help me understand objects, closures and prototypes.
(Please don't say Google it. I have, and I got plenty of results, and I've read several of them, I'm asking if anyone can recommend something they believe is particularly helpful, as a lot of the google results have been difficult to understand.)

Thanks,
JD.
Was This Post Helpful? 0
  • +
  • -

#20 andrewsw   User is offline

  • no more Mr Potato Head
  • member icon

Reputation: 6957
  • View blog
  • Posts: 28,696
  • Joined: 12-December 12

Re: Is this good or bad code?

Posted 18 January 2013 - 03:28 PM

You can prevent globals by including all of your code within an immediately invoked function expression IIFE:

(function() {
    // all your code here
    var var1;
    function f1() {
        if(var1){...}
    }

    window.var_name = something; //<- if you have to have global var
    window.glob_func = function(){...} //<- ...or global function
})();


Added: It is the brackets at the end ( ) that invoke (call) this function.

[This is, also, a closure.]

If this code starts with

var myName = (function() {


then there is one global added called myName, which acts as a namespace (a container) for all your JS code. In jQuery this is the jQuery object, usually the dollar-sign $ (or YUI with Yahoo).

I can't recall good references at the moment; someone else might supply.

This post has been edited by andrewsw: 18 January 2013 - 03:35 PM

Was This Post Helpful? 0
  • +
  • -

#21 JD.CoolPenguin   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 05-May 12

Re: Is this good or bad code?

Posted 18 January 2013 - 06:01 PM

Thanks Adrewsw, that helps a lot!
I actually found a way of just reordering the code I had to avoid using any globals other than the functions themselves.
I have also implemented JSON_encode as per Dormilich's advice.
It's working well.
It's not perfect, as I'm still having a few issues with the imgSize function that I didn't have with the code the way it was before, which is definitely confuzzling, if it continues to be confusing, I will create a new thread for help, but I'm going to keep trying to work out for a while yet.

Anyway, here is the code now (excluding the imgSize function).
This is a PHP file and I have used <?php include 'functions.php'; ?> in the page.
<?php
$directory = "images/featured/";
$images = glob($directory . "*.*");
?>
<script>	
	var playSlide = function(){
		var imgArr = <?= json_encode($images) ?>;
		var len = imgArr.length-1;
		var strt = Math.floor(Math.random()*len);
		$("#bkgr img").attr("src",imgArr[strt]);
		strt++;
		
		setInterval(function(){
		$("#bkgr img").attr("src",imgArr[strt]);
		if (strt < len)
		   {
		   strt++;
		   }
		else
		   {
		   strt = 0;
		   }
		imgSize();
		}, 3000);
	};
	
$(window).load(function(){
	imgSize();
	playSlide();
});

</script>

Was This Post Helpful? 0
  • +
  • -

#22 andrewsw   User is offline

  • no more Mr Potato Head
  • member icon

Reputation: 6957
  • View blog
  • Posts: 28,696
  • Joined: 12-December 12

Re: Is this good or bad code?

Posted 18 January 2013 - 06:17 PM

Hello. The idea is that you would put ALL of your JS code in the IIFE, including your playSlide and imgSize functions. As you are using jQuery you can put them in window.load [I believe; I should double-check this statement.]

What this means is that you can no longer use event-attributes, such as:

<button onclick="doThis()">Go</button>

Events would need to be attached to elements using addEventListener (or attachEvent for older IE) within the same closure (our IIFE) so that the event-functions are available to be called; that is, within the same scope.

addEventListener is how events should be attached anyway, removing inline Javascript: separation of concerns/unobtrusive Javascript.

You could post your imgSize() function if you want someone to have a look.

Andy.

This post has been edited by andrewsw: 18 January 2013 - 06:19 PM

Was This Post Helpful? 0
  • +
  • -

#23 andrewsw   User is offline

  • no more Mr Potato Head
  • member icon

Reputation: 6957
  • View blog
  • Posts: 28,696
  • Joined: 12-December 12

Re: Is this good or bad code?

Posted 18 January 2013 - 06:23 PM

Added: I think I would put window.load inside my, separate, IIFE, rather than the other way round.
Was This Post Helpful? 0
  • +
  • -

#24 Dormilich   User is offline

  • 痛覚残留
  • member icon

Reputation: 4303
  • View blog
  • Posts: 13,677
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 19 January 2013 - 01:16 AM

@andrew: an IIFE is not a Closure per se. though it has the potential to be one, almost always it is not.
Was This Post Helpful? 0
  • +
  • -

#25 JD.CoolPenguin   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 05-May 12

Re: Is this good or bad code?

Posted 21 January 2013 - 02:13 PM

What's wrong with doing this?
	//rotater
	var rotate = funtion() {
		setInterval(function() {
			$("#slides li").eq(currents).fadeTo(1000, 0.0, function() {
				$("#slides li").eq(currents).removeClass("crnt");
				$("#slides li").eq(nexts).removeClass("nxt").addClass("crnt");
				currents+1;
				nexts+1;
				$("#slides li").eq(nexts).addClass("nxt");
			});
		}, 5000);
	};
	
	rotate();

Chrome console says "Unexpected Identifier" and points to setInterval(function() {

Thanks,
JD.
Was This Post Helpful? 0
  • +
  • -

#26 JD.CoolPenguin   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 05-May 12

Re: Is this good or bad code?

Posted 21 January 2013 - 02:21 PM

Added:
I tried this first:
		var rotate = setInterval(function() {
			$("#slides li").eq(currents).fadeTo(1000, 0.0, function() {
				$("#slides li").eq(currents).removeClass("crnt");
				$("#slides li").eq(nexts).removeClass("nxt").addClass("crnt");
				currents+1;
				nexts+1;
				$("#slides li").eq(nexts).addClass("nxt");
			});
		}, 5000);

Chrome console says: "number is not a function " and points to the rotate() call.

EDIT: Woo. I can edit now :)

This post has been edited by JD.CoolPenguin: 21 January 2013 - 02:23 PM

Was This Post Helpful? 0
  • +
  • -

#27 Dormilich   User is offline

  • 痛覚残留
  • member icon

Reputation: 4303
  • View blog
  • Posts: 13,677
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 21 January 2013 - 02:37 PM

issue #1: "funtion" is not a Java​Script keyword. "function" is.

issue #2: setInterval() returns a number that identifies the queue (for clearInterval()). that obviously is not a function.
Was This Post Helpful? 1
  • +
  • -

#28 JD.CoolPenguin   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 05-May 12

Re: Is this good or bad code?

Posted 21 January 2013 - 02:43 PM

Damn... a typo?
That's been bugging me for hours! Thanks again Dormilich!!
Was This Post Helpful? 0
  • +
  • -

#29 Dormilich   User is offline

  • 痛覚残留
  • member icon

Reputation: 4303
  • View blog
  • Posts: 13,677
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 21 January 2013 - 11:56 PM

View PostJD.CoolPenguin, on 21 January 2013 - 10:43 PM, said:

That's been bugging me for hours!

looks like you need an editor with tag highlighting. as you can see in the code block here, "function" is bold and blue while "funtion" is plain black.
Was This Post Helpful? 0
  • +
  • -

#30 JD.CoolPenguin   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 27
  • Joined: 05-May 12

Re: Is this good or bad code?

Posted 22 January 2013 - 05:13 AM

Yeah, I'd say I do!!!

(I only really did css and html before, and I didn't need it for that, never had any problems, but looks like I'm going to need one for work with JS).

Okay, so here's what I've got now. My only problem is, because I've put it in a containing function, I can't just call imgSize(); in $(window).resize(function() {

I've seen people using methods for this. So instead of having slideShow as a function, it would be a method, then I would call it like slideShow.imgSize or something like that. Except for I have no idea how to do any of this. Anyone help me out? Or point me to a tutorial on the matter? I'm still looking around, might work it out myself eventually, but your help is all greatly appreciated!

functions.php
<?php
$directory = "images/featured/";
$images = glob($directory . "*.*");
?>
<script>
$(function(){

var slideShow = function(){

	//write slides
	var imgArr = <?= json_encode($images) ?>;
	var len = imgArr.length - 1;

	for (var i=0;i<imgArr.length;i++)
		{
		$("#slides").append("<li><img src='" + imgArr[i] + "' id='img" + i + "'></li>");
		}

	//set start and next
	var strt = Math.floor(Math.random()*len);
	var currentS = strt;
	var nextS = (strt<len) ? strt+1 : 0;
	
	//fade in first slide set next slide
	$("#slides li").eq(strt).fadeTo(2000, 1.0, function() {
		$("#slides li").eq(currentS).addClass("crnt").end();
		$("#slides li").eq(nextS).addClass("nxt").css("opacity","1.0").end();
	});
	
	//rotater
	var rotate = function() {
		setInterval(function() {
			$("#slides li").eq(currentS).css("opacity","1.0").fadeTo(2000, 0.0, function() {
				$("#slides li").eq(currentS).removeClass("crnt").end();
				$("#slides li").eq(nextS).removeClass("nxt").end();
				$("#slides li").eq(nextS).addClass("crnt").end();
				currentS = (currentS<len) ? currentS+1 : 0;
				nextS = (nextS<len) ? nextS+1 : 0;
				$("#slides li").eq(nextS).addClass("nxt").css("opacity","1.0").end();
			});
		}, 5000);
	};
	
	//resizer
	var imgSize = function(){
	
		$("#slides li").each(function(){
			var theimg = $(this).find("img");
			$(theimg).load(function(){
				var imgrat = theimg.width()/theimg.height();
				var elmw = $("#slideshow").width();
				var elmh = $("#slideshow").height();
				var elmrat = elmw/elmh;
				if (imgrat<elmrat)
				   {
				   theimg.css({"width":"100%","height":"auto"});
				   }
				else
				   {
				   theimg.css({"height":"100%","width":"auto"});
				   }
	 	
				//Centering
				theimg.css('left', (elmw - theimg.width())/2);
				theimg.css('top', (elmh - theimg.height())/2);
			});
		});
	
	};

rotate();
imgSize();

};
slideShow();
});

</script>



Here's the start of an example of the methods thing I was talking about:
(function($){
	$.fn.extend({
		blueberry: function(options) {

Here's that examples full script: https://github.com/m...ry.blueberry.js

Thanks,
JD.

This post has been edited by JD.CoolPenguin: 22 January 2013 - 05:25 AM

Was This Post Helpful? 0
  • +
  • -

  • (3 Pages)
  • +
  • 1
  • 2
  • 3