2
\$\begingroup\$

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 %}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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?

\$\endgroup\$
2
\$\begingroup\$

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.

\$\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.