JS Code Advice (greatly appreciated)


R

rupoopin

Hello,
I am not a good programmer. I wrote a javascript to check my lotto
numbers. I would like feedback on how I could make the script I wrote
'better'. Thanks in advance, I really look forward to seeing different
solutions to the script I wrote. I am sure there there is a much
better way to accomplish the end result.

Here is the code:

<html>
<head>
<title>Check Lotto Nums</title>
<script type="text/javascript">
var oSetOne=new Array("2","7","13","17","33","47");
var oSetOneIds=new Array("1","2","3","4","5","6");

var oSetTwo=new Array("12","19","22","31","32","41");
var oSetTwoIds=new Array("7","8","9","10","11","12");

var oSetThree=new Array("17","22","23","25","26","42");
var oSetThreeIds=new Array("13","14","15","16","17","18");

var oSetFour=new Array("14","21","28","29","37","46");
var oSetFourIds=new Array("19","20","21","22","23","24");

var oSetFive=new Array("1","5","6","15","18","35");
var oSetFiveIds=new Array("25","26","27","28","29","30");

var oSetSix=new Array("10","11","31","36","38","48");
var oSetSixIds=new Array("31","32","33","34","35","36");

var oSetSeven=new Array("3","4","15","23","36","49");
var oSetSevenIds=new Array("37","38","39","40","41","42");

var oSetEight=new Array("1","3","27","36","39","42");
var oSetEightIds=new Array("43","44","45","46","47","48");

var oSetNine=new Array("2","8","14","18","22","36");
var oSetNineIds=new Array("49","50","51","52","53","54");

var oSetTen=new Array("5","23","36","40","43","45");
var oSetTenIds=new Array("55","56","57","58","59","60");

function CheckNums(x,z)
{
var i;
var y=document.getElementById("myform");
for(i=0;i<6;i++)
{
if(x == y.elements[0].value)
{
document.getElementById(z).style.color="red";
}
else if(x == y.elements[1].value)
{
document.getElementById(z).style.color="red";
}
else if(x == y.elements[2].value)
{
document.getElementById(z).style.color="red";
}
else if(x == y.elements[3].value)
{
document.getElementById(z).style.color="red";
}
else if(x == y.elements[4].value)
{
document.getElementById(z).style.color="red";
}
else if(x == y.elements[5].value)
{
document.getElementById(z).style.color="red";
}
else
{
document.getElementById(z).style.color="black";
}
}
}
</script>
<style type="text/css" media="screen">
body
{
font-family:Arial;
}
</style>
</head>

<body>

<h3>Winning Numbers</h3>

<form name="myform" id="myform" title="A Little Form">
<input type="text" name="fieldone" size="1" maxlength="2" />

&nbsp;
<input type="text" name="fieldtwo" size="1" maxlength="2" />
&nbsp;
<input type="text" name="fieldthree" size="1" maxlength="2" />
&nbsp;
<input type="text" name="fieldfour" size="1" maxlength="2" />
&nbsp;
<input type="text" name="fieldfive" size="1" maxlength="2" />
&nbsp;

<input type="text" name="fieldsix" size="1" maxlength="2" />
</form>

<h3>Your Numbers</h3>

<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table1">
<tr>
<td width="100"><b>Set One</b></td>
<td width="35" id="1">2</td>
<td width="35" id="2">7</td>

<td width="35" id="3">13</td>
<td width="35" id="4">17</td>
<td width="35" id="5">33</td>
<td width="35" id="6">47</td>
<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetOne,oSetOneIds)" /></td>
</tr>
</table>

<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table2">
<tr>
<td width="100"><b>Set Two</b></td>
<td width="35" id="7">12</td>
<td width="35" id="8">19</td>
<td width="35" id="9">22</td>
<td width="35" id="10">31</td>

<td width="35" id="11">32</td>
<td width="35" id="12">41</td>
<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetTwo,oSetTwoIds)" /></td>
</tr>
</table>
<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table3">
<tr>
<td width="100"><b>Set Three</b></td>

<td width="35" id="13">17</td>
<td width="35" id="14">22</td>
<td width="35" id="15">23</td>
<td width="35" id="16">25</td>
<td width="35" id="17">26</td>
<td width="35" id="18">42</td>

<td><input type="submit" name="submit" id="submit" value="Check"
onClick="CheckNums(oSetThree,oSetThreeIds)" /></td>
</tr>
</table>
<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table4">
<tr>
<td width="100"><b>Set Four</b></td>
<td width="35" id="19">14</td>
<td width="35" id="20">21</td>

