Django view and template for a virtual zoo

Question

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

Answer 1

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.

• skaz: 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.
• janos: I added a new section about that, hope it helps