Is this good or bad code?

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

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

#1 JD.CoolPenguin  Icon User is offline

  • New D.I.C Head

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

Is this good or bad code?

Posted 16 January 2013 - 02:12 PM

Wasn't really sure how to title this, but basically, I've written my second javascript, a very simple image slideshow, and I was hoping somebody who was more experienced could look over the code and basically tell me if it's "good code" or not. (My first was script that made the image fit it's container perfectly, a "fullscreen" image script almost.)
It also uses the jQuery library.

Have I used anything that could be considered bad practice?
Have I done something in a particularly long winded way?
What are the drawbacks of the method I've used (I've noticed the fading isn't exactly perfect every time)?

Those sorts of things.

So first of all, I had to get all the images (unknown number and unknown name), so I used PHP to fetch them and create a javascript array.

<?php
$directory = "images/featured/";
$images = glob($directory . "*.*");
$i = 0;
echo "<script>\nvar imgArr = new Array();\n";
foreach($images as $image)
{
$i++;
echo "imgArr[".$i."] = new Image();\nimgArr[".$i."].src = '".$image."';\n";
}
echo "var totimg = ".$i.";\n</script>";
?>
<!DOCTYPE html>
<head>
<link rel="stylesheet" href="style.css">
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.0/jquery.min.js"></script>
<script src="slide.js"></script>
</head>
<body>
<div id="bkgr"><img src="images/featured/ex01.jpg" alt="Turn It Up background"></div>
</body>
</html>


Then in slide.js I have:
var strt = Math.floor(Math.random()*totimg);

var playSlide = function(){
	$("#bkgr img").fadeOut("1000", function() {
	$("#bkgr img").attr("src",imgArr[strt].src).fadeIn("3000");
	if (strt < totimg)
	{
	strt++;
	}
	else
	{
	strt = 1;
	}
	imgSize();
	});
};

$(window).load(function(){
	imgSize();
	setInterval("playSlide()", 5000);
});


(NOTE: imgSize() refers to my first script the one that is resizing the images to fill the element).

And that's it.
The css is pretty much irrelevant but here it is anyway:
#bkgr {
background: #000;
margin: 0 auto;
padding: 0;
height: 100%;
width: 80%;
position: fixed; top: 0; left: 20%;
z-index: -100;
}

#bkgr img
{
position: relative;
border: 0;
}


If you're wandering about the positioning and size of the container, it's built around the rest of the website, I've just posted the relevant bits of code here.

What other methods for an image slider would people recommend?
I had an idea about stacking all the images on top of each other, and then just fading them out one by one revealing the next img behind it until you get to the bottom and then it would start again (would have to work out how to make the nice fade effect in on the last one to the first one again).

Thanks in advance,
JD

Is This A Good Question/Topic? 0
  • +

Replies To: Is this good or bad code?

#2 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3489
  • View blog
  • Posts: 10,057
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 16 January 2013 - 02:49 PM

on first glance I see some issues.

1) there must not be content before the HTML DTD

2) in slide.js there are external variables (imgArr, totimg). all variables in a script should be declared inside it. otherwise it will cease to work if used stand-alone (i.e. you donít provide the external variable).
in other words, you have an unwanted context dependency.

3) setInteval() (as well as setTimeout()) expects a function to be given. passing a string also works, but opens you up to all problems you get with eval().

4) use $(function() { /* your code */ }); instead of $(window).load(function() { /* your code */ });

5) global variables are always a sign of bad code. they can lead to unpredictable behaviour and are hard to debug.

6) do not mix languages, use templates.

7) the indentation is insufficient. good indentation makes code easy to read and easy to debug.
Was This Post Helpful? 3
  • +
  • -

#3 Martyr2  Icon User is offline

  • Programming Theoretician
  • member icon

Reputation: 4316
  • View blog
  • Posts: 12,096
  • Joined: 18-April 07

Re: Is this good or bad code?

Posted 16 January 2013 - 03:02 PM

To add onto what Dormilich has already mentioned:

8) You will want to always use "type" when using things like the <script> or <style> tag.

