Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I am trying to write a price calculator for proof reading using javascript. It works but I am sure that my code could be improved quite a bit.

The concept is as follows:

There are three factors affecting the price:

  • word count
  • proof reading type
  • timeline (how quickly the document is proof read)

The user enters a word count and then selects a proof reading type.

The timeline select box (which has various timeline options such as 6hours, 24hours etc) will be generated based on the wordcount.

  • If the word count is less than 5000 there will be 5 options
  • If the word count is between 5000-10000, there will be only four options (it's not humanly possible to proof read 10000 words in 6 hours)
  • and so on

Can you have a look at the code and tell me how this can be improved?

A working version of the code can be found on jsfiddle here:

http://jsfiddle.net/8K6wr/

Here is the source:

<!DOCTYPE html>
<html>
<head>
<script type="text/javascript">
function getQuantity() {
    var theForm = document.forms["pricequote"];
    var quantity = theForm.elements["quantity"];
    var howmany =0;
    if(quantity.value!="") {
        howmany = parseInt(quantity.value);
    }
    return howmany;
}

function ClearOptionsFast(hoursbased)
{
    var selectObj = document.getElementById('hoursbased');
    var selectParentNode = selectObj.parentNode;
    var newSelectObj = selectObj.cloneNode(false); // Make a shallow copy
    selectParentNode.replaceChild(newSelectObj, selectObj);
    return newSelectObj;
}

function setOption1(arrayvals) {
    document.pricequote.hoursbased.options[0]=new Option("Select Hour Specifications","");
    document.pricequote.hoursbased.options[1]=new Option("6 Hours",arrayvals[0]);
    document.pricequote.hoursbased.options[2]=new Option("24 Hours",arrayvals[1]);
    document.pricequote.hoursbased.options[3]=new Option("48 Hours",arrayvals[2]);
    document.pricequote.hoursbased.options[4]=new Option("1 Week",arrayvals[3]);
    document.pricequote.hoursbased.options[5]=new Option("2 Weeks",arrayvals[4]);
    return true;
}
function setOption2(arrayvals) {
    document.pricequote.hoursbased.options[0]=new Option("Select Hour Specifications","");
    document.pricequote.hoursbased.options[1]=new Option("24 Hours",arrayvals[1]);
    document.pricequote.hoursbased.options[2]=new Option("48 Hours",arrayvals[2]);
    document.pricequote.hoursbased.options[3]=new Option("1 Week",arrayvals[3]);
    document.pricequote.hoursbased.options[4]=new Option("2 Weeks",arrayvals[4]);
    return true;
}
function setOption3(arrayvals) {
    document.pricequote.hoursbased.options[0]=new Option("Select Hour Specifications","");
    document.pricequote.hoursbased.options[1]=new Option("48 Hours",arrayvals[2]);
    document.pricequote.hoursbased.options[2]=new Option("1 Week",arrayvals[3]);
    document.pricequote.hoursbased.options[3]=new Option("2 Weeks",arrayvals[4]);
    return true;
}
function setOption4(arrayvals) {
    document.pricequote.hoursbased.options[0]=new Option("Select Hour Specifications","");
    document.pricequote.hoursbased.options[1]=new Option("1 Week",arrayvals[3]);
    document.pricequote.hoursbased.options[2]=new Option("2 Weeks",arrayvals[4]);
    return true;
}
function setOption5(arrayvals) {
    document.pricequote.hoursbased.options[0]=new Option("Select Hour Specifications","");
    document.pricequote.hoursbased.options[1]=new Option("2 Weeks",arrayvals[4]);
    return true;
}

function fixUnitPrice(proofarray) {
    if(getQuantity() <= 5000) {
        ClearOptionsFast('MySelect');
        setOption1(proofarray);
    }
    else if(getQuantity() >= 5000 && getQuantity() <= 10000){
        ClearOptionsFast('MySelect');
        setOption2(proofarray);
    }
    else if(getQuantity() >= 10000 && getQuantity() <= 30000){
        ClearOptionsFast('MySelect');
        setOption3(proofarray);
    }
    else if(getQuantity() >= 30000 && getQuantity() <= 60000){
        ClearOptionsFast('MySelect');
        setOption4(proofarray);
    }
    else if(getQuantity() >= 60000 && getQuantity() <= 100000){
        ClearOptionsFast('MySelect');
        setOption5(proofarray);
    }
    else {
        ClearOptionsFast('MySelect');
    }
    return true;
}
function hourBasisPrice(documtype) {
    document.pricequote.hoursbased.options.length = 0;

    switch (documtype) {
    case "students":
        var studarray = [0.8,0.6,0.5,0.45,0.4];
        fixUnitPrice(studarray);
    break;
    case "academicians":
        var acadarray = [1.0,0.7,0.6,0.5,0.45];
        fixUnitPrice(acadarray);
    break;
    case "professionals":
        var profarray = [1.2,0.8,0.7,0.6,0.55];
        fixUnitPrice(profarray);
    break;
    case "personal":
        var persarray = [1.4,0.9,0.8,0.7,0.65];
        fixUnitPrice(profarray);
    break;
    }
    return true;
}

function getTotal() {
var e = document.getElementById("hoursbased");
var strUser = e.options[e.selectedIndex].value;
var totalPrice = getQuantity() * strUser;

    //display the result
    document.getElementById('totalPrice').innerHTML =
                                      "Total Price For Proofreading USD"+totalPrice;

}
</script>
</head>
<body>
    <form action="#" id="pricequote" name="pricequote" onsubmit="return false">
    <table width="501" border="0" cellpadding="10" cellspacing="20" style="padding-top:30px;">
        <tr>
            <th scope="row">Word Count:</th>
                <td><input type="text"  name="quantity" id="quantity" onchange="javascript: hourBasisPrice(document.pricequote.size.options[document.pricequote.size.selectedIndex].value);"/></td>
        </tr>
        <tr>
            <th width="67" scope="row">Type:</th>
                <td width="273" class="select-box">  
                    <select id="type" name="size" onchange="javascript: hourBasisPrice(this.options[this.selectedIndex].value);">
                        <option value="None">Select Type</option>
                        <option value="students">Proofreading for students</option>
                        <option value="academicians">Proofreading for academicians</option>
                        <option value="professionals">Proofreading for Professionals/Businesses</option>
                        <option value="personal">Proofreading &amp; editing personal documents</option>
                    </select>
                </td>
        </tr>

        <tr>
            <th width="67" scope="row">Hour Specifications:</th>
                <td width="273" class="select-box">  
                    <select id="hoursbased" name="hoursbased">
                        <option value="">Select Hour Specifications</option>
                    </select>
                </td>
        </tr>



        <tr>
            <th scope="row">&nbsp;</th>
                <td><input class="button" type="button" value="Update" onmousedown="getTotal()"/></td>
        </tr>

        <tr>
            <th> Price:</th>
            <td><div id="totalPrice" style="float:right;"></div></td>
        </tr>
    </table>
</form>
</body>
</html>​
share|improve this question
Your calculation is not clear .. can you break it down to simple formula x + y etc .... – Baba Oct 9 '12 at 20:24
@Baba If you can have a look at the code and its result, it may give you an idea i believe... – user1732144 Oct 9 '12 at 20:45

migrated from stackoverflow.com Oct 9 '12 at 23:08

1 Answer

up vote 1 down vote accepted

You repeat yourself alot. You always want to keep things DRY (don't repeat yourself). Look at your code. If you see sections that are similar but different in small ways combine the similarities.

e.g. in your hourBasisPrice function you do the same thing for each switch option. Establish an array and then pass on to the other function. Instead I would establish an array with all of this info and then perform a lookup. Check the code below. All the values are in one place and then the work is done at the bottom.

var priceArray = [
        ["students",      [0.8, 0.6, 0.5, 0.45, 0.40]],
        ["academicians",  [1.0, 0.7, 0.6, 0.50, 0.45]],
        ["professionals", [1.2, 0.8, 0.7, 0.60, 0.55]],
        ["personal",      [1.4, 0.9, 0.8, 0.70, 0.65]]
    ]

    for (i = 0; i < priceArray.length; i++) {
        if (priceArray[i][0] == documtype) {
            fixUnitPrice(priceArray[i][1]);
            break;
        }
    }

Your fixUnitPrice function does a few things that I would change. One is that you call getQuantity over and over again. If the value was 75,000 then it would have to calculate the quantity nine times. Call it once and put the value into a local variable. Then check that value.

But better yet would be to rethink the adding of options completely. You are checking whether a value is between two values and then adding options accordingly. But the options are all the same (with the lower ones missing). So really you are adding an option if the value is below a certain threshold. This is how I would handle this. I build another lookup table and check if the hours worked (notice the more descriptive variable name) is less than the threshold. Then I return the available options.

function getAllowedOptions(rateArray) {
    var hoursWorked = getQuantity();

    var hourlyTable = [
        [5000, "6 Hours"],
        [10000, "24 Hours"],
        [30000, "48 Hours"],
        [60000, "1 Week"], 
        [100000, "2 Weeks"]
    ]

    var options = [];

    for (i = 0; i < hourlyTable.length; i++) {
        if (hoursWorked < hourlyTable[i][0]) { 
            options.push(new Option(hourlyTable[i][1], rateArray[i]));
        }
    }

    return options;
}

This would change the other function as follows:

function fixUnitPrice(rateArray) {

    var options = getAllowedOptions(rateArray);

    ClearOptionsFast('MySelect');

    document.pricequote.hoursbased.options[0] = new Option("Select Hour Specifications", ""); // always added

    for (i = 0; i < options.length; i++) {
        document.pricequote.hoursbased.options.add(options[i]);
    }
    return true; // not sure why we are returning true here but I left it.
}

I did not look at the other functions. These jumped out at me as the problem.

http://jsfiddle.net/8K6wr/2/

share|improve this answer
Thanks for your answer...I have learned so many things from your answer... Is it possible to update the totalprice as soon as we change the word-count to increase or decrease? I have edited the code of yours with new variable names... and Here it is jsfiddle.net/YeCSh Thanks again... Thanks a lot – user1732144 Oct 10 '12 at 10:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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