<td width="35" id="21">28</td>
<td width="35" id="22">29</td>
<td width="35" id="23">37</td>
<td width="35" id="24">46</td>
<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetFour,oSetFourIds)" /></td>
</tr>
</table>

<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table5">
<tr>
<td width="100"><b>Set Five</b></td>
<td width="35" id="25">1</td>
<td width="35" id="26">5</td>
<td width="35" id="27">6</td>
<td width="35" id="28">15</td>

<td width="35" id="29">18</td>
<td width="35" id="30">35</td>
<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetFive,oSetFiveIds)" /></td>
</tr>
</table>
<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table6">
<tr>
<td width="100"><b>Set Six</b></td>

<td width="35" id="31">10</td>
<td width="35" id="32">11</td>
<td width="35" id="33">31</td>
<td width="35" id="34">36</td>
<td width="35" id="35">38</td>
<td width="35" id="36">48</td>

<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetSix,oSetSixIds)" /></td>
</tr>
</table>
<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table7">
<tr>
<td width="100"><b>Set Seven</b></td>
<td width="35" id="37">3</td>
<td width="35" id="38">4</td>

<td width="35" id="39">15</td>
<td width="35" id="40">23</td>
<td width="35" id="41">36</td>
<td width="35" id="42">49</td>
<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetSeven,oSetSevenIds)" /></td>
</tr>
</table>

<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table8">
<tr>
<td width="100"><b>Set Eight</b></td>
<td width="35" id="43">1</td>
<td width="35" id="44">3</td>
<td width="35" id="45">27</td>
<td width="35" id="46">36</td>

<td width="35" id="47">39</td>
<td width="35" id="48">42</td>
<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetEight,oSetEightIds)" /></td>
</tr>
</table>
<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table9">
<tr>
<td width="100"><b>Set Nine</b></td>

<td width="35" id="49">2</td>
<td width="35" id="50">8</td>
<td width="35" id="51">14</td>
<td width="35" id="52">18</td>
<td width="35" id="53">22</td>
<td width="35" id="54">36</td>

<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetNine,oSetNineIds)" /></td>
</tr>
</table>
<br />
<table cellpadding="5" cellspacing="0" border="1" style="border-
collapse:collapse;" id="table10">
<tr>
<td width="100"><b>Set Ten</b></td>
<td width="35" id="55">5</td>
<td width="35" id="56">23</td>

<td width="35" id="57">36</td>
<td width="35" id="58">40</td>
<td width="35" id="59">43</td>
<td width="35" id="60">45</td>
<td><input type="submit" name="submit" id="submit"
value="Check" onClick="CheckNums(oSetTen,oSetTenIds)" /></td>
</tr>
</table>

</body>
</html>
 
Ad

Advertisements

J

Jeremy J Starcher

Hello,
I am not a good programmer. I wrote a javascript to check my lotto
numbers. I would like feedback on how I could make the script I wrote
'better'. Thanks in advance, I really look forward to seeing different
solutions to the script I wrote. I am sure there there is a much better
way to accomplish the end result.


I don't understand the logic of WHAT the script should do or why,
it might be obvious to lottery players though.

I like to embed documentation into what I do because I suffer from
a bad case of CRS syndrome.

Here is the code:

<html>
<head>
<title>Check Lotto Nums</title>
<script type="text/javascript">

A step ahead of most folks! A valid script declration.
var oSetOne=new Array("2","7","13","17","33","47");
var oSetOneIds=new Array("1","2","3","4","5","6");

These should be numbers, not strings. I'll address that below.

I don't know what the connection is between oSetOne and oSetOneIds.
Could the second be calculated from the former?

In addition, a 2D array would work better than your current arrangement


var myNums = [
[2,7,13,17,33,47],
[12,19,22,31,32,41],
<snip>
[5,23,36,40,43,45]
];

By using JSON (object notation) you could store all the data together in
a single object -- but thats a touch more advanced and wouldn't worry
about right now.

