PDA

View Full Version : Compacting Code=Less is Best



Schrene
01-26-2011, 09:17 PM
I have a LOT of code...
Too much code over 4,000 lines of this....
I have already coded it all...
using a great shortcut method combining Excell and Notepad.:thumbsup:
It really bogs down everything with all that code.:(



function showSample() {

if (document.getElementById('pic').value == 'PIC100') {
document.getElementById('pic_id').value = "PIC100";
document.getElementById('_Ath_Slide').src = "_images/ins_1.jpg";
document.getElementById('pic_name').value = "Butterfly Resting";
document.getElementById('pic_num').value = "1";
}
else if (document.getElementById('pic').value == 'PIC200') {
document.getElementById('pic_id').value = "PIC200";
document.getElementById('_Ath_Slide').src = "_images/ins_2.jpg";
document.getElementById('pic_name').value = "Blue Dragonfly";
document.getElementById('pic_num').value = "2";
}

else if (document.getElementById('pic').value == 'pic100') {
document.getElementById('pic_id').value = "PIC100";
document.getElementById('_Ath_Slide').src = "_images/ins_1.jpg";
document.getElementById('pic_name').value = "Butterfly Resting";
document.getElementById('pic_num').value = "1";
}
else if (document.getElementById('pic').value == 'pic200') {
document.getElementById('pic_id').value = "PIC200";
document.getElementById('_Ath_Slide').src = "_images/ins_2.jpg";
document.getElementById('pic_name').value = "Blue Dragonfly";
document.getElementById('pic_num').value = "2";
}


}

Here is a link to my SAMPLE site(which only has a tiny bit of the code): http://meschrene.host22.com/

The reason for "repeating" entries ( example: PIC200 and pic200) is that I wanted to error proof the entry it needs to be caps and the text in the text box appears to be caps (which is what I want) but if I only code it with caps and the person types in lower case then the code doesn't work.

How can I accomplish this without having to code it the way I have with duplicates. Less code is best!! :D

And further more I am sure there must be a better method than 2,000+ else if statements. I hope there is a solution using Javascript. If not perhaps PHP or something else? Any advice would be greatly appreciated. :D

Philip M
01-26-2011, 09:37 PM
if (document.getElementById('pic').value.toLowerCase() == 'pic100') {

or


if (document.getElementById('pic').value.toUpperCase() == 'PIC100') {

All advice is supplied packaged by intellectual weight, and not by volume. Contents may settle slightly in transit.

venegal
01-26-2011, 09:47 PM
Ha! What a fun way to go about this.

Now, let's see ... the only *real* information you got in that code are the image names; all the other values you're setting are all based on the same number. So, you're gonna have to provide the image names somewhere as an array, but the rest can be completely automated:



var imageNames = ['Butterfly Resting', 'Blue Dragonfly', 'all', 'your', 'other', 'images'];

function showSample() {

var number = parseInt(document.getElementById('pic').value.replace(/pic/i, '')) / 100;

document.getElementById('pic_id').value = 'PIC' + number + '00';
document.getElementById('_Ath_Slide').src = '_images/ins_' + number + '.jpg';
document.getElementById('pic_name').value = imageNames[number - 1];
document.getElementById('pic_num').value = number;

}


And that's it. 4000 lines of code gone in the blink of an eye.

Schrene
01-26-2011, 09:53 PM
Thanks that worked great!
Cutting out at least half my code.
Any advice on cutting it down more would be great! :D

Schrene
01-26-2011, 09:55 PM
@venegal thanks gonna check that out now. You rock! :D

Schrene
01-26-2011, 10:14 PM
Awesome!
It worked like magic! LOL:thumbsup::thumbsup:
http://meschrene.host22.com/index.php
Thanks again venegal! :D

On to the next piece of the puzzle! :rolleyes:

venegal
01-26-2011, 10:33 PM
No problem. Do me a favour though:

Really read through my code and try to understand how it works it's pretty easy.

In this thread (http://www.codingforums.com/showthread.php?t=215597) you had a similar convoluted if/else mess, and you really need to avoid that.

Schrene
01-26-2011, 11:14 PM
I did refer back to that post today and tried to apply the example you gave me there to what I am doing. I am trying to understand how it works. My issue is that I don't have all numbers that are even numbers like my sample. I have numbers like 101,102,103.....,153....,1,639.
I have been messing with the math and ended up with a mess so far.

venegal
01-26-2011, 11:27 PM
You just have to know the system behind your conventions, the rest is easy.

If, for example, PIC100 corresponds to the file _images/ins_1.jpg, what does PIC101 correspond to?

Schrene
01-26-2011, 11:39 PM
This is what I came up with the previous help you gave me.


<script type="text/javascript">

function showSample() {
var i;
var input = document.getElementById('input');
var element = document.getElementById(input.value);

// If the element specified in the input does not exist, don't do anything
if ( ! element) return;

// Hide all elements
for (i = 1; i <= 9; i++) {
document.getElementById('SW000' + i).style.display = 'none';
}

// Show the one specified in the input
element.style.display = 'block';
}


</script>

<style type="text/css">
<!--
#SW0001 {
background: url(_images/Colorfil/SW0001.jpg) no-repeat;
height: 150px;
width: 150px;
display: block;
}
#SW0002 {
background: url(_images/Colorfil/SW0002.jpg) no-repeat;
}
#SW0003 {
background: url(_images/Colorfil/SW0003.jpg) no-repeat;
}
#SW0004 {
background: url(_images/Colorfil/SW0004.jpg) no-repeat;
}
#SW0005 {
background: url(_images/Colorfil/SW0005.jpg) no-repeat;
}
#SW0006 {
background: url(_images/Colorfil/SW0006.jpg) no-repeat;
}
#SW0007 {
background: url(_images/Colorfil/SW0007.jpg) no-repeat;
}
#SW0008 {
background: url(_images/Colorfil/SW0008.jpg) no-repeat;
}
#SW0009 {
background: url(_images/Colorfil/SW0009.jpg) no-repeat;
}
#SW0010 {
background: url(_images/Colorfil/SW0010.jpg) no-repeat;
}
#SW0011 {
background: url(_images/Colorfil/SW0011.jpg) no-repeat;
}
#SW0012 {
background: url(_images/Colorfil/SW0012.jpg) no-repeat;
}
#SW0013 {
background: url(_images/Colorfil/SW0013.jpg) no-repeat;
}
#SW0014 {
background: url(_images/Colorfil/SW0014.jpg) no-repeat;
}
#SW0015 {
background: url(_images/Colorfil/SW0015.jpg) no-repeat;
}
#SW0016 {
background: url(_images/Colorfil/SW0016.jpg) no-repeat;
}
#SW0017 {
background: url(_images/Colorfil/SW0017.jpg) no-repeat;
}
#SW0018 {
background: url(_images/Colorfil/SW0018.jpg) no-repeat;
}
#SW0019 {
background: url(_images/Colorfil/SW0019.jpg) no-repeat;
}
#SW0020 {
background: url(_images/Colorfil/SW0020.jpg) no-repeat;
}
.pic {
height: 150px;
width: 150px;
display: none;
}
.drp {

width: 150px;
}
-->
</style>
</head>

