JS Code Advice (greatly appreciated)

Discussion in 'Javascript' started by rupoopin@gmail.com, Feb 22, 2008.

  1. Guest

    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>
     
    , Feb 22, 2008
    #1
    1. Advertising

  2. On Fri, 22 Feb 2008 13:03:27 -0800, rupoopin wrote:

    > 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.
     
    Jeremy J Starcher, Feb 23, 2008
    #2
    1. Advertising

  3. HughPugh Guest

    On Feb 22, 6:06 pm, Jeremy J Starcher <> wrote:
    > On Fri, 22 Feb 2008 13:03:27 -0800, rupoopin wrote:
    > > 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.
    >
    >
    >


    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!
     
    HughPugh, Feb 23, 2008
    #3
  4. On Fri, 22 Feb 2008 19:25:16 -0800, HughPugh wrote:
    > Thank you very much! I now understand that a quoted array member is a
    > string type, not number type. I didn't know that...


    *nods*

    Understanding types in Javascript can be a hurdle for someone getting
    into it. Because of automatic type conversion, Javascript sometimes
    *appears* typeless.

    This might be of some use:
    http://www.jibbering.com/faq/faq_notes/type_convert.html
     
    Jeremy J Starcher, Feb 23, 2008
    #4
  5. Jeremy J Starcher wrote:
    > On Fri, 22 Feb 2008 13:03:27 -0800, rupoopin wrote:
    >> 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 i;

    >
    > 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 = ...; ...; ...)

    >> 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.


    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.

    >> <style type="text/css" media="screen"> body
    >> {
    >> font-family:Arial;
    >> }


    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
     
    Thomas 'PointedEars' Lahn, Feb 24, 2008
    #5
  6. HughPugh Guest

    On Feb 24, 7:21 am, Thomas 'PointedEars' Lahn <>
    wrote:
    > Jeremy J Starcher wrote:
    > > On Fri, 22 Feb 2008 13:03:27 -0800, rupoopin wrote:
    > >> 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 i;

    >
    > > 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 = ...; ...; ...)
    >
    > >> 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.

    >
    > 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.
    >
    > >> <style type="text/css" media="screen"> body
    > >> {
    > >> font-family:Arial;
    > >> }

    >
    > 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!
     
    HughPugh, Feb 24, 2008
    #6
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Simon Harvey
    Replies:
    1
    Views:
    328
    Guest
    Apr 2, 2004
  2. Tom
    Replies:
    3
    Views:
    2,498
  3. Kirok
    Replies:
    2
    Views:
    350
    Kirok
    Nov 10, 2005
  4. Deryck
    Replies:
    7
    Views:
    475
  5. windandwaves

    feedback greatly appreciated

    windandwaves, Oct 6, 2005, in forum: HTML
    Replies:
    12
    Views:
    695
Loading...

Share This Page