current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite
1

I'm writing a checkout form for purchasing online tests. You can buy a certification or a recertification, as well as a pdf or book study manual.

I'm using Jinja2 templates to provide test data into the Javascript. The test object is a dictionary with the following structure. All prices are in cents.

test {
    id: Integer,
    name: String,
    acronym: String,
    certification_price: Integer,
    recertification_price: Integer,
    study_pdf_price: Integer,
    study_book_price: Integer,
}

If a user buys more than one test, they get a discount of 10% off for each additional test.

<!doctype html>
<html>
<head>
<script>
$(document).ready(function() {
    var calcTotal = function(){
        var total = 0;
        var numTests = 0;
        {% for test in tests %}
            {% if test.certification_price %}
            if ($('#buy-{{ test.acronym }}-certification').is(':checked')) {
                total += {{ test.certification_price }};
                numTests++;
            }
            {% endif %}
            {% if test.recertification_price %}
            if ($('#buy-{{ test.acronym }}-recertification').is(':checked')) {
                total += {{ test.recertification_price }};
                numTests++;
            }
            {% endif %}
            {% if test.study_pdf_price %}
            if ($('#buy-{{ test.acronym }}-pdf').is(':checked')) {
                total += {{ test.study_pdf_price }};
            }
            {% endif %}
            {% if test.study_book_price %}
            if ($('#buy-{{ test.acronym }}-book').is(':checked')) {
                total += {{ test.study_book_price }};
            }
            {% endif %}
        {% endfor %}

        // take 10 percent off for each additional test after the first
        if (numTests > 0) numTests--;
        var discountPrice = Math.round(((10 - numTests) * 0.1) * total);

        $('.product-total').html(Math.round(discountPrice));
        $('#final-total').val(discountPrice);
    }
    $('.product-selection').click(calcTotal);
    calcTotal();
});
</script>
<body>
<h2>Selected Products</h2>
<form id="registration-form" action="" method="POST">
    <table id="products-table">
        <tr>
            <th>Product</th>
            <th>Price</th>
            <th>Add to Cart</th>
        </tr>
        {% for test in tests %}
            {% if test.certification_price %}
            <tr>
                <td>{{ test.acronym }} Certification{% if test.study_pdf_price %} (PDF Manual Included){% endif %}</td>
                <td>${{ test.certification_price }}</td>
                <td><input class="product-selection" type="checkbox" id="buy-{{ test.acronym }}-certification" name="buy-{{ test.acronym }}-certification" /></td>
            </tr>
            {% endif %}
            {% if test.recertification_price %}
            <tr>
                <td>{{ test.acronym }} Recertification</td>
                <td>${{ test.recertification_price }}</td>
                <td><input class="product-selection" type="checkbox" id="buy-{{ test.acronym }}-recertification" name="buy-{{ test.acronym }}-recertification" /></td>
            </tr>
            {% endif %}
            {% if test.study_pdf_price %}
            <tr>
                <td>{{ test.acronym }} {{ test.study_pdf_description }}</td>
                <td>${{ test.study_pdf_price }}</td>
                <td><input class="product-selection" type="checkbox" id="buy-{{ test.acronym }}-pdf" name="buy-{{ test.acronym }}-pdf" /></td>
            </tr>
            {% endif %}
            {% if test.study_book_price %}
            <tr>
                <td>{{ test.acronym }} {{ test.study_book_description }}</td>
                <td>${{ test.study_book_price }}</td>
                <td><input class="product-selection" type="checkbox" id="buy-{{ test.acronym }}-book" name="buy-{{ test.acronym }}-book" /></td>
            </tr>
            {% endif %}
            <tr><td>&nbsp;</td><td>&nbsp;</td><td>&nbsp;</td></tr>
        {% endfor %}
            <tr><td><strong>Total</strong></td><td><strong>$<span class="product-total">0</span></strong></td></tr>
    </table>
    <input type="hidden" id="final-total" name="final-total" />
    <p><input id="submit-button" type="submit" value="Place Order"></p>
</form>
</body>
</html>

I feel like I am repeating myself a lot, but I don't know how I would do this differently. Also, this way of providing data by writing Javascript code with the templating language seems messy. Any ideas?

share|improve this question

1 Answer 1

up vote 2 down vote accepted

You're right, templating JavaScript code in the way you're doing is messy and usually a bad option. If possible it's almost always better to pass in a single JSON object with all of the values you need and then operate on that object in JavaScript. I don't know Python but something like this:

var tests    = {{ json.dumps( tests ) }}, // this is the only template insertion
    total    = 0,
    numTests = 0,
    test,
    testIdPfx
    // only one `var` is necessary for multiple declarations
;

// do this loop in JavaScript instead of the templating system
for ( var i; i < tests.length; i++ ) {
  test = tests[ i ]

  // do this concatenation just once at the beginning
  testIdPfx = '#buy-' + test.acronym + '-';

  // this condition also in JavaScript instead of the template--combined with the
  // `if` you're already doing
  if ( test.certification_price &&
       $( testIdPfx + 'certification' ).is( ':checked' )
     ) {
    total += test.certification_price;
    numTests++;
  }

  if ( test.recertification_price &&
       $( testIdPfx + 'recertification' ).is( ':checked' )
     ) {
    total += test.recertification_price;
    numTests++;
  }

  // and so on...
}

// ...

(You could also load the JSON object with an Ajax request and avoid templating in your JavaScript entirely, but that might be overkill.)

Already that looks a lot cleaner but you still have a lot of repetition.

Basically the only thing different between your four if blocks is one word: certification, recertification, study_pdf and study_book. So why not put those four words in an array and then reuse the same code four times?

var tests = {{ json.dumps( tests ) }}, // this is the only template insertion
    // here's your four words:
    types = [ 'certification', 'recertification', 'study_pdf', 'study_book' ]
    total    = 0,
    numTests = 0,
    test, testIdPfx,
    type,
    price
;

for ( var testIdx; testIdx < tests.length; testIdx++ ) {
  test = tests[ testIdx ]
  testIdPfx = '#buy-' + test.acronym + '-';

  // repeat for each of the four words in `types`
  for ( var typeIdx; typeIdx < types.length; typeIdx++ ) {
    type  = types[ typeIdx ];
    // exploit the fact that `test.foo` and `test['foo']` are equivalent in JavaScript
    price = test[ type + '_price' ];

    if ( price && $( testIdPfx + type ).is( ':checked' ) ) {
      total += price;
      numTests++;
    }
  }
}

// ...

(Note: You'll have to adjust your markup to have #buy-{{ test.acronym }}-study_pdf insteady of just -pdf and #buy-{{ test.acronym }}-study_book instead of -book to match the types values, or vice versa.)

A nice side-effect of doing the main for loop in JavaScript instead of the templating system is that you're sending proportionally less JavaScript code to the client, which saves on bandwidth.

If you wanted you could perform a similar reduction in your markup.

Hope that helps!

share|improve this answer

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.