<body>

<form id="form1" name="form1" method="post" action="">
<div id="SW0001"></div>
<div class="pic" id="SW0002"></div>
<div class="pic" id="SW0003"></div>
<div class="pic" id="SW0004"></div>
<div class="pic" id="SW0005"></div>
<div class="pic" id="SW0006"></div>
<div class="pic" id="SW0007"></div>
<div class="pic" id="SW0008"></div>
<div class="pic" id="SW0009"></div>
<div class="pic" id="SW0010"></div>
<div class="pic" id="SW0011"></div>
<div class="pic" id="SW0012"></div>
<div class="pic" id="SW0013"></div>
<div class="pic" id="SW0014"></div>
<div class="pic" id="SW0015"></div>
<div class="pic" id="SW0016"></div>
<div class="pic" id="SW0017"></div>
<div class="pic" id="SW0018"></div>
<div class="pic" id="SW0019"></div>
<div class="pic" id="SW0020"></div>
<br />

<input name="textfield" type="text" id="input" onkeyup="showSample()" />

If you enter anything larger than SW0009 the display none doesn't work.
I tried changing these elements (i = 1; i <= 9; i++) {...
I I change the 9 to anything larger then nothing works.
I even played around with the < and =..
I am pretty good at logic but math.. eh not so much.

So I stuck with the method I knew which was the else if statements...

Schrene
01-26-2011, 11:48 PM
You just have to know the system behind your conventions, the rest is easy.

If, for example, PIC100 corresponds to the file _images/ins_1.jpg, what does PIC101 correspond to?

Right got that it's the numbers not so much..

Here is what I tried to apply to the SW samples.


var number = parseInt(document.getElementById('color').value.replace(/sw/i, '')) / 100;

document.getElementById('color_id').value = 'SW' + number;
document.getElementById('_Ath_Slide').src = '../_images/Colorfil/standard/SW' + number + '.jpg';
document.getElementById('color_name').value = imageNames[number - 1];
document.getElementById('color_num').value = number;



All of the image ID's and image Names match.
Still working on it and trying to figure it out on my own.
Just thought you'd want to know that I was trying :)

Schrene
01-27-2011, 12:19 AM
var number = parseInt(document.getElementById('color').value.replace(/sw/i, '')) ;

document.getElementById('color_id').value = 'SW' + number;
document.getElementById('_Ath_Slide').src = '../_images/Colorfil/standard/SW' + number + '.jpg';


This is all I could figure out.

Schrene
01-27-2011, 07:33 PM
I looked and looked and tried and tried.
But alas the rest of it seems to be over my head...
So I guess I will stick with the else if statements.

venegal
01-27-2011, 08:22 PM
Please don't do that!

What's wrong with something like

document.getElementById('color_name').value = imageNames[number];
document.getElementById('color_num').value = number;
?

If your SW codes are arbitrary and not incremental values starting with 0, you would of course have to set up your imageNames differently:

var imageNames = {100: 'Butterfly', 115: 'Unicorn'};
That would work for SW100 and SW115 then.

Schrene
01-27-2011, 09:16 PM
Thanks again venegal!:thumbsup:


That fixed the problem with the color name.

var colorNames = {1256: 'Hot Pink', 1257: 'Purple' , 1258:'Green' , 1259:'Blue' };
The value of color_num is the placement value in the list

color_id='SW1256' color_name='Hot Pink' color_num='1'
color_id='SW1257' color_name='Purple' color_num='2'
color_id='SW1258' color_name='Green' color_num='3'
color_id='SW1259' color_name='Blue' color_num='4'

So I created another variable colorNumber

var colorNumber = {1256: '1', 1257: '2' , 1258:'3' , 1259:'4' };

that worked great! Is there a better way to accomplish that?

venegal
01-27-2011, 10:49 PM
This should work:

var i = 0;
for (var key in colorNames) {
if (key == number) break;
i++;
}
document.getElementById('color_num').value = i + 1;

Would be better than duplicating all those numbers in another object.

Schrene
01-27-2011, 10:59 PM
I did run into one issue if my number is SW0002
then the display looks like this SW2
and the image will not show up.
Is there any way to fix that?


function showSample() {

var number = parseInt(document.getElementById('color').value.replace(/sw/i, '')) ;

document.getElementById('color_id').value = 'SW' + number;
document.getElementById('_Ath_Slide').src = '../_images/Colorfil/standard/SW' + number + '.jpg';
document.getElementById('color_name').value = imageNames[number];
document.getElementById("color_num").value = imageNumber[number];

}

If they all started with 000 I could do this:

document.getElementById('color_id').value = 'SW000' + number
Changing the ID numbers is not something I can do.... or I would do that.
I have numbers from 0001 to 0080 then the numbers start at 6000.

Just seems like there are too many variables to use this method??

Schrene
01-27-2011, 11:15 PM
This should work:

var i = 0;
for (var key in colorNames) {
if (key == number) break;
i++;
}
document.getElementById('color_num').value = i + 1;

Would be better than duplicating all those numbers in another object.

Thanks that did the trick!
I'm sure by now I must look like some kinda confusing nut.:oLOL
The truth is I am applying the same concept to three different areas..
My Photos and two different color sample areas.
Thus the change between color and image titles.
My photos area is fine because I can control all of the names.
But the color sample ID numbers come from another company and I have to use them and have no control on them.
I hope that clears up any confusion.

venegal
01-27-2011, 11:25 PM
I see. I was wondering where the butterflies where gone.

As for the leading zeros, that's no biggie. There's no native Javascript function for padding a number with zeros, but see for instance here (http://www.electrictoolbox.com/pad-number-zeroes-javascript/).

Schrene
01-29-2011, 12:20 AM
Well I checked out the function in the link.
I couldn't figure out how to apply it to only some of the items and not others...
So I set up something using some PHP and MySql.:thumbsup:
Which was easier for me to figure out and I like more since all the data is in the MySql database instead of in the Javascript.
Here is the link: http://meschrene.host22.com/Color_Samples_Test.html


<script language="javascript" type="text/javascript">
<!--
//Browser Support Code
function showID(){
var idRequest;

try{
// Opera 8.0+, Firefox, Safari
idRequest = new XMLHttpRequest();
} catch (e){
// Internet Explorer Browsers
try{
idRequest = new ActiveXObject("Msxml2.XMLHTTP");
} catch (e) {
try{
idRequest = new ActiveXObject("Microsoft.XMLHTTP");
} catch (e){
// Something went wrong
alert("Your browser broke!");
return false;
}
}
}


idRequest.onreadystatechange = function(){
if(idRequest.readyState == 4){
var idDisplay = document.getElementById('color_id');
idDisplay.innerHTML = idRequest.responseText;
}
}
var color = document.getElementById('color').value;

var queryString = "?color=" + color;
idRequest.open("GET", "data_color_id.php" + queryString, true);
idRequest.send(null);
}
function showName(){
var nameRequest;

try{
// Opera 8.0+, Firefox, Safari
nameRequest = new XMLHttpRequest();
} catch (e){
// Internet Explorer Browsers
try{
nameRequest = new ActiveXObject("Msxml2.XMLHTTP");
} catch (e) {
try{
nameRequest = new ActiveXObject("Microsoft.XMLHTTP");
} catch (e){
// Something went wrong
alert("Your browser broke!");
return false;
}
}
}


nameRequest.onreadystatechange = function(){
if(nameRequest.readyState == 4){
var nameDisplay = document.getElementById('color_name');
nameDisplay.innerHTML = nameRequest.responseText;
}
}
var color = document.getElementById('color').value;

var queryString = "?color=" + color;
nameRequest.open("GET", "data_color_name.php" + queryString, true);
nameRequest.send(null);
}
function showNum(){
var numRequest;

try{
// Opera 8.0+, Firefox, Safari
numRequest = new XMLHttpRequest();
} catch (e){
// Internet Explorer Browsers
try{
numRequest = new ActiveXObject("Msxml2.XMLHTTP");
} catch (e) {
try{
numRequest = new ActiveXObject("Microsoft.XMLHTTP");
} catch (e){
// Something went wrong
alert("Your browser broke!");
return false;
}
}
}


numRequest.onreadystatechange = function(){
if(numRequest.readyState == 4){
var numDisplay = document.getElementById('color_num');
numDisplay.innerHTML = numRequest.responseText;
}
}
var color = document.getElementById('color').value;

var queryString = "?color=" + color;
numRequest.open("GET", "data_color_num.php" + queryString, true);
numRequest.send(null);

}

function showPic() {

var number = document.getElementById('color').value ;

document.getElementById('color_pic').src = '_images/Colorfil/' + number + '.jpg';

}




//-->
</script>

</head>

<body>
<form name='myForm'id='myForm'>
<div id="right">
<img src="_images/Colorfil/SW0001.jpg" id="color_pic"/><br /><br />
<label>Number will display here</label>
<br />
<div id="color_num" class="dataBx"></div>
<br />
<label>ID will display here</label>
<br />
<div id="color_id"class="dataBx"></div>
<br />
<label>Name will display here</label>
<br />
<div id="color_name"class="dataBx"></div>
</div>
<div id="left">Type Color ID:
<input type='text' id='color' onkeyup="setTimeout(function() {showID();showName();showNum();showPic()}, 2000)" onPaste="setTimeout(function() {showID();showName();showNum();showPic()}, 30)" autocomplete="off"/> <br />
</select>



<br />

<em><strong>Color ID List:</strong><br />
SW0001, SW0002, SW0074, SW6606, SW6586, SW7141</em><br />
</div>
</form>
Works great except I am having the same problem that I was having with the other code. The text and image "flash nonsense" until the whole ID is typed in. I tried putting in a time out that sort of works.... but I am not exactly happy with the result. I tried setting up some sort of show/hide so that the item wouldn't show till the whole code was entered. No luck with that though. Any suggestions??:confused:

venegal
01-29-2011, 03:41 PM
Well I checked out the function in the link.
I couldn't figure out how to apply it to only some of the items and not others...


Why would you do that? The way you described your situation, you wanted all of the numbers to be four digits long. That's exactly what zero-padding does. You apply it to all of the numbers, so all of them have the same length.

If you're doing this with PHP/MySQL, you really have to know what you're doing. For example, I entered a backtick in your field, which for some reason completely broke your site. It's not working any more.

Also, it's silly to send out five different requests to get the info. It's bad enough to need a request at all, and not have that information available from the start (unless we're talking about huge amounts of data, but if it's less than, say, 50kB, I'd definitely send it with the page itself, so there will be no need for AJAX requests, which make the whole user experience much slower). But, if you really have to use AJAX, there's no reason for you to not just use one request, which sends back all the for the requested color.

Setting a timeout is definitely the wrong way to do what you're trying to do. If you'd want to make this work ok, you'd have to use debouncing (http://unscriptable.com/index.php/2009/03/20/debouncing-javascript-methods/), and you'd have to check in your onreadystatechange callback whether what's been sent back by the server actually makes sense, before putting it in your DOM.

Schrene
01-31-2011, 05:31 PM
Why would you do that? The way you described your situation, you wanted all of the numbers to be four digits long. That's exactly what zero-padding does. You apply it to all of the numbers, so all of them have the same length.

Because my understanding of the link you gave me is that you can pad a specific number with a set number of zeros.


<script type="text/javascript">
function pad(number, length) {

var str = '' + number;
while (str.length < length) {
str = '0' + str;
}

return str;

}
document.write(pad(0, 4) + '<br />');
document.write(pad(1, 4) + '<br />');
document.write(pad(6035, 5) + '<br />');
</script>
I don't understand how to apply that to what I have.:confused:

I tried several ways...
This works for the image and the ID (as long as the number starts with 3 zeros) but then the name and number are messed up.

function showSample() {

var i = 0;
for (var key in colorNames) {
if (key == number) break;
i++;
}
var number = pad(0,3)+parseInt(document.getElementById('color').value.replace(/sw/i, '')) ;

document.getElementById('color_id').value = 'SW' + number ;
document.getElementById('color_pic').src = '../_images/Colorfil/SW' + number + '.jpg';
document.getElementById('color_name').value = colorNames[number];
document.getElementById('color_num').value = i + 1;


}
I could show you all the different ways I tried and did not work but I will spare you the rubbish. Just know that I could not figure it out.


If you're doing this with PHP/MySQL, you really have to know what you're doing. For example, I entered a backtick in your field, which for some reason completely broke your site. It's not working any more.

Still works for me from two different locations????


Also, it's silly to send out five different requests to get the info. It's bad enough to need a request at all, and not have that information available from the start (unless we're talking about huge amounts of data, but if it's less than, say, 50kB, I'd definitely send it with the page itself, so there will be no need for AJAX requests, which make the whole user experience much slower). But, if you really have to use AJAX, there's no reason for you to not just use one request, which sends back all the for the requested color.

About as silly as a toddler crawling:p
I am still learning remember.:rolleyes:
I realize it is not proper coding method.
It was the only way I could quickly set it up.
Would I leave it that way? umm NO

Again thanks for your help:)

venegal
01-31-2011, 06:20 PM
Zero-padding puts zeroes in front of the number until the specified length is reached:

pad (1, 4) will give you 0001
pad(25, 4) will give you 0025
pad(6035, 4) will give you 6035

The situation you described earlier sounded like that's what you wanted.

Maybe it was just your hosting that gave out on me; I don't really know how your server side code looks like, so it might be ok.

I didn't mean to offend; I understand that you are still learning and trying things out, which is a good thing. I just wanted to make very clear that as soon as you start using AJAX, and you still want it to be a good user experience, you suddenly have to be aware of a lot of pitfalls, and that it might be easier to just put the data you need directly into your page.

Schrene
01-31-2011, 06:43 PM
Zero-padding puts zeroes in front of the number until the specified length is reached:

pad (1, 4) will give you 0001
pad(25, 4) will give you 0025
pad(6035, 4) will give you 6035

The situation you described earlier sounded like that's what you wanted.

Yes essentially that is what I need.... but there seems to be no way to apply the appropriate rule to the appropriate number. Not that I see anyhow???


Maybe it was just your hosting that gave out on me..

Could be it's a free server.


I didn't mean to offend; I understand that you are still learning and trying things out, which is a good thing. I just wanted to make very clear that as soon as you start using AJAX, and you still want it to be a good user experience, you suddenly have to be aware of a lot of pitfalls, and that it might be easier to just put the data you need directly into your page.

No problem... my site is a dynamic site.. I was only dealing with displaying data from one element from the data at a time before. I figured it would be simple enough to display data from three different elements of the data at one time. So I just quickly copied and pasted the original script changing text where needed. I figured that I would go back and clean it up later... That is if I can figure out how to fix the flashing nonsense until ID is finished being typed in. It is really unexceptionable as is.

So far it looks like I am going to have to stick with the else if statements with this list because there are too many variables in the numbers.:rolleyes:

venegal
01-31-2011, 06:56 PM
Yes essentially that is what I need.... but there seems to be no way to apply the appropriate rule to the appropriate number. Not that I see anyhow???


I don't really know what you mean with "the appropriate rule". If your situation is indeed like you described before, it's always the same rule ("add zeroes until the number is 4 digits long"). Your code would look like this then:

document.getElementById('color_id').value = 'SW' + pad(number, 4);




That is if I can figure out how to fix the flashing nonsense until ID is finished being typed in. It is really unexceptionable as is.


I thought I already told you that before? There's only stuff flashing because you don't make sure that what the server sends back is valid data you actually want to show on the website. So, just make sure that it is before doing anything in your callback.

Schrene
01-31-2011, 09:17 PM
I don't really know what you mean with "the appropriate rule". If your situation is indeed like you described before, it's always the same rule ("add zeroes until the number is 4 digits long"). Your code would look like this then:

document.getElementById('color_id').value = 'SW' + pad(number, 4);


That is one of the methods I tried this is what happens :http://meschrene.host22.com/Color_Samples_B.html
Try to enter SW0003 all works but color number.
Try to enter SW0009 interesting result... name is correct but pic and id is not.
Try to enter SW0010 interesting result...
Try to enter SW6613 all works but color number.
Like I said there seems to be too many variables.

venegal
02-01-2011, 12:29 PM
It's a bit hard to keep track, since your requirements (or at least the ones you're sharing) change quite often.

In the setup you got right now, you don't need zero-padding at all, since those zeroes are already there in the input field. You have to make sure, though, that you treat those as strings, and not numbers, so the leading zeroes won't get lost.

That means, the keys of your color object have to be strings:


var colorNames = {
'0001': 'Mulberry Silk',
'0002': 'Chelsea Mauve',
'0003': 'Cabbage Rose',
// ... and so on. Mind the quotes.
}

Your function could look like this then:

function showSample() {

var number = document.getElementById('color').value.replace(/sw/i, '');

var i = 0;
for (var key in colorNames) {
if (key == number) break;
i++;
}

document.getElementById('color_id').value = 'SW' + number;
document.getElementById('color_pic').src = '../_images/Colorfil/SW' + number + '.jpg';
document.getElementById('color_name').value = colorNames[number];
document.getElementById('color_num').value = i + 1;

}

I feel that it's becoming increasingly important, though, that you really understand what your code does, and that you are able to find out on your own why things don't work.

Go get yourself Firebug and read some tutorial on debugging. You have to go right in there, set breakpoints, see what's what, or you'll never be able to write a concise piece of code that you can be sure will work reliably. Check out the code you had in there before (with a debugger!) and try to find out on your own, what it does, and why it doesn't do what you want it to do. Trial and error will get you nowhere.

Schrene
02-01-2011, 06:19 PM
Thanks for all your help and advice. I do desire to understand why things function and don't function the way I need them to so I can successfully write code . I am running out of time however and need to move on to my next project. I decided to stick with the PHP/MySql set up. I was able to make it function properly/satisfactorily for all elements. I just added this before my data "echo" : if(strlen($color) == 6)

Again thanks for all your help.:thumbsup: