Problem
The code is the view part of my bookmarker – project for managing bookmarks by categories. It uses Python 3.4/Django 1.6. I have also included models.py for reference.
The code does the following
- Providing relevant autocomplete JSON data
- Providing details of categories/bookmarks (both are DB models of my project)
- Open website stored in bookmark
I am mainly concerned with the quality of the abstractions. Other reviews are also welcome but I am mainly concerned with the structure of the code.
views.py
from django.http import HttpResponse
from django.shortcuts import render
from BookMarker.models import BookMark, Category
import json
import webbrowser
###########################
## Auxiliary ##
###########################
def _get_objects_by_params(request, param_name, object_class):
ids_string = request.GET.get(param_name, '')
if ids_string == '':
return []
ids = ids_string.split(',')
return [object_class.objects.get(pk=_id) for _id in ids]
class Autocomplete():
@staticmethod
def _get_json(cur_objects, func):
results = []
for cur_object in cur_objects:
results.append(func(cur_object))
return json.dumps(results)
@staticmethod
def autocomplete(request, class_name, attr_name):
term = request.GET.get('term', '')
objects = class_name.objects.filter(**{
attr_name + '__icontains': term
})
exclude_value = request.GET.get("excludes", "")
if exclude_value != "":
objects = objects.exclude(id__in=[int(i) for i in exclude_value.split(",")])
data = Autocomplete._get_json(
objects,
lambda x: getattr(x, attr_name)
)
return HttpResponse(data, 'application/json')
class GetBookMarks:
@staticmethod
def get_bookmarks_by_categories(categories):
bookmarks = set()
for category in categories:
bookmarks.update(category.bookmark_set.all())
return bookmarks
@staticmethod
def get_bookmark_by_name(bookmark_name):
return BookMark.objects.filter(name=bookmark_name)[0]
class GetCategories:
@staticmethod
def get_categories_by_bookmark(bookmark):
return bookmark.category.all()
###########################
## Services ##
###########################
def category_autocomplete(request):
return Autocomplete.autocomplete(request, Category, 'name')
def bookmark_autocomplete(request):
return Autocomplete.autocomplete(request, BookMark, 'name')
def website_open(request):
cur_url = _get_objects_by_params(request, 'id', BookMark)[0].url_content
webbrowser.open(cur_url)
return HttpResponse('success', 'text/html')
def get_bookmarks(request):
categories = _get_objects_by_params(request, 'value', Category)
bookmarks = GetBookMarks.get_bookmarks_by_categories(categories)
return render(request, 'BookMarker/partials/bookmarks.html', {
'bookmarks': bookmarks,
})
#TODO Need to check the name
def get_bookmark_by_name(request):
bookmark = GetBookMarks.get_bookmark_by_name(request.GET.get("name", ""))
categories = GetCategories.get_categories_by_bookmark(bookmark)
result = [category.name for category in categories]
return HttpResponse(json.dumps(result), 'application/json')
def get_category(request):
category = Category.objects.get(name=request.GET.get('value', ''))
return render(request, 'BookMarker/partials/category.html', {
'category': category,
})
models.py
from django.db import models
class Category(models.Model):
name = models.CharField(max_length=50)
def __str__(self):
return self.name
class BookMark(models.Model):
name = models.CharField(max_length=50)
url_content = models.CharField(max_length=500)
category = models.ManyToManyField(Category)
def __str__(self):
return self.name
Solution
Overall, this is pretty nicely written.
Abstractions and code structure
I am mainly concerned with the quality of the abstractions. Other reviews are also welcome but I am mainly concerned with the structure of the code.
The models are nice and simple, no comments there.
The classes in the views file are not great.
Since they only have static methods,
they are essentially utility classes.
As such, the methods don’t really need to be in a class at all,
they could be simply top-level functions.
For example, these kind of calls are a bit awkward:
return Autocomplete.autocomplete(request, Category, 'name')
Normally a class should help you avoid repeating terms.
For example a Person class would have a get_name method instead of get_person_name. Since Person is already implied by the class,
methods in it don’t need to repeat the term “person”.
The presence of calls like Autocomplete.autocomplete is a symptom of a problem with the class structure.
Possibly you created a class for Autocomplete to hide the private method _get_json.
That’s not a bad idea.
However, since that method is only called from autocomplete itself,
it could just as well be inside autocomplete itself.
Also note that another way to group methods is to put them in a separate file.
Overall, I think classes with only static methods are not great.
They would be better converted to regular functions at the top level,
or in their dedicated file.
The same goes for GetBookmarks and GetCategories too.
And this is especially true for classes that only contain one public method.
Going back to your main question:
such classes don’t represent useful abstractions,
they are simple groupings of related methods.
Utility classes are useful,
but if you only have a single method in them,
then they are a bit of an overkill and might not help all that much.
Coding practices
A couple of things are not very Pythonic or can be improved.
Instead of this:
if ids_string == '':
return []
It’s more Pythonic to write this way:
if not ids_string:
return []
Instead of this:
def _get_json(cur_objects, func):
results = []
for cur_object in cur_objects:
results.append(func(cur_object))
return json.dumps(results)
The common approach is to use a list comprehension, which makes it more compact and Pythonic:
def _get_json(cur_objects, func):
return json.dumps([func(cur_object) for cur_object in cur_objects])
Similar to earlier:
if exclude_value != "":
# ...
The Pythonic way would be:
if exclude_value:
# ...
This is more complicated than it needs to be:
data = Autocomplete._get_json(
objects,
lambda x: getattr(x, attr_name)
)
Since the _get_json function will simply call the lambda on the elements,
it would be more natural to use a simple list comprehension here,
and pass a list to _get_json without needing a lambda at all:
data = Autocomplete._get_json([getattr(x, attr_name) for x in objects])
And since we know just how simple is _get_json,
we could simply bypass it entirely:
data = json.dumps([getattr(x, attr_name) for x in objects])