Log in

View Full Version : My JavaScript is broken.


techno_race
08-22-2008, 12:44 AM
As usual.
The idea is to take a user-submitted hex color code, split it into its R, G, and B parts, and then display them. What's wrong with this picture?

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<title>Color Splitter</title>
<script type="text/javascript">
function splitColors()
{
var input = document.getElementById('input').value;
var array = input.split("");
var result = array;
if (array[0] == "#") {
result = array.splice(0,1);
}
var ah = array.length;
if (ah == 3) {
var v1 = result[0];
var v2 = result[1];
var v3 = result[2];
result = [v1,v1,v2,v2,v3,v3];
}
if (ah == 0) {
result = [0,0,0,0,0,0];
if (ah == 1) {
var old = result[0];
result = [old,old,old,old,old,old];
}
if (ah == 2) {
var v1 = result[0];
var v2 = result[1];
result = [v1,v2,v1,v2,v1,v2];
}
if (ah == 4) {
var v1 = result[0];
var v2 = result[1];
var v3 = result[2];
var v4 = result[3];
result = [v1,v2,v3,v4,v1,v2];
}
if (ah == 5) {
var v1 = result[0];
var v2 = result[1];
var v3 = result[2];
var v4 = result[3];
var v5 = result[4];
result = [v1,v2,v3,v4,v5,v1];
}
if (ah == 7) {
var old = result;
result = old.splice(6,1); //No pun intended
}
if (result[0] != "0" || result[0] != "1" || result[0] != "2" || result[0] != "3" || result[0] != "4" || result[0] != "5" || result[0] != "6" || result[0] != "7" || result[0] != "8" || result[0] != "9" || result[0] != "A" || result[0] != "B" || result[0] != "C" || result[0] != "D" || result[0] != "E" || result[0] != "F" || result[0] != "a" || result[0] != "b" || result[0] != "c" || result[0] != "d" || result[0] != "e" || result[0] != "f") {
result[0] = 'F';
}
if (result[1] != "0" || result[1] != "1" || result[1] != "2" || result[1] != "3" || result[1] != "4" || result[1] != "5" || result[1] != "6" || result[1] != "7" || result[1] != "8" || result[1] != "9" || result[1] != "A" || result[1] != "B" || result[1] != "C" || result[1] != "D" || result[1] != "E" || result[1] != "F" || result[1] != "a" || result[1] != "b" || result[1] != "c" || result[1] != "d" || result[1] != "e" || result[1] != "f") {
result[1] = 'F';
}
if (result[2] != "0" || result[2] != "1" || result[2] != "2" || result[2] != "3" || result[2] != "4" || result[2] != "5" || result[2] != "6" || result[2] != "7" || result[2] != "8" || result[2] != "9" || result[2] != "A" || result[2] != "B" || result[2] != "C" || result[2] != "D" || result[2] != "E" || result[2] != "F" || result[2] != "a" || result[2] != "b" || result[2] != "c" || result[2] != "d" || result[2] != "e" || result[2] != "f") {
result[2] = 'F';
}
if (result[3] != "0" || result[3] != "1" || result[3] != "2" || result[3] != "3" || result[3] != "4" || result[3] != "5" || result[3] != "6" || result[3] != "7" || result[3] != "8" || result[3] != "9" || result[3] != "A" || result[3] != "B" || result[3] != "C" || result[3] != "D" || result[3] != "E" || result[3] != "F" || result[3] != "a" || result[3] != "b" || result[3] != "c" || result[3] != "d" || result[3] != "e" || result[3] != "f") {
result[3] = 'F';
}
if (result[4] != "0" || result[4] != "1" || result[4] != "2" || result[4] != "3" || result[4] != "4" || result[4] != "5" || result[4] != "6" || result[4] != "7" || result[4] != "8" || result[4] != "9" || result[4] != "A" || result[4] != "B" || result[4] != "C" || result[4] != "D" || result[4] != "E" || result[4] != "F" || result[4] != "a" || result[4] != "b" || result[4] != "c" || result[4] != "d" || result[4] != "e" || result[4] != "f") {
result[4] = 'F';
}
if (result[5] != "0" || result[5] != "1" || result[5] != "2" || result[5] != "3" || result[5] != "4" || result[5] != "5" || result[5] != "6" || result[5] != "7" || result[5] != "8" || result[5] != "9" || result[5] != "A" || result[5] != "B" || result[5] != "C" || result[5] != "D" || result[5] != "E" || result[5] != "F" || result[5] != "a" || result[5] != "b" || result[5] != "c" || result[5] != "d" || result[5] != "e" || result[5] != "f") {
result[5] = 'F';
}
var red = result[0] + "" + result[1];
var green = result[2] + "" + result[3];
var blue = result[4] + "" + result[5];
var full = red + "" + green + "" + blue;
var newHTML = '<span style="background-color: #' + full + ';">' + full + '</span><br>is made of<br><span style="background-color: #' + red + '0000;">' + red + '0000</span><br><span style="background-color: #00' + green + '00;">00' + green + '00</span><br><span style="background-color: #0000' + blue + ';">0000' + blue + '</span>';
document.getElementById('output').innerHTML = newHTML;
}
</script>
</head>

<body>
<input name="input" type="text" id="input" size="12" maxlength="7">
<input type="button" name="button" id="button" value="Split" onClick="splitColors()">
<div id="output">Your splitted color will be here.</div>
</body>
</html>
Twey
08-22-2008, 06:06 PM
There's too much useless code in there for me to feel like reading it, but you may get some hints from this post (http://dynamicdrive.com/forums/showpost.php?p=53485&postcount=1337).

Stay away from innerHTML — it's non-standard and will make your life harder in the long run.
hosdank
08-22-2008, 06:27 PM
You forgot a semicolon in your onclick, and a bracket in one of your ifs... I don't even get the rest :P
Nile
08-22-2008, 07:43 PM
It looks like this is a lot shorted then yours:
http://www.linuxtopia.org/online_books/javascript_guides/javascript_faq/hextorgb.htm

If there's anything else your looking for other then that. You can add those onto that script.
Also, one of the reasons your javascript was broken was because you forgot to end your function.

Again, like Twey said, try to make your code shorter.
Thats important when worrying about file size. Its also a bad habbit.
Also, when makign code, don't use the [TAB] button, here's what your code looks like(example):


function(e){
e.split("");
}

Here's a shorter way:


function(e){
e.split("");
}
mburt
08-22-2008, 09:48 PM
I believe Twey was referring to the overuse of if commands and whatnot...
Nile
08-22-2008, 09:57 PM
Mburt, it also could be a lot shorter with the use of short if statements, for loops, and having lines such as:


var red = result[0] + "" + result[1];
var green = result[2] + "" + result[3];
var blue = result[4] + "" + result[5];

Put it a proper format such as:


var red = result[0] + result[1];
var green = result[2] + result[3];
var blue = result[4] + result[5];

And even turning those variables into an array would be a bit more orgainized.
mburt
08-22-2008, 10:01 PM
The point of this script is to change hex into rgb, then display right?

This script is shorter:


<script type="text/javascript">
_ = function(e) { return document.getElementById(e); };
</script>
<div id="object">&nbsp;</div>
<input type="text" id="foo">
<input type="button" value="Test" onclick="_('object').style.background = 'rgb('+rgb(_('foo').value)+')'">
<script type="text/javascript">
var rgb = function(hex) {
hex = hex.replace(/\#/i, "")
for (var i = 2, str = "";i <= 6;i=i+2) {
str += parseInt(hex.substring(i-2, i), 16)+", ";
};
return str.substring(0, str.length-2);
};
</script>
mburt
08-22-2008, 10:35 PM
Actually... After looking at it, I don't know why I always use the "str += ", when I could simply use an array, then join it:


<script type="text/javascript">
_ = function(e) { return document.getElementById(e); };
</script>
<div id="object">&nbsp;</div>
<input type="text" id="foo">
<input type="button" value="Test" onclick="_('object').style.background = 'rgb('+rgb(_('foo').value)+')'">
<script type="text/javascript">
var rgb = function(hex) {
for (var i = 2, out = [];i <= 6;i=i+2) {
out[(i/2)-1] = parseInt((hex.replace(/\#/i, "")).substring(i-2, i), 16);
};
return out.join(", ");
};
</script>
Twey
08-22-2008, 10:38 PM
I was referring to the repetitiveness of the code. Computers exist so that you don't have to perform the same repetitions over and over again manually. If you find yourself doing so, you're doing it wrong and need to abstract more — try a loop, for example.

mburt, your code has errors.

I refer you once again to my Colour object in the post to which I linked above, which contains code for converting to/from various string formats.
Nile
08-22-2008, 10:40 PM
Twey, like I said?
(Laughs at Mburt, saying I told you!)
mburt
08-22-2008, 10:40 PM
I was referring to the repetitiveness of the code. Computers exist so that you don't have to perform the same repetitions over and over again manually. If you find yourself doing so, you're doing it wrong and need to abstract more — try a loop, for example.

Well, yeah, that's what I meant... What do you think of the example I gave? Is there a better way of writing it?
mburt
08-22-2008, 10:43 PM
mburt, your code has errors.
Yeah, I looked at your color object too, in that post, but it seemed kind of pointless having all those other functions there.

I thought it would be simple enough just to include the RGB converter. Anyways, what's wrong with my script?
mburt
08-22-2008, 10:59 PM
And here's a better version of the script, which handles all inputs:


<script type="text/javascript">
_ = function(e) { return document.getElementById(e); };
</script>
<div id="object">&nbsp;</div>
<input type="text" id="foo">
<input type="button" value="Test" onclick="_('object').style.background = 'rgb('+rgb(_('foo').value)+')'">
<script type="text/javascript">
String.prototype.pad = function(x) {
for (var i = 0, r = this.split(""), out = [];i < x;++i) {
out[i] = r[i % r.length];
};
return out.join("");
};
var rgb = function(hex) {
hex = !hex ? "f" : hex;
for (var i = 2, out = [];i <= 6;i=i+2) {
out[(i/2)-1] = parseInt((hex.replace(/\#/i, "").pad(6)).substring(i-2, i), 16);
};
return out.join(", ");
};
</script>