-1

I'm working on a tip calculator in Javascript where the user should be able to type in the amount of the bill, push "calculate", and see both the tip and the total (which includes the bill and the tip).

I've been working at this for a while, but can't seem to make it work correctly.

<!DOCTYPE html>
<head>
<title>Tip Calculator</title>
<link href="style.css" rel="stylesheet" type="text/css">
</head>

<body>
<div id="wrapper">
    <header>
        <h1>Tip Calculator</h1>
    </header>
    <form>
        Amount: $<input id="bill" type="text">
        <br>
        <button onclick="calc()">Calculate</button>
        <br>
        Tip: <span id="tip"></span>
        <br>
        Total: <span id="total"></span>
    </form>
</div>
<script>
    function calc() {
        var bill = document.getElementById('bill').value;
        var tip = bill * .15;
        var total = bill + tip;

        document.getElementById("tip").innerHTML= "$"+(tip).toFixed(2);
        document.getElementById("total").innerHTML= "$"+(total).toFixed(2);         
    }
</script>
</body>

Any suggestions?

Jonathan Hall
  • 75,165
  • 16
  • 143
  • 189
user3221658
  • 13
  • 1
  • 1
  • 3

6 Answers6

4

You have a few problems. You need the event passed by the click function so that you can stop the form from submitting traditionally, which causes a page refresh. The best practice is to attach the click handler with javascript. You also need to convert the string value of the input into an actual number so that it can be used for addition.

Live demo (click).

// get element references - no need to keep repeating this in the function
var myBtn = document.getElementById('my-button');
var tipElem = document.getElementById("tip");
var totalElem = document.getElementById("total");

//attach click event listener
myBtn.addEventListener('click', calc);

//the event object is passed to this automatically - I'm assigning it to "e"
function calc(e) {
  //prevent the default action - form submit / page refresh
  e.preventDefault();
  var input = document.getElementById('bill');
  //convert string value to Number
  var bill = parseFloat(input.value);
  var tip = bill * 0.15;
  var total = bill + tip;

  //textContent is better than innerHTML for setting text
  tipElem.textContent = "$"+(tip).toFixed(2);
  totalElem.textContent = "$"+(total).toFixed(2);  
}

Here is the markup I recommend:

<form>
  <p>Amount: $<input id="bill" type="number"></p>
  <button id="my-button">Calculate</button>
  <p>Tip: <span id="tip"></span></p>
  <p>Total: <span id="total"></span></p>
</form>

<br> should not be used for layout. Instead, you can use display: block on the elements that should go to a new line. Many elements do this by default, such a <p>. I also changed the input type to number because that that seems more appropriate here.

Unfortunately, some people are going to suggest and argue that inline js (that's any kind of javascript in your html, like onclick) is ok to use. It isn't. I hope you will read some of these results: Why is inline JS bad?

m59
  • 43,214
  • 14
  • 119
  • 136
  • This example will be broken in older versions of IE. If you are messing with event listeners, you need to consider http://stackoverflow.com/questions/6927637/addeventlistener-in-internet-explorer (or just use a lib that does this correctly). – Kevin Seifert Jan 22 '14 at 02:34
  • @user1389596 It's worth asking yourself though "do users of old versions of IE (or current versions) deserve code to work for them?" – Rhyono Jan 22 '14 at 02:36
  • 1
    @user1389596 the important thing is to follow the professionally accepted standards first and implement backwards compatible solutions, not to disregard best practices altogether just because they may need to be shimmed. My aim in answering questions is to promote a world of better developers, not lazy ones with high SO reputation. – m59 Jan 22 '14 at 02:37
  • 2
    Arguing for support in old IE is just a bit silly. It leads to less maintainable code, not to mention having an actual cost associated with it, for a very small number of users. This is a good answer. – phenomnomnominal Jan 22 '14 at 02:41
  • A third of the planet is probably running Windows XP, and the corporate world is also behind in IE versions. What needs to be supported is not always up to the developer. – Kevin Seifert Jan 22 '14 at 02:44
  • @phenomnomnominal, personally I find the old-school method a bit more maintainable. It follows the ASP.net model. It's the same number of lines of code to do it either way. The advantage of explicit handlers is you can inspect the element and see the event binding instantly. If you've ever had to hunt for an event listener in a large project, you'll start to question the benefit of extreme code decoupling. Especially if you don't even know to what/how it's been attached... it's not fun. My $.02 :) – Kevin Seifert Jan 22 '14 at 02:50
2

Forms are typically used to submit data to a server for various reasons. However, they have some benefits even when everything is being done on the same page (such as being able to press enter in any input and still have it submit without using extra code). What I did here was give it an action of the function you wanted to use and then changed your button to a standard submit.

There are various methods for taking a string and making it into a number, I used the simplest here: Number(). I used it on bill in order to make the total_bill display properly (instead of adding two strings together which just places them side by side). I changed total to total_bill for the variable because it is just not a good idea to name multiple things the same (e.g. the id and the variable). I missed the bill/bill, but I'll leave it as is because it does usually still work (like here).

Lastly, toFixed() takes a number and makes it a string. Which means you have to make sure it's a number and not already a string. This is why Number() was used again before outputting.

Try this:

<!DOCTYPE html>
<head>
<title>Tip Calculator</title>
<link href="style.css" rel="stylesheet" type="text/css">
</head>

<body>
<div id="wrapper">
    <header>
        <h1>Tip Calculator</h1>
    </header>
    <form action="javascript:void(calc())">
        Amount: $<input id="bill" type="text">
        <br>
        <input type="submit" value="Calculate">
        <br>
        Tip: <span id="tip"></span>
        <br>
        Total: <span id="total"></span>
    </form>
