CodeSOD: Mr Number

Ted's company hired a contract team to build an application. The budget eventually ran out without a finished application, so the code the contract team had produced was handed off to Ted's team to finish.

This is an example of the Ruby code Ted inherited:

def self.is_uniqueness(mr_number) out = false mrn = PatientMrn.find_by_mr_number(mr_number) if mrn out = true return mrn end return nil end

The function is called is_uniqueness which is definitely some language barrier naming (is_unique is a more English way of wording it). But if we trace through the logic, this is just a wrapper around PatientMrn.find_by_mr_number- it returns an "mrn".

So, the first obvious problem: this isn't checking uniqueness in any way, shape or form.

Then there's the whole check for a valid record- either we find a record or we return nil. But since find_by_mr_number is clearly returning something falsy, that doesn't seem necessary.

And that is the default behavior for the Rails generated find_by methods- they just return nil if there are no records. So none of the checks are needed here. This whole method isn't needed here.

Finally, there's out. I have no idea what they were trying to accomplish here, but it smells like they wanted a global variable that they could check after the call for error statuses. If that was their goal, they failed in a few ways- first, returning nil conveys the same information. Second, global variables in Ruby get a $ sigil in front of them.

What the out variable truly represents is "do I want out of this codebase?" True. That is definitely true.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!