2
\$\begingroup\$

I often have method like this one with two or more render to do due to catching the error for example. I currently do something like this:

def update
  @user = current_user
  if params[:user][:camera_type].present? && params[:user][:lens_type].present? && params[:user][:photo].present?
    r = Photo.instance.Upload(camera_type: @user.id,
                              album: @user.default_album,
                              photo: params[:user][:photo],
                              lens_type: params[:user][:lens_type],
                              camera_type: params[:user][:camera_type])
    @user.camera_type = params[:user][:camera_type]
    @user.lens_type = params[:user][:lens_type]
    @user.photo = params[:user][:photo]
    @user.save!
    render action: 'edit'
    return
  end
rescue Photo => e
  redirect_to photo_path, :flash => { :alert => t(:text_photo_edit_failed) }
end

But may be there is a better way to do it ?

\$\endgroup\$
  • \$\begingroup\$ can you fix the indentation? \$\endgroup\$ – tokland Mar 15 '13 at 22:27
  • 1
    \$\begingroup\$ @tokland Fixed it - was thinking about doing that earlier, but wasn't even sure I knew how. A lot going on there, none of it clear :/ \$\endgroup\$ – Flambino Mar 15 '13 at 23:37
4
\$\begingroup\$

Some notes:

  • Keep your controllers lean, this is too much code for an action.
  • The photo upload should be done as a part of the object's save chain (after validation), so create a before_save callback in the User model that returns false if the upload fails.
  • You set r (cryptic name) but don't use it anywhere? does the upload raise an exception on error?

At the end the controller should look extremely simple, something like:

def update
  @user = current_user

  if @user.update_attributes(params[:user])
    render action: 'edit'
  else
    redirect_to photo_path, flash: {alert: t(:text_photo_edit_failed)}
  end
end

Any additional logic should be in User. Models are easy to test in isolation, controllers on the other hand have a lot of state to worry about.

\$\endgroup\$
  • 1
    \$\begingroup\$ Agreed, most of this should be moved into models. There's also some weirdly suspect bits of code here suggesting you need to work out some kinks in your model layer, like Upload as a method name, or camera_type: @user.id. What is a user's ID doing there, being called a "camera_type"? You also generally shouldn't have to check for specific parameters to be present, rather you prevent models from being moved into an invalid state at the model level, via validations. \$\endgroup\$ – meagar Mar 15 '13 at 19:24
  • \$\begingroup\$ @meagar camera_type: @user.id is the typo mistake. my bad. \$\endgroup\$ – bl0b Mar 15 '13 at 23:47

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.