Saturday, August 30, 2008

Open Source Code Review - A choice to name a method "Inscrutable"?

I've been reviewing various open source projects related to Amazon S3 and discovered the excellent Resourceful library and the related project SpaceBlock.  The following code is a utility class in the SpaceBlock project. 
The method I found offensive was named "Inscrutable" and is located in the class named F.  The definition of "Inscrutable" from is "incapable of being investigated, analyzed, or scrutinized; impenetrable."  This reminds me of a quote I heard 25 years ago from an old mainframe programmer at Westinghouse.  I was complaining about the understandability of his VMS code and he said "If it was hard to write, it should be hard to understand".  I couldn't have disagreed with him more.  I believe in intellectual manageability of the code you write.  You don't write code for yourself or the compiler.  You write it for other people.  By naming a function "Inscrutable" you are flipping me and everyone else the bird! 
I guarantee this will happen to you one day (if it has not already).  You will be reviewing your own code for a bug fix and discover a particularly tricky piece of code that you didn't comment or name your function properly.  If you were so arrogant as to name your method "YouAreTooStupidToUnderstandHowThisWorksSoDontEvenTry" you will be essentially flipping yourself the bird!  You will then end up spending way too much time figuring out and reflecting on exactly you were thinking when you wrote that code.
Now, I'm not saying I'm not guilty of doing similar complexities with my code.  In fact, I can hear my co-workers now shouting over the cubical walls "Hey, isn't that the pot calling the kettle black".  I'm posting this as a reminder to myself and all developers to never do something this blatant and egotistic as this in our code.
I am not slamming these projects. I think they are well written and have a great coding style. I plan to use them in the future.


John Spurlock said...

Heh, actually I named it Inscrutable because *I* couldn't understand it - was basically experimenting with psuedo-functional C# behavior and seeing how far the compiler took type inference. (using algebraic-like refactoring)

I take your point tho, perhaps a warning label on this class would be a good idea... btw "F" stands for "functional"

Hope that helps!
- John

D. Mark Lindell said...

John, I thought you might show up and comment here. Thanks. I understood that these methods were representing functional programming style. I have a very similar class I've written with various methods eg. Reduce, Map, Filter, etc.

Great library, thanks for you hard work.

allingstonhowsham said...

the open source code review tools are important aspects for the programming. Good info added. thank you for sharing.