5
\$\begingroup\$

Thanks to this post, I was able to create a working piece of javascript that fits my needs. I have a div/ul/li based table where each "row" has 2 values that I want to have the sum for. Currently the code does exactly what I want, but somehow I have the feeling that the javascript is not coded in the most efficient way since I am repeating a lot of code. Any suggestions?

$(calculateSum1);
	function calculateSum1() {
	var sum1 = 0;
	// searching li for equal class and add the values
	$(".pr1").each(function() {
    var value = $(this).text();
    // add only if the value is number
    if(!isNaN(value) && value.length != 0) {
        sum1 += parseFloat(value);
    }
});
	$('#result1').text(sum1);
};

$(calculateSum2);
	function calculateSum2() {
	var sum2 = 0;
	// searching li for equal class and add the values
	$(".pr2").each(function() {
    var value = $(this).text();
    // add only if the value is number
    if(!isNaN(value) && value.length != 0) {
        sum2 += parseFloat(value);
    }
});
	$('#result2').text(sum2);
};

$(calculateSum3);
	function calculateSum3() {
	var sum3 = 0;
	// searching li for equal class and add the values
	$(".pr3").each(function() {
    var value = $(this).text();
    // add only if the value is number
    if(!isNaN(value) && value.length != 0) {
        sum3 += parseFloat(value);
    }
});
	$('#result3').text(sum3);
};

$(calculateSum4);
	function calculateSum4() {
	var sum4 = 0;
	// searching li for equal class and add the values
	$(".pr4").each(function() {
    var value = $(this).text();
    // add only if the value is number
    if(!isNaN(value) && value.length != 0) {
        sum4 += parseFloat(value);
    }
});
	$('#result4').text(sum4);
};
div.col, div.price {float:left}
div.col {width:25%;margin;none;}
ul {list-style-type: none;margin:0;}
<html>
<head>
<script type="text/javascript" src="//code.jquery.com/jquery-2.1.0.js"></script>
</head>
<body>
<div class="col price">
<ul>
<li>Price</li>
<li class="txt pr1">150</li>
<li class="txt pr2">199</li>
<li class="txt pr3">249</li>
<li class="txt pr4">299</li>
</ul>
</div>
<div class="add price">
<ul>
<li>+</li>
<li>+</li>
<li>+</li>
<li>+</li>
<li>+</li>
</ul>
</div>
<div class="col price">
<ul>
<li>Add costs</li>
<li class="txt pr1">27</li>
<li class="txt pr2">20</li>
<li class="txt pr3">30</li>
<li class="txt pr4">39</li>
</ul>
</div>
<div class="add price">
<ul>
<li>=</li>
<li>=</li>
<li>=</li>
<li>=</li>
<li>=</li>
</ul>
</div>
<div class="col price">
<ul>
<li>Totaal</li>
<li><a id="result1" href=""></a></li>
<li><a id="result2" href=""></a></li>
<li><a id="result3" href=""></a></li>
<li><a id="result4" href=""></a></li>
</ul>
</div>
</body>
</html>

\$\endgroup\$
6
  • 1
    \$\begingroup\$ You may want to look into using objects and arrays instead of multiple variables to structure your data. \$\endgroup\$
    – gcampbell
    Jul 17 '16 at 11:45
  • \$\begingroup\$ I am pretty new to other coding than HTML, CSS. Array / objects is PHP right? Are can this still be done via javascript that includes magic with arrays? \$\endgroup\$
    – Louis
    Jul 17 '16 at 13:34
  • \$\begingroup\$ In JavaScript [foo, bar, baz] creates an array; { foo: x, bar: y, baz: z } creates an object. Objects are usually used as associative arrays / dictionaries / hashes, as before ES6 (ECMAScript 6) there wasn't a "proper" way to do it. (And don't confuse the object literal with the JSON data format, or you will be chased by a screaming pack of wolves.) \$\endgroup\$
    – gcampbell
    Jul 17 '16 at 13:40
  • \$\begingroup\$ Is there some reason why you are avoiding the use of HTML tables? \$\endgroup\$ Jul 17 '16 at 14:25
  • \$\begingroup\$ @200_success faster will a html table contribute to a more efficient javascript code? If not, what would be the reason switching to a table? \$\endgroup\$
    – Louis
    Jul 17 '16 at 14:52
5
\$\begingroup\$

I think you could rewrite the code in a shorter way.

You can write the calculateSum# function to be generic.

This you can do by providing the selector or directly the jQuery object you need to loop and sum values.

And instead to directly write the result, you could return the computation.

Of course you could pass the result selector or jQuery element and do the job, but if you return the result, the code is more changeble in the future.

Then you create an object literal with the keys as your element selector, and the values as the result element selector.

And finally create an anonymous function to pass on jQuery document ready step $(myFuncOnDocReadyHere...), where you put all together.

