CodeSOD: Not Exactly Gems

Sammy‘s company “jumped on the Ruby on Rails bandwagon since there was one on which to jump”, and are still very much a Rails shop. The company has been around for thirty years, and in that time has seen plenty of ups and downs. During one of those “ups”, management decided they needed to scale up, both in terms of staffing and in terms of client base- so they hired an offshore team to promote international business and add to their staffing.

A “down” followed not long after, and the offshore team was disbanded. So Sammy inherited the code.

I know I’m generally negative on ORM systems, and that includes Rails, but I want to stress: they’re fine if you stay on the happy path. If your data access patterns are simple (which most applications are just basic CRUD!) there’s nothing wrong with using an ORM. But if you’re doing that, you need to use the ORM. Which is not what the offshore team did. For example:

class Request < ActiveRecord::Base
  def self.get_this_years_request_ids(facility_id)  # There are several other methods that are *exactly* the same, except for the year

      requests = Request.where("requests.id in (select t.id from requests as t                    # what is the purpose of this subquery?
              where t.unit_id=token_requests.unit_id and t.facility_id=token_requests.facility_id
              and t.survey_type = '#{TokenRequest::SURVEY_TYPE}'                                  # why is SURVEY_TYPE a constant?
              and EXTRACT( YEAR FROM created_at) = EXTRACT(YEAR FROM current_timestamp)          
                 order by t.id desc) and token_requests.facility_id = #{facility_id.to_i}         # so we get all the requests by year, then by by ???
                 and token_requests.survey_type = '#{Request::SURVEY_TYPE}'")               

Comments from Sammy.

Now, if we just look at the signature of the method, it seems like this should be a pretty straightforward query: get all of the request IDs for a given facility ID, within a certain time range.

And Sammy has helpfully provided a version of this code which does the same thing, but in a more “using the tools correctly” way:

def self.request_ids_for_year(facility_id,year = Time.now.year)
   
    token_requests = TokenRequest.where(
                              :facility_id=>facility_id,
                              :survey_type=>TokenRequest::SURVEY_TYPE,
                              :created_at=>(DateTime.new(year.to_i).beginning_of_year .. DateTime.new(year.to_i).end_of_year))

Now, I don’t know Ruby well enough to be sure, but the DateTime.new(year.to_i) whiffs a bit of some clumsy date handling, but that may be a perfectly cromulent idiom in Ruby. But this code is pretty clear about what it’s doing: finding request objects for a given facility within a given year. Why one uses Request and the other uses TokenRequest is a mystery to me- I’ m going to suspect some bad normalization in the database or errors in how Sammy anonymized the code. That’s neither here nor there.

Once we’ve gotten our list of requests, we need to process them to output them. Here’s how the offshore code converted the list into a comma delimited string, wrapped in parentheses.

	string_token_request_ids = "(-1)"
    if token_requests && token_requests.length > 0
      for token_request in token_requests
        if string_token_request_ids != ""
          string_token_request_ids = string_token_request_ids + ","
        end
        string_token_request_ids = string_token_request_ids + token_request.id.to_s
      end
        string_token_request_ids = "(" + string_token_request_ids + ")"
    end
  end
end

Look, if the problem is to “join a string with delimiters” and you write code that looks like this, just delete your hard drive and start over. You need extra help.

We start by defaulting to (-1) which is presumably a “no results” indicator. But if we have results, we’ll iterate across those results. If our result string is non-empty (which it definitely is non-empty), we append a comma (giving us (-1),). Then we append the current token ID, giving us (-1),5, for example. Once we’ve exhausted all the returned IDs, we wrap the whole thing in parentheses.

So, this code is wrong– it’s only supposed to return (-1) when there are no results, but as written, it embeds that in the results. Presumably the consuming code is able to handle that error gracefully, since the entire project works.

Sammy provides us a more idiomatic (and readable) version of the code which also works correctly:

    return "(#{token_requests.count > 0 ? token_requests.map(&:id).join(',') : '(-1)'})"

I’ll be honest, I hate the fact that this is returning a stringly-typed list of integers, but since I don’t know the context, I’ll let that slide. At the very least, this is a better example of what joining a list of values into a string should look like.

Sammy writes:

It seems these devs never took the time to learn the language. After asking around a bit, I found out they all came from a Java background. Most of this code seems to be from a VB playbook, though.

That’s a huge and undeserved insult to Visual Basic programmers, Sammy. Even they’re not that bad.

[Advertisement]
Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

Source: Read More