function CheckNums(x,z)
{
var i;

Local variables. That's good.
var y=document.getElementById("myform");

Excellent, not calling 'document.getElementById("myform")' repeatedly. A
better variable name would work.
for(i=0;i<6;i++)

Magic number! (6). You can work out the end of the array if you do it
that way.

{
if(x == y.elements[0].value)


If you switch the above from strings to numbers, then you'll want this
syntax:

if(x === +y.elements[0].value)

Or, easier read:
if(x === +(y.elements[0].value))


The '===' matches type and unary + does a conversion to a number.


No need to repeat this over and over, BTW:
if(x === +(y.elements[ i ].value))
^^^^^^
document.getElementById(z).style.color="red";


Better to specify the RGB. Not all UAs will decode color names.
<style type="text/css" media="screen"> body
{
font-family:Arial;
}
</style>
</head>

<body>


You repeat much of the same information in the tables below as you do in
your arrays above. You could auto-generate that display from your arrays
and not duplicate things.



I'd find another way to display things besides a table, but I didn't give
a lot of thought about your markup.

Include a DOCTYPE. HTML 4.01 strict. (Don't use XHTML.) Specify widths
in your CSS and be sure to include units. You specify "35" a lot, but 35
WHAT?

Best practices say to specify sizes either in em's or percentages.
 
H

HughPugh

I don't understand the logic of WHAT the script should do or why,
it might be obvious to lottery players though.

I like to embed documentation into what I do because I suffer from
a bad case of CRS syndrome.

Reply:
Thank you very much! I now understand that a quoted array member is a
string type, not number type. I didn't know that...

I very much appreciate your feedback. I should have given a
description of what the script is doing for the user(me). I put in the
winning lotto nums in the text input, and then if I have a matching
number in one of my sets, the matching number is set to
style.color="red";

Thanks again!
 
T

Thomas 'PointedEars' Lahn

Jeremy said:
I don't understand the logic of WHAT the script should do or why,
it might be obvious to lottery players though.

I like to embed documentation into what I do because I suffer from
a bad case of CRS syndrome.



A step ahead of most folks! A valid script declration.

Local variables. That's good.

However, since `i' is only used as the iterator value,
it would ease maintenance to declare it there:

for (var i = ...; ...; ...)
These should be numbers, not strings.
Nonsense.

I'll address that below.

Me too.
{
if(x == y.elements[0].value)


If you switch the above from strings to numbers, then you'll want this
syntax:

if(x === +y.elements[0].value)

Or, easier read:
if(x === +(y.elements[0].value))


The '===' matches type and unary + does a conversion to a number.


`+' does convert a value to a number, however it not only converts
representations of decimal values to number. Therefore, parseInt(..., 10)
is more reliable here.

However, using strings for both the values to compare against and foregoing
type conversion entirely is probably most efficient here.
[...]
document.getElementById(z).style.color="red";


Better to specify the RGB. Not all UAs will decode color names.


Then those UAs are broken and don't deserve to be supported.
`red' is a color value that is specified since CSS 1.

Here is a real problem for a change. The last item of a `font-family'
property value should be a generic font-family, to take care of the
event that a font with the listed names is not available.


PointedEars
 
Ad

Advertisements

H

HughPugh

I don't understand the logic of WHAT the script should do or why,
it might be obvious to lottery players though.
I like to embed documentation into what I do because I suffer from
a bad case of CRS syndrome.
A step ahead of most folks! A valid script declration.
Local variables. That's good.

However, since `i' is only used as the iterator value,
it would ease maintenance to declare it there:

for (var i = ...; ...; ...)
These should be numbers, not strings.
Nonsense.

I'll address that below.

Me too.
{
if(x == y.elements[0].value)

If you switch the above from strings to numbers, then you'll want this
syntax:
if(x === +y.elements[0].value)

Or, easier read:
if(x === +(y.elements[0].value))

The '===' matches type and unary + does a conversion to a number.

`+' does convert a value to a number, however it not only converts
representations of decimal values to number. Therefore, parseInt(..., 10)
is more reliable here.

However, using strings for both the values to compare against and foregoing
type conversion entirely is probably most efficient here.
[...]
document.getElementById(z).style.color="red";

Better to specify the RGB. Not all UAs will decode color names.

Then those UAs are broken and don't deserve to be supported.
`red' is a color value that is specified since CSS 1.

Here is a real problem for a change. The last item of a `font-family'
property value should be a generic font-family, to take care of the
event that a font with the listed names is not available.

PointedEars


All points taken and appreciated greatly! Please do understand, I use
this script for my own personal benefit at this time. If I were
posting this on a website for consumption by the masses, I would have
specified a doctype, and I would have definitely ensured that my font-
family included a generic serif or sans-serif declaration.

:D

Thanks so much for the feedback!
 
Ad

Advertisements


Top