If you prefer to avoid sharing the object with elements you could place it inside the on doc ready function, to keep it "private".

Here is my changes:

var summableAndResult = {
	".pr1": "#result1",
	".pr2": "#result2",
	".pr3": "#result3",
	".pr4": "#result4"
}

function calculateSum ($list) {
	var sum = 0;

	$list.each(function() {
		var value = $(this).text();
		// test isNaN only if string is not empty
		if (value.length != 0 && !isNaN(value)) {
			sum += parseFloat(value);
		}
	});
	
	return sum;
}

$(function(){

	for(var el in summableAndResult) {
		var sum = calculateSum($(el));
		$(summableAndResult[el]).text(sum);
	}
	
});
div.col, div.price {float:left}
div.col {width:25%;margin;none;}
ul {list-style-type: none;margin:0;}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<html>
<head>
<script type="text/javascript" src="//code.jquery.com/jquery-2.1.0.js"></script>
</head>
<body>
<div class="col price">
<ul>
<li>Price</li>
<li class="txt pr1">150</li>
<li class="txt pr2">199</li>
<li class="txt pr3">249</li>
<li class="txt pr4">299</li>
</ul>
</div>
<div class="add price">
<ul>
<li>+</li>
<li>+</li>
<li>+</li>
<li>+</li>
<li>+</li>
</ul>
</div>
<div class="col price">
<ul>
<li>Add costs</li>
<li class="txt pr1">27</li>
<li class="txt pr2">20</li>
<li class="txt pr3">30</li>
<li class="txt pr4">39</li>
</ul>
</div>
<div class="add price">
<ul>
<li>=</li>
<li>=</li>
<li>=</li>
<li>=</li>
<li>=</li>
</ul>
</div>
<div class="col price">
<ul>
<li>Totaal</li>
<li><a id="result1" href=""></a></li>
<li><a id="result2" href=""></a></li>
<li><a id="result3" href=""></a></li>
<li><a id="result4" href=""></a></li>
</ul>
</div>
</body>
</html>

\$\endgroup\$
3
\$\begingroup\$

Just passing by, I thought I should make things a little bit simpler and neater by using a table rather than divs with ul but the example can be extended to suit other scenarios. The changes I made is below:

  1. As the other reviewers clearly stated before, using objects or Arrays are better off than declaring variables. To solve the problem I used JQuery to extract the column price and addedCosts as an Array. So manipulating it as an array avoids DRY e.g

    var price = new Array(); $("#table1 tbody tr td:nth-child(1)").each(function (i) { price.push($(this).text()); });

  2. I believe the four sum functions could be reduced to one. Hence I created a sum function that takes as input two arrays; these are supplied from the extracted arrays (price and addedCosts) using JQuery as above. Also, notice that I only perform the calculation when the two arrays have equal length.

  3. Notice the sum function returns an array(the sum of the two arrays). I used Javascript to insert the array generated from the calculation into the third column(Total)

  4. Just to beautify the table I used bootstrap css. Twitter bootstrap is reusable so you can use it in your code.

var price = new Array();
var addedCosts = new Array();
var total = new Array();
$("#table1  tbody tr td:nth-child(1)").each(function (i) {
    price.push($(this).text());
});

$("#table1 tbody tr td:nth-child(2)").each(function (j) {
    addedCosts.push($(this).text());
});
function sum(array1, array2) {

    if (array1.length == array2.length) {


        for (var i = 0; i < array1.length; i++) {
            total.push(parseInt(array1[i]) + parseInt(array2[i]))

        }
        return total;
    }
}
var result = sum(price, addedCosts);


var table = document.getElementById("table1"),
rows = table.rows, rowcount = rows.length, r,
  cells;


for (r = 1; r < rowcount; r++) {
    cells = rows[r].cells;   
    cells[2].innerHTML = result[r - 1];
}
<link href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css" rel="stylesheet"/>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>
<table id="table1" class="table table-striped table-hover">
<thead>
<tr>
    <td>Price</td>
    <td>Add Costs</td>
    <td>Total</td>
</tr>
</thead>
<tbody>
<tr>
<td> 150</td>
<td>27</td>
<td></td>
</tr>
<tr>
<td>199</td>
<td>20</td>
<td></td>
</tr>
<tr>
<td>249</td>
<td>30</td>
<td></td>
</tr>
<tr>
<td>299</td>
<td>39</td>
<td></td>
</tr>
</tbody>
</table>

\$\endgroup\$
2
  • \$\begingroup\$ Wow!!! Thanks a lot guys!!! Awesome!! Both scripts look a lot more efficient!! \$\endgroup\$
    – Louis
    Jul 18 '16 at 19:52
  • 1
    \$\begingroup\$ I guess you can up vote my answer once you gain enough points @Louis \$\endgroup\$
    – Tolani
    Jul 18 '16 at 19:59

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.