1
\$\begingroup\$

What it does is simple: the user inputs a from- and a to-value in a field, sets it with ok. With the go-button, the user makes the program count from f to t and adds it to the site. With delete, the user clears the added markup.

I feel I could simplify the two set-functions into one, but I'm quite stuck.

$(document).ready(function(){

    var f, t

    $("#setfrom").click(function(){

        f = parseInt($("#from").val());

        if (isNaN(f)) {
            $("#test").append("<div>You said you needed numbers, right</div>");
        } else {$("#test").append("<div>" + f + "</div>");
        }
    });

    $("#setto").click(function(){
        
        t = parseInt($("#to").val());

        if (isNaN(t)) {
            $("#test").append("<div>You said you needed numbers, right</div>");
        } else {
            $("#test").append("<div>" + t + "</div>");
        }
   });

    $("#count").click(function(){

        $("#num").append("<div>Okay, here they come:</div>");

        for (i=f; i<=t; i++) {

            $("#num").append("<div>" + i + "</div)");
        }
    });

    $("#delnum").click(function(){
    $("#num").empty();
    $("#test").empty();
});

});
<head>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.2.6/jquery.min.js">          </script>
</head>
<body>
     <h2>Hi, need numbers?</h2>
     <br>

    <p>Yes, from</p>
    <input type="text" id="from"></input>
    <button id="setfrom" class="button">OK</button>

    <p>to</p>
    <input type="text" id="to"></input>
    <button id="setto" class="button">OK</button>

    <br>
    <button id="count" class="button">go!</button>
    <button id="delnum" class="button">delete!</button>
    <div id="test"></div>
    <div id="num"></div>
</body>

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Currently, your presented code will throw a SyntaxError, on line 21 on your Javascript code, making your code broken. As in the help section, broken code is off-topic here. If you had tested the code, you could clearly see that problem. You can try it on jsfiddle.net/ttdpcuco \$\endgroup\$ – Ismael Miguel Nov 14 '15 at 14:50
  • 1
    \$\begingroup\$ That is quite weird. That line has an unfinished string. Javascript doesn't support multi-line strings. You need to escape newlines. Even if Javascript supported multiline strings, the HTML produced after would be invalid. It would create a <div>, but the output would be broken. Check here, to see what I mean: jsfiddle.net/n24Leg89 \$\endgroup\$ – Ismael Miguel Nov 14 '15 at 15:03
  • \$\begingroup\$ Ah, now i see, it's a formatting problem. Copied the code from js-anywhere (on mobile), where it enters a linebreak in the view but not in the code.. \$\endgroup\$ – gelpi Nov 14 '15 at 15:19
  • \$\begingroup\$ Take my suggestion: Delete this question, edit the code to make sure it works, set up a stacksnippet (it's just a button that lets you paste HTML and Javascript there, and lets us run the code right in the page), make sure it works and then undelete it. That way, all formatting mistakes won't have an effect and you can be sure that your code will work everywhere. \$\endgroup\$ – Ismael Miguel Nov 14 '15 at 15:23
2
\$\begingroup\$

Just regarding your question of how to simplify the two set-functions to one, you can merge them in one function which reacts on clicks on both buttons and behaves different depending on the button which has been clicked.

This could look like this:

$("#setfrom, #setto").click(function(event){
    var value;
    if (event.target.id == "setfrom") {
        value = parseInt($('#from').val());
        f = value;
    } else {
        value = parseInt($('#to').val());
        t = value;
    }

    if (isNaN(value)) {
        $("#test").append("<div>You said you needed numbers, right</div>");
    } else {
        $("#test").append("<div>" + value + "</div>");
    }
});

To explain what has changed here, it is important to understand what is happening in the code.

$("#setfrom, #setto") selects all elements which have either "setfrom" or "setto" as the id (look for "id=" in your html code), those are the two buttons.

The next part .click(function(event) { says that the following code should be executed each time that one of these elements is clicked. event is automatically filled by jQuery to contain information about the click of the user. For example event.target contains the concrete element which has been clicked, in this case always one of the two buttons.

We use this to find out, which of the buttons was clicked by the user and read the value from the depending input field (again selected using the $("#something") syntax explained before).

We store the value in two variables:

  • value is used to check if the value is a number and to output either the error or the number
  • f or t (depending on which button was clicked) is (as in your code) stored for later counting from f to t

With broader changes you could avoid the need for the user to click the "OK" buttons by just reading the values whenever the "go!" button is clicked.

This will change the behavior of the formular, but I think it will help you to understand how you can further simplify your code.

To do so you can eliminiate the above function and put the relevant code inside the click handler for the "#count" button:

$("#count").click(function(){

    var from = parseInt($("#from").val());
    var to = parseInt($("#to").val());

    if (isNaN(from) || isNaN(to)) {
        $("#test").append("<div>You said you needed numbers, right</div>");
        return
    }

    $("#num").append("<div>Counting from " + from + " until " + to + "</div>");
    for (i=from; i<=to; i++) {

        $("#num").append("<div>" + i + "</div)");
    }
});

Whenever the button with the id "count" is clicked, the following happens:

The values of the two input fields are read and written to variables from and to. If one of these values is not a number, an error message is printed and the flow is aborted using the return statement.

Otherwise a message is printed showing the user the start and the end of the row to be printed. After that the count is done.

For sure the code and the functionality could be further improved, but I will leave that to you. Just some hints what could be your next steps to improve this code:

  • Not always appending the output, but replacing old output and thus avoiding the user to need to click the "delete!" button
  • Extracting the duplicate code of reading values from input fields using functions

For the style of your code, I would recommend a few points:

  • Make sure that every new command is on a new line. Putting multiple things in one line will make it a lot harder to find out what what is happening while reading your code. In most cases you will spend more time reading your code than writing it, thus readability is very important. This applies even for control structures and commands like this line in your code: } else {$("#test").append("<div>" + f + "</div>");
  • As already said in the comments (and afterwards heavily improved by you): Look for correct indentation of your code. The three lines following $("#delnum").click(function() { should each be indented one (more) level. This would make it even easier to see that those lines are somehow depending on this line.
  • Do give your variables a speaking name. E.g. f and t could be renamed to from and to. Even in a small program like this one that helps to understand what each variable means. Keep in mind that others might want/need to understand your code - including yourself in a later point of time, when you are not completely aware of what you did there anymore.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.