current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 6 down vote favorite

Using Rails 3.2, I have the following:

g = Geocoder.search(address)

# if no address found, geocode with latitude and longitude
if g.empty?
  g = Geocoder.search(latitude, longitude)
  if g.empty?
    log.info "Geocode failed for #{latitude}, #{longitude}"
  end
else
  ---save---
end

If the first geocoding with address fails, it will try latitude and longitude. If still fails, then it logs error, else it will save.

Is this a good way to write this code?

share|improve this question
add comment

2 Answers

up vote 6 down vote accepted

Some notes:

  • g: use meaningful names.
  • Don't mix code with parentheses and without.
  • Check the pattern Object#presence.
  • # if no address found, ...: declarative code makes this kind of comments unnecessary.

I'd write:

results = Geocoder.search(address).presence || Geocoder.search(latitude, longitude)
if results.present?
  # save
else
  log.info("Geocode failed for addr='#{search}' and coords=#{latitude}/#{longitude}")
end
share|improve this answer
add comment
up vote 4 down vote

Nesting your ifs is unnecessary, and flatting them will look better:

g = Geocoder.search(address)

# if no address found, geocode with latitude and longitude
if g.empty?
  g = Geocoder.search(latitude, longitude)
end

if g.empty?
    log.info "Geocode failed for #{latitude}, #{longitude}"
  end
else
  ---save---
end

You can even go one-liners to make the code look more succinct:

g = Geocoder.search(address)

# if no address found, geocode with latitude and longitude
g = Geocoder.search(latitude, longitude) if g.empty?

log.info "Geocode failed for #{latitude}, #{longitude}" && return if g.empty?

---save---
share|improve this answer
    
Ha! Your first answer was what I had before refactoring! –  Victor Feb 8 at 18:33
    
Nested ifs are considered a smell, just imagine how your code look if had two more fallbacks - each fallback will nest another if, and it would be hard to read the flow of the code - does it save? does it log? –  Uri Agassi Feb 8 at 19:10
add comment

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.