[nonchalance]

> it was only supposed to pre-pend one or two zeros when importing zip codes from CSV files. Anything with fewer than three digits is supposed to be an invalid code

The true problem here lies with the poor choice of function name on the part of the original author and lack of commentary. The name "formatted_zip_code" automatically primes people to expect that you passed a (possibly mal-formed) zip code and the function should format it properly. Under that metric, the original code was indeed deficient and should have been changed. Whether it retained the original style is secondary, but there were clear functional deficiencies.

"Anything with fewer than three digits is supposed to be an invalid code" is not reflected in the code (why does it return digits and not trigger an error there?) and apparently there are no comments, so it should be no surprise that it needed to be fixed.

[gmjoe]

Exactly. The fault lies with the original author in not explaining the intent behind the function, why it is written in the way it is.

I've seen this kind of code time and again from coworkers who have the "code should be self-documenting" mentality, who refuse to put in comments explaining why they wrote a function a particular way. But what they think is self-documenting, very, very, very rarely is to others.

[snorkel]

Excellent point. Clear concise code only explains what it does, names and comments should be added to explain why it is needed, how is it being used.

[collyw]

Problem is that specs change and need tweaks afterwards. I wouldn't always think to update a method name, though I am quite liberal with comments. I guess I should start to look out for that as a way of improving my code.

[danielweber]

Maybe another component threw the error when zip codes were not 5 digits long.

The whole tale is just "if it ain't broke, don't fix it."

[drstewart]

>The whole tale is just "if it ain't broke, don't fix it."

I struggle between this and the accumulation of technical debt. Sure, it's not broken... but that doesn't mean it can't be improved.

[lifeformed]

This is the correct code:

  def formatted_zip_code(digits)
    case digits.size 
    when 4 then "0#{digits}" 
    when 3 then "00#{digits}" 
    # 2 or less digits is an invalid input
    else digits 
    end
  end

[pnathan]

I'd improve that because of this:

-> if the argument doesn't conform to contract, an assert/exception should be thrown so that the caller can recognize the existence of an incorrectly used function.

[dekz]

Are there zip codes that are 2 digits? I'd fall fallacy to this post and once again argue to the contrary. If this function is about formatting zip codes with leading zeros, and zip codes are either 4 or 3 digits. Why would an invalid zip code be passed to this method, a formatter?

Normally I'd agree with your argument about failing to meet a contract, but why would this formatter method get anything but a valid zip code.

[nonchalance]

The first 3 digits of a zip code represent the sectional center facility code, the smallest of which is 005. One would assume that 00501 is the smallest zip code (which apparently maps to HOLTSVILLE NY )

http://pe.usps.com/text/LabelingLists/L002.htm#xd_L002

https://tools.usps.com/go/ZipLookupAction!input.action

[pnathan]

> why would this formatter method get anything but a valid zip code.

Ask not, lest ye receive an answer.

Seriously speaking, you can't trust that callers will be sane unless you establish a priori with a higher level contract that all possible callers will be sane.

[nonchalance]

I'd argue that, since the comment regards valid input, the note should appear at the top.