Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

@login_required
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
                new_education.save()
                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')]:
            Education.objects.get(id=education_id).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, 
                         'education':education, 
                         '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():
                form.save()
                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" />
    </p>
{% endfor %}
share|improve this question

2 Answers 2

up vote 2 down vote accepted

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:
    whatever

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

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

share|improve this answer

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?

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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