9) When you want to reuse selectors like $("#bkgr img") it might be better for you to put them in a variable and use that variable rather than the specifying the selector again (which goes through the entire dom reselecting elements you already selected before). It is a bit of a performance issue.

Just a couple other things I noticed. :)
Was This Post Helpful? 0
  • +
  • -

#4 JD.CoolPenguin  Icon 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 16 January 2013 - 03:10 PM

Thanks for the reply.

1) I did wander about that, it did seem odd to me, I don't usually use PHP very often, but I had to google for this line:
$images = glob($directory . "*.*");
and their example was about the HTML DTD. So thanks, I will move it, where would you put this? I don't suppose it really matters, unless someone says otherwise I will just put it before the closing body tag (where I was going to put it originally).

2) That's because those 2 variables are generated using PHP and there is no way (that I know of) of getting that information from inside the js file. (Unless I didn't make a js file and just made it a php file and included it in the page? Would that be a good way of doing it?)
Though, I wasn't planning on releasing it like a plugin, just for personal use, so it's down to me to remember to provide those external variables

3) Yep, I realised that after posting, but I don't seem to have an edit post button yet and didn't wanna spam the thread with an additional post for that little edit, I also noticed that when I made the array I did
new Array()
instead of
new Array([])


4) Does that mean exactly the same thing? I've seen it used, but without an explanation it meant absolutely jack all really.

5) Sometimes global variables must be necessary though? For example the
strt
variable can't be declared inside
playSlide()
because I want to add 1 to it's previous value every time not generate a new value.

6) PHP was the only way I knew I could get the files from a folder, I know that javascript has no file system access. What are these templates you speak of?

7) Working on it ;)

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

#5 JD.CoolPenguin  Icon 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 16 January 2013 - 03:21 PM

View PostMartyr2, on 16 January 2013 - 03:02 PM, said:

To add onto what Dormilich has already mentioned:

8) You will want to always use "type" when using things like the <script> or <style> tag.

9) When you want to reuse selectors like $("#bkgr img") it might be better for you to put them in a variable and use that variable rather than the specifying the selector again (which goes through the entire dom reselecting elements you already selected before). It is a bit of a performance issue.

Just a couple other things I noticed. :)/>


8) According to w3schools, in HTML5 (which is what that doctype is), you do not need to use the type attribute for the script tag for javascript as it is the default. If I haven't used it on a style tag somewhere I shall give myself a slap on the wrist ;)

9) That's a good point, I think when I started writing it, I didn't realise I would be using it so many times so didn't declare it as a variable, but I shall go back and change it now.

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

#6 JD.CoolPenguin  Icon 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 16 January 2013 - 04:12 PM

I realise this is my 3rd post in a row, but I don't get an edit button til I've made 15 posts, so, sorry, a moderator could merge them should they wish.

Anyway.
@Dormilich - just to say, I googled your suggested use of $(function() { and have found that it is the shorthand of document ready not window load. I have used window load as apposed to document ready on purpose for the other script (the image resize one I mentioned earlier), as I had to wait for the images to load fully before executing that code. So I figured I may as well put this function in the same place as it does no harm.

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

#7 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3489
  • View blog
  • Posts: 10,057
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 16 January 2013 - 11:35 PM

1) I was not speaking of PHP in general, I was speaking of your echo statements.

2) why not making the js file a PHP file? it is nothing different from making PHP echo HTML. (the only difference is, that text/html is the default MIME type of a PHP document and hence you have to explicitly declare the application/javascript MIME type for JS files via header()).

3) new Array([]) is even more bad than new Array(), just use the array literal []. besides that, look into json_encode().

5)

Quote

Sometimes global variables must be necessary though?

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.

6) Templates are a clean way to create code of one language by means of another without mixing.

ex. PHP/HTML
// you could load that from a file as well
$ex = <<<HTML
<!DOCTYPE html>
<html>
  <head>
    <title>%s</title>
  </head>
  <body>
    <div id="content">%s</div>
  </body>
</html>
HTML;

$html = sprintf($ex, "my first template", file_get_contents("inc.body.html"));

// ...

echo $html;


