25
\$\begingroup\$

I'm learning Django as I go. I know this model is missing user authentication, registration, comments/comment threading, and voting. But this is my starting code for my model. What are some of the things I can improve on, modify, rewrite, etc?

from django.contrib.auth.models import User
from django.db import models
from django.contrib import admin
from django.template.defaultfilters import escape
from django.utils.translation import ugettext as _
from django.utils.encoding import force_unicode
from django.http import HttpResponse, HttpResponseRedirect
from django.core.urlresolvers import reverse

class DateTime(models.Model):
  datetime = models.DateTimeField(auto_now_add=True)

  def __unicode__(self):
    return unicode(self.datetime.strftime("%b %d, %Y, %I:%M %p"))

class Country(models.Model):
  country = models.CharField(max_length=50)

  def __unicode__(self):
    return unicode(self.country)

class Artist(models.Model):
  artist = models.CharField(max_length=50)
  country = models.ForeignKey(Country, blank=True, null=True)
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  notes = models.TextField()

  def __unicode__(self):
    return artist

class Song(models.Model):
  name = models.CharField(max_length=200)
  artist = models.ForeignKey(Artist, blank=True, null=True)
  # language = models.ForeignKey(Country, blank=True, null=True)
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  notes = models.TextField()

  def __unicode__(self):
    return song

class FileType(models.Model):
  file_type = models.CharField(max_length=3)
  description = models.TextField()
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  notes = models.TextField()

  def __unicode__(self):
    return file_type

class Level(models.Model):
  level = models.CharField(max_length=3)
  description = models.TextField()
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  notes = models.TextField()

  def __unicode__(self):
    return level

class MusicSheet(models.Model):
  version = models.CharField(max_length=2)
  song = models.ForeignKey(Song, blank=True, null=True)
  artist = models.ForeignKey(Artist, blank=True, null=True)
  file_type = models.ForeignKey(FileType, blank=True, null=True)
  level = models.ForeignKey(Level, blank=True, null=True)
  user = models.ForeignKey(User, blank=True, null=True)
  created = models.ForeignKey(DateTime)
  text = models.TextField()
  notes = models.TextField()

#include votes

#include comments

#include registration code


########################################################################################################################
######################################## ADMIN STUFF ###################################################################
########################################################################################################################


class MusicSheetAdmin(admin.ModelAdmin):
  list_display = ["version", "song", "artist", "file_type", "level", "user" , "created", "text", "notes"]
  search_fields = ["version"]

class MusicSheetInline(admin.TabularInline):
  model = MusicSheet

class DateAdmin(admin.ModelAdmin):
  list_display = ["datetime"]
  inlines = [MusicSheetInline]

  def response_add(self, request, obj, post_url_continue='../%s/'):
    """ Determines the HttpResponse for the add_view stage.  """
    opts = obj._meta
    pk_value = obj._get_pk_val()

    msg = "Song(s) were added successfully."
    # Here, we distinguish between different save types by checking for
    # the presence of keys in request.POST.
    if request.POST.has_key("_continue"):
      self.message_user(request, msg + ' ' + _("You may edit it again below."))
      if request.POST.has_key("_popup"):
        post_url_continue += "?_popup=1"
      return HttpResponseRedirect(post_url_continue % pk_value)

    if request.POST.has_key("_popup"):
      return HttpResponse(
          '<script type="text/javascript">opener.dismissAddAnotherPopup(window, "%s", "%s");'
          '</script>' % (escape(pk_value), escape(obj)))
    elif request.POST.has_key("_addanother"):
      self.message_user(request, msg + ' ' + (_("You may add another %s below.") %
                                              force_unicode(opts.verbose_name)))
      return HttpResponseRedirect(request.path)
    else:
      self.message_user(request, msg)

    for music_sheet in MusicSheet.objects.filter(created=obj):
      if not music_sheet.user:
        music_sheet.user = request.user
        music_sheet.save()

    return HttpResponseRedirect(reverse("admin:musicsheet_musicsheet_changelist"))


class CountryAdmin(admin.ModelAdmin):
  list_display = ["country"]
  search_fields = ["country"]

class CountryInline(admin.TabularInline):
  model = MusicSheet



class SongAdmin(admin.ModelAdmin):
  list_display = ["name", "artist", "user", "created", "notes"]
  search_fields = ["name", "artist"]

class SongInline(admin.TabularInline):
  model = MusicSheet



class ArtistAdmin(admin.ModelAdmin):
  list_display = ["artist", "country", "user", "created", "notes"]
  search_fields = ["artist", "country"]

class ArtistInline(admin.TabularInline):
  model = MusicSheet



class FileTypeAdmin(admin.ModelAdmin):
  list_display = ["file_type", "description", "user", "created", "notes"]
  search_fields = ["file_type"]

class FileTypeInline(admin.StackedInline):
  model = MusicSheet


class LevelAdmin(admin.ModelAdmin):
  list_display = ["level", "description", "user", "created", "notes"]
  search_fields = ["level"]

