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 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?

share|improve this question

1 Answer 1

up vote 2 down vote accepted

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

share|improve this answer
    
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. –  Ecnalyr Aug 1 '14 at 0:27
    
@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. –  Flambino Aug 1 '14 at 0:54

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.