</div>
<script>
    function calc() {
        var bill = Number(document.getElementById('bill').value);
        var tip = bill * .15;
        var total_bill = bill + tip;

        document.getElementById("tip").innerHTML= "$"+Number(tip).toFixed(2);
        document.getElementById("total").innerHTML= "$"+Number(total_bill).toFixed(2);         
    }
</script>
</body>
Rhyono
  • 2,420
  • 1
  • 25
  • 41
  • Thanks, this worked perfectly! I really appreciate you taking the time to explain the modified code. Thanks again! – user3221658 Jan 22 '14 at 02:18
  • @user3221658 Accept whatever, I have plenty of rep, but PLEASE consider the best practices I show you and explain in my answer. Please. I spent a lot of time preparing that for you. – m59 Jan 22 '14 at 02:22
  • ^ I can't agree enough. Please don't use inline JS. Use an MVC approach and you'll be set – Sterling Archer Jan 22 '14 at 02:23
  • 1
    @m59 I like to fix a person's code while keeping as true to what they original had as possible. You are telling them to use parseInt with a specified base 10...this is money, which has hundredths places. Wutrudoin? – Rhyono Jan 22 '14 at 02:25
  • @Rhyono Good point - I updated. As for the philosophy - the majority of code I see coming from the web world is rife with awful practices. It is impossible for me to see SO posts that also have bad practices all over them and not think it's a related issue. I think it's therefore very important that we evangelize the best practices as often and as passionately as we can. Just think how much identity theft could have been prevented if people saw a desperate please about prepared statements and listened to it. – m59 Jan 22 '14 at 03:06
  • Please don't encourage javascript: urls, nor inline JS. – rvighne Jan 22 '14 at 03:40
1

Try a standard input button

<input type="button" onclick="calc()" value="Calculate">

Also, the toFixed() method doesn't necessarily exist on the var. In testing in Chrome, it will work however if you explicitly cast to number

    document.getElementById("tip").innerHTML= "$"+(new Number(tip)).toFixed(2);
    document.getElementById("total").innerHTML= "$"+(new Number(total)).toFixed(2);
Kevin Seifert
  • 3,494
  • 1
  • 18
  • 14
  • 1
    Sad. Upvotes for inline javascript. This is terrible practice and should never be used or recommended. Also, if you want a button, use a button. – m59 Jan 22 '14 at 02:02
  • 1
    Button doesn't work in older browsers. Inline js is not terrible practice as long as it's sparse. This is the same practice used in ASP.net. Otherwise, I dislike hunting through thousands of lines of external javascript files just to find an event binding. – Kevin Seifert Jan 22 '14 at 02:05
  • Believe what you will, but "sparse" is no excuse. It's ok to use it to test something really quickly - that's it. – m59 Jan 22 '14 at 02:07
  • 2
    You really shouldn't advocate inline JS – Sterling Archer Jan 22 '14 at 02:08
  • I agree that inlining js is not a good idea. But the "best practice" advice that goes for extreme decoupling is a bit overboard. The code in his example is perfectly readible. I'd sooner suggest using jquery than mess with event binders without the lib. But that opens up another can of worms. – Kevin Seifert Jan 22 '14 at 02:17
  • 1
    event binding is quite simple. @m59 has an answer that isn't difficult to read at all. And, it's not overhead in the slightest to use a good practice for even the smallest of tasks – Sterling Archer Jan 22 '14 at 02:19
  • The only thing of course is that example is completely broken in older versions of IE ... :) I don't understand a best practice that doesn't work 100% of the time. – Kevin Seifert Jan 22 '14 at 02:23
  • Oh, and btw, the fact that you'd advocate a whole library of JavaScript in leu of MCV scripting scares me – Sterling Archer Jan 22 '14 at 02:26
  • If you are messing with event listeners and want cross browser support, you need to do this differently or rely on a lib. The example provided is broken in older versions of IE. – Kevin Seifert Jan 22 '14 at 02:32
0

Try to use this

function calc() {
    var bill = $("#bill").value;
    var tip = bill * .15;
    var total = bill + tip;

    document.getElementById("tip").innerHTML= "$"+(tip).toFixed(2);
    document.getElementById("total").innerHTML= "$"+(total).toFixed(2);    

}

Also try to create JSFIDDLE for you to create some demo.

Marc
  • 139
  • 1
  • 7
0

  1. Your form is being submitted hence it is losing values. There're many way to avoid this - one - define your form as

    <form onsubmit="return false">

  2. Your total won't calculate because bill is read as string. One way to force it into number is multiply by 1, so total will be

    var total = bill*1 + tip;

Here's working demo: http://jsfiddle.net/5GQZ8/

Yuriy Galanter
  • 38,833
  • 15
  • 69
  • 136
-1
<!DOCTYPE html>
<head>
<title>Tip Calculator</title>
<link href="style.css" rel="stylesheet" type="text/css">
</head>

<body>
<div id="wrapper">
    <header>
        <h1>Tip Calculator</h1>
    </header>
    <form>
        Amount: $<input id="bill" type="text">
        <br>
        <a onclick="calc()">Calculate</a>
        <br>
        Tip: <span id="tip"></span>
        <br>
        Total: <span id="total"></span>
    </form>
</div>
<script>
    function calc() {
        var bill = document.getElementById('bill').value;
        var tip = bill * .15;
        var btotal = bill + tip;

        document.getElementById("tip").innerHTML= "$"+Number(tip);
        document.getElementById("total").innerHTML= "$"+Number(btotal);
    }
</script>
</body>
Coder
  • 1
  • 1
    Your answer actually doesn't fix the majority of the problems. You may want to test code prior to posting it in the future. – Rhyono Jan 22 '14 at 02:17