3
\$\begingroup\$

I have a Django view and template that work together, but I think I probably did things in a crappy way. I basically have many dynamic values that I want to display on the page, and in two different ways. When the user first comes to the page, these values get returned in the normal context and are stored in javascript variables. When the user clicks a button, an ajax request is fired off, and returns json, which then updates the js variables. I think I could have done it better. Beyond that, I am a Django/python beginner and appreciate any tips that would help me improve.

index.html

{% extends "base.html" %}
{% load i18n %}

{% block head %}
{% load static %}
{% endblock %}


{% block sub-nav %}
{% if user.is_authenticated %}
<a href= '{% url 'animals.views.mycontributions' %}' id='nav_mycontribs' name='nav_mycontribs' class='button'/>{% trans 'My Contributions' %}</a>
<a href= '{% url 'animals.views.index' %}' id='nav_allcontribs' name='nav_allcontribs' class='button'/>{% trans 'All Contributions' %}</a>
{% endif %}
{% endblock %}

{% block content %}

<script src="{% static 'js/jquery.js' %}"> </script>
<script >
var current = 0;
var stats = {

'meals':"{{calculations.meals}}",
'animals_saved': "{{calculations.animals_per_meal}}",
'co2_per_meal': "{{calculations.co2_per_meal}}",
'water_per_meal': "{{calculations.water_per_meal}}",
'grain_per_meal':"{{calculations.grain_per_meal}}"
};
function decrement(){
        if(current == 0)
                current = 4 
    else
        current--;
}

function increment(){ 
    if(current == 4)
        current = 0;
    else
        current++;
}
function changeMainContent(tab, stats){
    text = "<strong>";

    if(tab == 0)
                text += stats.meals
    else if(tab == 1)
                text += stats.animals_saved
    else if(tab == 2)
                text += stats.co2_per_meal 
        else if(tab == 3) 
                text += stats.water_per_meal 
        else if(tab == 4)
                text += stats.grain_per_meal 

    text += "</strong>";
    $('#main-number').html(text);
}

$(document).ready(function() {
    $('#mealClick').click(function() {
                var meals = $('#mealsToday option:selected').text();
                var auto_update = $('#vegetarian_cb').prop('checked');
                $.ajax({method: 'POST', url:'/update_count/' + meals, data:{csrfmiddlewaretoken: "{{ csrf_token }}", 'auto-update': auto_update}}).done(function(data) {
            if(data['type']  === 'error'){
                 $('#error-message').html(data['message'])
            }
            else {
                                stats = {
                                'meals':data['calculations']['meals'],
                                'animals_saved': data['calculations']['animals_per_meal'],
                                'co2_per_meal' :data['calculations']['co2_per_meal'],
                                'water_per_meal':data['calculations']['water_per_meal'],
                                'grain_per_meal':  data['calculations']['grain_per_meal']
                                };
                $('#error-message').html("");
                changeMainContent(current, stats);
            }
                       {% if not user.is_authenticated %} $('#vegetarian_cb').attr('checked', false) {% endif %}
                }).fail(function(a, b, c) { alert(a.responseText) } );
    });

    $('#prev').click(function() {
        decrement();
        changeMainContent(current,stats);
    });
    $('#next').click(function() {
        increment();

        changeMainContent(current, stats);
    });
});

</script>
<div class="content-block">
<div class="block-item"><a id="prev" class="main-link" href="javascript:void(0)"><</a></div>
    <div id="main-number" class="block-item">
            <strong>{{ calculations.meals}}</strong>
    </div>
    <div class="block-item"> 
        <a id="next" class="main-link" href="javascript:void(0)">></a>
    </div>
</div>
<p id="error-message" class="error">
</p>

<div>
    {% trans 'Meals without meat today' %}: 
<select id="mealsToday">
    <option>{% trans '1' %}</option>
    <option>{% trans '2' %}</option>
    <option>{% trans '3' %}</option>
</select>
</div>

<input type='checkbox' id='vegetarian_cb' {% if auto_increment %} checked {% endif %}  name='auto-update'>{% trans 'Automatically add daily meals' %}</input>

<div>
    <input type="submit" id="mealClick" value="{% trans 'Add Meals' %}"></button>
</div>
{% endblock %}

views.py

def build_context_for_contribution_pages(total_meals, user):
    autoincrement = user.extuser.auto_increment_meals if user.is_authenticated() else False

    # Let's get the varying calculations together
    cs = build_calculations_context(total_meals)
    context = {'calculations': cs,'user' : user, 'auto_increment' : autoincrement}
    return context

