1
\$\begingroup\$

my shoppingcart is working but i used to many var and functions and i think it looks sloppy. i dont know how to set it right.

here is the Jsfiddle link:

https://jsfiddle.net/z0c1ja37/

can someone help me to put things right in this code?

look at my jsfiddle link there is the full code

$('.productprijzen').hide();
var $select = $('<select>').addClass('keuzes');
$select.append('<option>Kies je optie</option>');
$select.append('<option value="volvo">Volvo</option>');
$select.append('<option value="saab">saab</option>');
$select.append('<option value="peugeot">peugeot</option>');
$select.append('<option value="mazda">mazda</option>');

$select.on('change', function () {
    $parent = $(this).closest(".row");
    $('.productprijzen', $parent).slideDown();

    // prijs en btw laten zien
    var merk = $(this).val();
    var productInfo = oProducten[merk];
    $parent.find('.prijs').html("€" + productInfo.prijs);
    $parent.find('.btw').html(productInfo.btw + "%");

    // berekening van aantal x prijs
    var $aantalKeuze = $parent.find('input.aantal');
    var $subtotaal = $parent.find('.sub');

    $aantalKeuze.bind('keyup mouseup', function () {
        var $subtotaalUiteindelijk = productInfo.prijs * $aantalKeuze.val();
        $subtotaal.html($subtotaalUiteindelijk);

        var totaalBtw6 = 0;
        var totaalBtw21 = 0;
        var $totaal = 0;
        var $subtotaalVeld = $(".sub");
        $subtotaalVeld.each(function () {
            if ($(this).text() !== "") {
                $totaal = $totaal + Number($(this).text());
            }
        });
\$\endgroup\$
1
  • 1
    \$\begingroup\$ This code appears to be incomplete - there is no }); to end the block started with $select.on('change', function () {. This may also be why totaalBtw6 and totaalBtw21 are never used. \$\endgroup\$ – IMSoP Oct 13 '17 at 14:15
1
\$\begingroup\$

Use named functions

It's very common to write event handlers inline as anonymous functions, but as code gets more complex, this leads to deeply nested code that can be hard to follow. The inner event in particular could be something like $aantalKeuze.bind('keyup mouseup', handleAantalKeuzeChange); (apologies for the mix of languages).

Register events only once

At the moment, you seem to be registering the keyup mouseup handler every time the user changes the parent select. If the HTML it's checking isn't being replaced, this is going to end up with duplicate handlers. If the HTML is dynamically created (which doesn't show in this code) you could use a .on() "delegated event" to bind it in advance.

Separate UI logic from calculations

Try to separate your workflow into:

  1. Get data from HTML
  2. Calculate values (totals etc)
  3. Populate new HTML

This might mean more (named!) functions, or it might just mean moving lines around and more variables. The aim is that you should be able to examine and change the "business logic" in step 2 separately from the "display logic" in steps 1 and 3.

Be consistent

You mostly use the convention that variables prefixed with $ are jQuery DOM objects and others are values, but you have var $totaal = 0; instead of var totaal = 0;. This is closely related to the previous point - the business logic part of the code should have no $ variables in it.

You mostly scope your variables with var, but have it missing on $parent. Because of how JS "hoists" variables, it may be sensible to declare all your variables close to the top of the function as possible. Again, this may encourage you to split out some sections into their own function - if a set of variables is only used within a tight section of code, that code might be asking to become a separate function, with defined inputs and outputs.

You mostly use $parent.find(...), except in $('.productprijzen', $parent). Standardise on one or the other, unless I'm mistaken and there's a significant difference.

\$\endgroup\$
-1
\$\begingroup\$

My (Disclaimer: opinion-based from a German point of view) main issue here, are the non-english identifier. Of course I can guess some, but a minimized version would be as readable and unsuitable for an detailed review as this code. Always code in english, you never know who will need to read your code.

In addition to that, it is actually up to you, if you prefer a clarifying variable, if it is only used once in the next line, or not. There is no global valid recommendation.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ I don't really buy this criticism. Which language makes sense depends entirely on the context where this is being developed - if it's going to be open source, or contributed to some international concern, then using English as a lingua franca might make sense. But in terms of code style, identifiers in your own language are going to be easier to read by you and others with the same language as you compared to identifiers in some second language which happens to be popular in the world of computing. \$\endgroup\$ – IMSoP Oct 13 '17 at 14:11
  • \$\begingroup\$ But the context for this snippet is an english speaking review board? Even your " (apologies for the mix of languages)" in your own answer, indicates that there is something wrong here. \$\endgroup\$ – mheinzerling Oct 13 '17 at 14:35
  • 1
    \$\begingroup\$ No, the context is wherever this code was written for. The problem in my answer is not that the OP didn't use English, but that I didn't use Dutch. At best, this should be a comment under the question, saying that you find the code hard to review because you don't speak the OP's language, although I think the structural problems are pretty obvious. \$\endgroup\$ – IMSoP Oct 13 '17 at 14:40
  • \$\begingroup\$ Just saw, that you are most likely from UK. Maybe we just have a different view on this topic. In Germany I would not hire anybody that is showing me German sample code. If you don't begin with writing English code, you will pretty soon hit a wall here, and change your behaviour. In addition to that, most languages have/had issues with special characters and encoding (äöüß). \$\endgroup\$ – mheinzerling Oct 13 '17 at 14:51
  • 1
    \$\begingroup\$ When I saw this question I skipped it due to the non-english variables. Didn't feel like wading through them. I also checked meta to see if there was already a guideline about non-english code, and there is. In short, it isn't an outright deal breaker: codereview.meta.stackexchange.com/q/3795/140581 \$\endgroup\$ – Conor Mancone Oct 13 '17 at 17:14

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.