Before other complaints come in, Java is a language for creating applications which has its own completely separate Java and JSP forum. JavaScript by comparison is not an application language, it is a scripting language instead. They are completely different and have about as much similarity to each other as a cup has to do with cupboards. Please learn this.
On to your code.
Problem 1: the variable called Integer is not defined. I presume that you’re just wanting to use the parseInt function instead. If that’s the case, you’ll also want to specify the second parameter of 10, which is very important given that people can enter “08” or “09” which parseInt will automatically interpret as being octal values. Force it to work in base 10 and you won’t have any potential bugs there.
var i = parseInt(newmarkup.trim(), 10);
Problem 2: Form values are strings, so when you do buy + freight, JavaScript will be concatenating strings, it won’t be adding any numbers together. If you expect them to be numbers, make sure that they are received from the form as numbers. If it’s not a number in the form then give a default value instead.
var buy = Number(document.getElementById(x).value) || 0;
var freight = Number(document.getElementById(y).value) || 0;
var markup = Number(document.getElementById(z).value) || 0;
Problem 3: document.write is deprecated for a good reason. Once the page has finished loading, any attempt to use document.write will completely wipe the page and start again with nothing.
Instead of that, you need to be able to target a certain part of the existing page, so that you can update it with your new info. When dealing with in-page content the innerHTML function can be useful for that.
Here is an update to the HTML that provides you a place to put the updated info.
Sell Price: <span id="sell"></span>
where you could update the contents of the span with:
document.getElementById('sell').innerHTML = sell;
Now for some improvements.
All of the var statements can be shifted to a single statement at the start. This way you can see the declared intentions right off, and it’s easier to rework the code.
var buy, freight, markup, newmarkup, sell, markupamount, i;
These next four lines can be improved quite a lot now, by some refactoring:
sell = buy + freight;
newmarkup= "0." + markup;
i = parseInt(newmarkup.trim());
markupamount=sell*i;
sell = sell + markupamount;
Guess what happens when newmarkup is “0.20”. The value of i will be 0, so there’s no markup.
The newmarkup calculation is the biggest problem. Strings and numbers should not be added together. Now that buy, freight and markup are actual numbers, the above 5 lines can be correctly reduced to this one line:
sell = (buy + freight) * (1 + markup / 100);
Where a 20% markup means multiplying by 1.2
Another improvement could be to not pass in all of the id values to the function. Currently there are three separate inline events that are required on the inputs. Consider an improvement where a common parent, such as a form element, has just the one onchange event that passes the form as a reference itself. That way the inputs can be referenced from within the script as e.g., form.elements.buy
This allows the HTML code to be much cleaner. Because you wrap the label around the input, there is already an implicit association with the label, so the for attribute doesn’t need to be specified either:
<form id="sales">
<div>
<label>Buy Price:
<input type="text" name="buy" value="13"/>
</label>
</div>
<div>
<label>Freight:
<input type="text" name="freight" value="7"/>
</label>
</div>
<div>
<label>Markup:
<input type="text" name="markup" value="20"/>
</label>%
</div>
<div>
Sell Price: <span id="sell"></span>
</div>
</form>
<script type="text/javascript">
...
</script>
And a simple script at the end of the body then attaches an onchange event to the form.
Here’s the final script after all of these updates:
document.getElementById('sales').onchange = function() {
solve(this);
};
function solve(form) {
var buy, freight, markup, sell;
buy = Number(form.elements.buy.value) || 0;
freight = Number(form.elements.freight.value) || 0;
markup = Number(form.elements.markup.value) || 0;
sell = (buy + freight) * (1 + markup / 100);
document.getElementById('sell').innerHTML = sell;
}
Edit:
IE fails to bubble the onchange event. See below for update