keyword esp. for PHP: "Front Controller (Pattern)"
Was This Post Helpful? 2
  • +
  • -

#8 quim  Icon 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 16 January 2013 - 11:37 PM

As good practice you should put any script at the end of the page unless you will need to use the script before the DOM loads.

something like this:

<!DOCTYPE html>
    <head>
        <meta charset="utf-8">
        <title></title>	
    </head>
    <body>

        <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.0/jquery.min.js"></script>
        <script src="main.js"></script>
    </body>
</html>


Javascript:

// Main js
(function(){
// try to avoid defining variables in the global scope, that is evil.


})();


This post has been edited by quim: 16 January 2013 - 11:39 PM

Was This Post Helpful? 0
  • +
  • -

#9 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3489
  • View blog
  • Posts: 10,057
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 16 January 2013 - 11:47 PM

View Postquim, on 17 January 2013 - 07:37 AM, said:

As good practice you should put any script at the end of the page unless you will need to use the script before the DOM loads.

Programmers have debated that for ages. in the end it comes down to personal preference as each variant has its pros and cons.


another version of the IIFE:
// using some little magic to make it a bit faster
(function(window, document, undefined){

}(window, document));

This post has been edited by Dormilich: 16 January 2013 - 11:49 PM

Was This Post Helpful? 0
  • +
  • -

#10 quim  Icon 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:38 AM

I think @#1 JD.CoolPenguin has been help. So lets not hijack this thread to discuss about personal preference. The fact is that if an unneeded script is inserted at the end of the page the browser will wait until the page is loaded to make an HTTP request for the script, thus saving some seconds.
Was This Post Helpful? 0
  • +
  • -

#11 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3489
  • View blog
  • Posts: 10,057
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 17 January 2013 - 12:48 AM

View Postquim, on 17 January 2013 - 08:38 AM, said:

So lets not hijack this thread to discuss about personal preference.

Iím not discussing personal preferences, Iím just mentioning that there is more than one way of good practice.

View Postquim, on 17 January 2013 - 08:38 AM, said:

The fact is that if an unneeded script is inserted at the end of the page the browser will wait until the page is loaded to make an HTTP request for the script, thus saving some seconds.

Quite debatable.
* there usually is no unneded script (unless the developer screwed up)
* browser caching
* if your script needs some seconds to load, you have other problems (or in other words, scripts usually load way faster than images)
* modern browsers can download in parallel
* transfer compression (i.e. gzip deflate)
Was This Post Helpful? 0
  • +
  • -

#12 andrewsw  Icon User is online

  • Fire giant boob nipple gun!
  • member icon

Reputation: 3237
  • View blog
  • Posts: 10,868
  • Joined: 12-December 12

Re: Is this good or bad code?

Posted 17 January 2013 - 04:23 AM

@Dormilich
Do you foresee any problem with the following aliases?

// using some little magic to make it a bit faster
(function(win, doc, undefined){

}(window, document));

Was This Post Helpful? 0
  • +
  • -

#13 JD.CoolPenguin  Icon 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 17 January 2013 - 05:55 AM

Thank you everyone for your help :)

It is greatly appreciated!

JD
Was This Post Helpful? 0
  • +
  • -

#14 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3489
  • View blog
  • Posts: 10,057
  • Joined: 08-June 10

Re: Is this good or bad code?

Posted 17 January 2013 - 08:18 AM

View Postandrewsw, on 17 January 2013 - 12:23 PM, said:

@Dormilich
Do you foresee any problem with the following aliases?

// using some little magic to make it a bit faster
(function(win, doc, undefined){

}(window, document));

in principle not.

This post has been edited by Dormilich: 17 January 2013 - 08:20 AM

Was This Post Helpful? 0
  • +
  • -

#15 andrewsw  Icon User is online

  • Fire giant boob nipple gun!
  • member icon

Reputation: 3237
  • View blog
  • Posts: 10,868
  • Joined: 12-December 12

Re: Is this good or bad code?

Posted 17 January 2013 - 08:27 AM

@Dormilich. Thank you.
Was This Post Helpful? 0
  • +
  • -

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