def build_calculations_context(total_meals):
    cs = CalculationSettings.load()
    calc = {'meals': "{0} {1}".format(total_meals,  _('meals')),
      'animals_per_meal':"{0} {1}".format(round(cs.animals_per_meal * total_meals, 2), _('animals saved')),
      'co2_per_meal':"{0} {1}".format(round(cs.co2_per_meal * total_meals, 2), _('CO2 saved')),
      'grain_per_meal':"{0} {1}".format(round(cs.grain_per_meal * total_meals, 2), _('grain saved')),
      'water_per_meal':"{0} {1}".format(round(cs.water_per_meal * total_meals, 2), _('liters of water saved')),
      }
    return calc


def index(request):
    # get the count from the database
    animal = Totals.objects.all()[0]
    context = build_context_for_contribution_pages(animal.total_meals, request.user)
    return render(request, 'animals/index.html', context)

# Called when a user clicks on the update button

def update_count(request, mealCount):
    mealCount = int(mealCount)
    total_meals = 0
    v = None
    auto_update = request.POST['auto-update'] == 'true'
    # Set their auto update setting, regardless of what happened with their other request
    if request.user.is_authenticated():
        trans_set_user_auto_preference(request.user, auto_update)
    else:
        # They aren't allowed to set auto_update if they are anonymous! Bail out and show a message
        if auto_update:
            return JsonResponse({'type': 'error', 'message': _('Only registered users can use this feature.')})

    # Make sure they can do this
    if 'mealsToday' in request.COOKIES:
        v = int(request.COOKIES['mealsToday'])
        if (v + mealCount > 3) :
            return JsonResponse({'type': 'error', 'message' : _('You have already submitted 3 meals today. Please try again tomorrow.')})

    # Do the work of updating
    trans_update_count(request.user, mealCount)
    # Form the response

    if request.user.is_authenticated():
        total_meals = UserTotals.objects.get(pk=request.user.id).total_meals
    else:
        # TODO DSF - need to pull in the real total here from the not
        # yet created cookie
        total_meals = mealCount
        if(v is not None):
            total_meals += v


    context = build_calculations_context(total_meals)
    response = JsonResponse({'calculations':context})

    if v is not None:
        v += mealCount
    else:
        v = mealCount

    # update the cookie
    set_cookie(response, 'mealsToday', v)

    return response
\$\endgroup\$

1 Answer 1

Reset to default
4
\$\begingroup\$

Naming

In update_count, the variable v is very poorly named. It seems that meals_today would reflect its purpose, and that would be a much better name.

Simplifications

In update_count, you check if v is None a few times. You could simplify the code by initializing its value to 0 instead of None, so:

meals_today = 0

Then, instead of this:

    total_meals = mealCount
    if(v is not None):
        total_meals += v

You can write simpler as:

    total_meals = mealCount + meals_today

And instead of this:

if v is not None:
    v += mealCount
else:
    v = mealCount

You can write simpler as:

meals_today += mealCount

Naming 2

Some variables are named camelCase and others snake_case. The convention in Python is to use snake_case, consistently, everywhere.

It's ok to use a different convention for variables in JavaScript and the cookies, that's up to you. For Python there is a standard (snake_case), and it's good to follow that.

Indenting

The indenting is off at many places in the JavaScript code, for example:

function decrement(){
        if(current == 0)
                current = 4 
    else
        current--;
}

The clean way to write:

function decrement(){
    if (current == 0) {
        current = 4; 
    } else {
        current--;
    }
}

Notice that I also added braces { ... }, a recommended practice, and a statement terminating ; was missing too.

Passing data to the template

You seem concerned about the way you pass data to the template. And then how the context is used to store these values in JavaScript. It doesn't seem so bad to me. I don't really have groundbreaking ideas to improve this part, it seems quite done the way it is now.

The only thing is, I would drop the term "context" from the function name build_calculations_context. Because the dictionary of calculations that this creates is only one part of the context (in Django's terms) that you pass to the template, which can be confusing, misleading readers that this dictionary is the whole context. For the same reason, also avoid assigning this dictionary to variables named context.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks. Any comments about the passing over data to the template? It seems like I constructed that in a strange way, but I don't know how to do it more cleanly. \$\endgroup\$ Commented Jul 9, 2015 at 21:36
  • \$\begingroup\$ I added a new section about that, hope it helps \$\endgroup\$ Commented Jul 9, 2015 at 22:19

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.