6
\$\begingroup\$

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?

\$\endgroup\$
6
\$\begingroup\$

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

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---
\$\endgroup\$
  • \$\begingroup\$ Ha! Your first answer was what I had before refactoring! \$\endgroup\$ – Victor Feb 8 '14 at 18:33
  • \$\begingroup\$ 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? \$\endgroup\$ – Uri Agassi Feb 8 '14 at 19:10

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.