I have a view function that allows a user to Add / Edit / Delete an object. How does the following function look, and in what ways could I improve it? I was thinking about separating aspects of the function into different parts, but since no 'component' is more than four-five lines of code, I thought that would be a bit overkill.

def edit_education(request, edit=0):
    profile = request.user.get_profile()
    education = profile.education_set.order_by('-class_year')
    form = EducationForm(data=request.POST or None, request=request)

    if request.method == 'POST':

        ##### to add a new school entry ######
        if 'add_school' in request.POST:
            if form.is_valid():
                new_education = form.save(commit=False)
                new_education.user = profile
                return redirect('edit_education')

        ##### to delete a school entry #####
        for education_id in [key[7:] for key, value in request.POST.iteritems() if key.startswith('delete')]:
            return redirect('edit_education')

        ###### to edit a school entry -- essentially, re-renders the page passing an "edit" variable #####
        for education_id in [key[5:] for key, value in request.POST.iteritems() if key.startswith('edit')]:
            edit = 1
            school_object = Education.objects.get(id = education_id)
            form = EducationForm(instance = school_object, request=request)
            return render(request, 'userprofile/edit_education.html', 
                        {'form': form, 
                         'edit': 1, 
                         'education_id': education_id}
        ##### to save changes after you edit a previously-added school entry ######
        if 'save_changes' in request.POST:
            instance = Education.objects.get(id=request.POST['education_id']) 
            form = EducationForm(data = request.POST, instance=instance, request=request, edit=1)
            if form.is_valid():
                return redirect('edit_education')

return render(request, 'userprofile/edit_education.html', {'form': form, 'education': education})

And in my template, if this help to clarify anything:

{% for education in education %}
    <p><b>{{ education.school }}</b> {% if education.class_year %}{{ education.class_year|shorten_year}}, {% endif %} {{ education.degree}}
        <input type="submit" name="edit_{{education.id}}" value='Edit' />
        <input type="submit" name="delete_{{education.id}}" value="Delete" />
{% endfor %}

3 Answers 3


This procedure is too long. I'll never believe you gonna stop with this logic. Sooner or later you'll decide adding anything else here and then this piece of code will become even longer. You have 4 absolutely great blocks there, so why don't you just separate them into 4 separate procedures, leaving only the higher-level logic in original edit_education?


The if conditions can be simplified.

fpv_list = [ item.partition("_") for item in request.POST.iteritems() ]
deletes = [ v for f,p,v in fpv_list if f == 'delete' ]
for education_id in deletes:

edits = [ v for f,p,v in fpv_list if f == 'edit' ]  
for education_id in edits:

This removes the dependence on knowing the length of 'delete' and the length of 'edit' prefixes on the buttons.


Separate view functions according to functionality:

def addSome(request):
   # Your Add logic here

def editSome(request):
   # Your Edit related logic .. 

def deleteSome(request):
   # Your Edit related logic ..

Now you can easly manage future updates in functionality(no more if/else) and think about url-name separations also.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.