class LevelInline(admin.StackedInline):
  model = MusicSheet

admin.site.register(MusicSheet, MusicSheetAdmin)
admin.site.register(Country, CountryAdmin)
admin.site.register(Song, SongAdmin)
admin.site.register(Artist, ArtistAdmin)
admin.site.register(FileType, FileTypeAdmin)
admin.site.register(Level, LevelAdmin)
admin.site.register(DateTime, DateAdmin)
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I suggest you highlight places you want feedback on. Other than that, sorting imports and (maybe) field names will make your life easier on the long run. \$\endgroup\$
    – TryPyPy
    Jan 24 '11 at 23:16
  • \$\begingroup\$ @TryPyPY - Mostly around the model itself, structure of the classes, how can I make it more efficient? increasingly better? How can I take this code from newb to pro \$\endgroup\$
    – dassouki
    Jan 25 '11 at 12:23
18
\$\begingroup\$

Disclaimer: I'm not a Django guru. Here are my thoughts, for what it's worth.

  • Common fields: user, created, and nodes occur in almost every class. I'd consider creating an abstract base class with these three fields.

  • Field names: for the sake of readability, I'd change Artist.artist to Artist.name, and *.user to *.creator.

  • File extensions: occasionally have more than 3 chars (e.g.: .jpeg, .java).

  • Indexing: I'd add db_index=True to the name fields, for the very least.

  • Redundancy: since you have MusicSheet.song and Song.artist, you no longer need MusicSheet.artist. It's called redundancy and it's generally a bad thing.

  • Primary keys, unique constraints: you probably don't want two FileTypes with the same file_type, so that field is a good candidate for primary key (primary_key=True). I'm not sure what Level is supposed to model, but my guess is that its level is a good candidate for primary key as well. Add unique=True for Country.country, or add a primary key with country code.

  • DateTime: I don't see a need for this class. Simply replace every occurrence of: created = models.ForeignKey(DateTime) with models.DateTimeField(auto_now_add=True).

\$\endgroup\$
6
\$\begingroup\$

A few things come to mind:

  • Lose the DateTime model and instead add a DateTimeField directly to the models that need it. The ForeignKey buys you nothing except incuring an additional database join for even the simplest of operations.
  • You probably did this only so you could paste this code in one piece, but if not: You should put your admin code in a file called admin.py inside your application. That way you can use the admin autodiscover feature
  • In DateAdmin you overwrite the response_add method to (as far as I can see) set the user/creator field. This can be acomplished much easier by overwriting the save_model method.
\$\endgroup\$
1
\$\begingroup\$
  • Instead of the (very long) list of Admin classes at the end, you can integrate them in the model, so:

    class MusicSheet(models.Model):
        version = models.CharField(max_length=2)
        song = models.ForeignKey(Song, blank=True, null=True)
        artist = models.ForeignKey(Artist, blank=True, null=True)
        file_type = models.ForeignKey(FileType, blank=True, null=True)
        level = models.ForeignKey(Level, blank=True, null=True)
        user = models.ForeignKey(User, blank=True, null=True)
        created = models.ForeignKey(DateTime)
        text = models.TextField()
        notes = models.TextField()
    
    class MusicSheetAdmin(admin.ModelAdmin):
        list_display = ["version", "song", "artist", "file_type", "level", "user" , "created", "text", "notes"]
        search_fields = ["version"]
    
    class MusicSheetInline(admin.TabularInline):
        model = MusicSheet
    
    admin.site.register(MusicSheet, MusicSheetAdmin)
    

    can be simplified (and neatened) to:

    class MusicSheet(models.Model):
        version = models.CharField(max_length=2)
        song = models.ForeignKey(Song, blank=True, null=True)
        artist = models.ForeignKey(Artist, blank=True, null=True)
        file_type = models.ForeignKey(FileType, blank=True, null=True)
        level = models.ForeignKey(Level, blank=True, null=True)
        user = models.ForeignKey(User, blank=True, null=True)
        created = models.ForeignKey(DateTime)
        text = models.TextField()
        notes = models.TextField()
    
        class Admin:
            list_display = ["version", "song", "artist", "file_type", "level", "user" , "created", "text", "notes"]
            search_fields = ["version"]
    

    Make sure that class Admin is inside the main class.

    Alternatively, if you did want them separate, the admin classes and registrations belong inside a separate file, admin.py. (You will have to import the models from myapp.models import *)

  • Some of your ForeignKeys have both blank=True and null=True. This is generally not a good idea. Do you want to allow them to be null (no value) or blank (an empty value)? Looking at your code, I'd say you want null=True but I can't be sure. Try one, then the other, and see which one does what you want. I highly doubt you need both. As a general rule, I only use blank=True for CharFields, so you can have a null string instead of an empty database field.

  • I can't work out what DateAdmin is meant to do. You probably won't need it when you switch to DateTimeField as recommended above, but I would want to know what its intended purpose is before I make a call.

\$\endgroup\$

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.