2
\$\begingroup\$

I smell something wrong here, but am not quite sure what the most idiomatic way to solve the problem.

I have a controller for (job)listings and it has a method called apply. This method is used when someone has a resume and is clicking to apply for the job associated with the listing.

def apply
  set_resume
  if @resume.apply_to(@listing)
    redirect_to @listing
  else
    redirect_to @listing, alert: 'Unable to apply for job'
  end
end

This calls a method on the resume object:

def apply_to(listing)
  jobapp = Jobapp.new(resume: self, listing: listing)
  jobapp.save
end

Everything above works just fine - it just seems inelegant (and I don't get the errors associated with the jobapp object, although that's not vitally important).

If I use the following method in place of the above method:

def apply_to(listing)
  Jobapp.create(resume: self, listing: listing)
end

The conditional if @resume.apply_to(@listing) always returns true, even if the Jobapp failed to be created.

What is a cleaner way of achieving this task?

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

Given the current setup, the simplest thing would probably be to raise an exception using #save!:

def apply_to(listing)
  jobapp = Jobapp.new(resume: self, listing: listing)
  jobapp.save! # note the bang (!)
end

And in the controller

def apply
  set_resume
  @resume.apply_to(@listing)
  redirect_to @listing
rescue ActiveRecordError
  redirect_to @listing, alert: 'Unable to apply for job'
end

It may be worth the effort to rename your method to apply_to! (also with a bang) to make it clear that it can raise an exception.


As for why create doesn't work, it's not that it returns true - it returns the new record (and all objects except nil and false are considered "true"). To check if create actually managed to persist that record in the database, you can check record.persisted?. If true, then it was saved just fine.

So using your create version of the method, you could do

def apply
  set_resume
  jobapp = @resume.apply_to(@listing)
  if jobapp.persisted?
    redirect_to @listing
  else
    redirect_to @listing, alert: 'Unable to apply for job'
  end
end

This would also let you access jobapp.errors, if you want.


A few other things caught my eye:

  1. set_resume should probably be in a before-filter. Or be a method that just returns a record, leaving it up to the caller to stick it in an instance variable (or not)

  2. I imagine a jobapp belongs to a listing (and/or a resume). If so, it'd be cleaner to build it like so:

    jobapp = owner.jobapps.new(other: foo)
    

Given that it's this simple, though, it might be something you could just do in the controller.

It's never quite clear where a join model like jobapp (which I assume is basically a join model) should be created. Should it be in the Resume model or in the Listing model? Either one makes sense. So the Solomonic solution is do neither (at least for now), and handle it in the controller:

def apply
  jobapp = @listing.jobapps.new(resume: @resume)
  if jobapp.save
    redirect_to @listing
  else
    redirect_to @listing, alert: 'Unable to apply for job'
  end
end

Lastly, you might want to tweak that error message. It seems a little "blunt" to me. Though it's the server that failed, it sounds as if the user is unable to apply for the job. "Failed to save job application" might be more appropriate

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Your assumptions were correct and you were definitely a step ahead of me with the idea of, ". . . Either one makes sense. So the Solomonic solution is do neither (at least for now), and handle it in the controller. . ." I like this. \$\endgroup\$
    – Ecnalyr
    Aug 1 '14 at 0:27
  • \$\begingroup\$ @Ecnalyr Yeah, it's the sort of solution I keep forgetting to use myself, because you always feel it belongs in a model - but it works, and really makes a lot of sense to have it in the controller. And if, some day, you need more logic, you can start by extracting it into a method or a service object. \$\endgroup\$
    – Flambino
    Aug 1 '14 at 0:54

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.