About a year ago, during my trip to Paris and Barcelona, I read Refactoring; improving the design of existing code. As it happens I like the way Martin Fowler writes his books and explains his ideas. I won't always agree with the guy but he has some pretty nifty insights.
I've been know to write code that ain't readable, don't we all love one liners that do all the heavy lifting and never fail? Well I do, coding in this way means almost always writing indecipherable code.
You would like an example, well take the next line of php code {syntaxhighlighter brush: php;} <?php ($index & 1) && echo 'gotcha ...'; ?> {/syntaxhighlighter} or for example the next line of bash/sh code {syntaxhighlighter brush: bash;} sh -c 'file="test.tst"; echo "${file##*.}"' {/syntaxhighlighter} Both lines are easy to understand when you've got a clue about what's happening. The first example determines if a certain index is even or odd; when it's odd, it prints to the screen the text 'gotcha';
The second example is also easy to decipher when you know how to read bash string replacement patterns.
Nowadays I will still write code like the previous examples, you'll find it living within my quick and dirty solutions, to test if something does work; you won't find it anymore in the code I write that will live within a production environment. I have no need to anger my co-workers, by abusing obscure language features without adding some comments. And as it happens, I'm way to lazy to write more than I need to.
I love the next mantras:
These will still allow me to write no more code than I need to and write it in such a way that I can still use the obscure features that express my level of language mastering. Those mantras mean nothing more as to remove difficult to understand parts of a method into their own method and name it after it's intent.
Let me give you guys an example of the code I had to bug hunt today.
{syntaxhighlighter brush: c#;}
public static void URLChecker(W3S.Page p_objPage, W3S.Page p_objRootPage, W3S.Page p_objCurrentPage, string p_strHost, string p_strRawURL)
{
String l_strServerName = HttpContext.Current.Request.ServerVariables["SERVER_NAME"];
l_strServerName = l_strServerName.Replace("www.", String.Empty);
String l_strSubdomain = l_strServerName.Substring(0, l_strServerName.IndexOf('.'));
Boolean l_blSubdomain = l_strSubdomain.Substring(0, p_objPage.Tag.Length).Equals(p_objPage.Tag);
Boolean l_blLocalhost = false;
if (p_strRawURL.IndexOf("/Default.aspx")>-1)
l_blLocalhost = true;
if (!l_blSubdomain && !l_blLocalhost)
{
String l_strRedirectUrl = String.Empty;
switch (l_strSubdomain)
{
default:
case "cursuswijzer":
l_strRedirectUrl = "/Cursuswijzer/Overzicht_cursussen/Default.aspx";
break;
case "vacaturewijzer" :
l_strRedirectUrl = "/Vacaturewijzer/Overzicht_vacatures/Default.aspx";
break;
case "carrierewijzer" :
l_strRedirectUrl = "/Carri%C3%A8rewijzer/Default.aspx";
break;
}
HttpContext.Current.Response.Redirect("http://" + l_strServerName + l_strRedirectUrl);
HttpContext.Current.Response.End();
}
}
{/syntaxhighlighter}
The code itself, seems to do some URLChecking, I've no clue why this method wouldn't return a Boolean saying the URL passes the check and why the heck does the method do a Redirect, without even any comment telling me so. Besides these nuisances the method also needs 5 parameters but won't use them all. To me, the method won't even express it's intent.
After some debugging, code reading I determined the culprit which causes the bug I'm hunting hides within the next line:
{syntaxhighlighter brush: c#;}
Boolean l_blSubdomain = l_strSubdomain.Substring(0, p_objPage.Tag.Length).Equals(p_objPage.Tag);
{/syntaxhighlighter}
If it's not immediate clear to you why the line contains a bug don't ponder to long over it, it took me a full five minutes in order to see it.
Whenever the p_objPage.Tag contains more characters as l_strSubdomain, the Substring method throws an ArgumentOutOfRangeException. The simple solution would be to add a try-catch block, rebuild and go for a coffee. I felt the need to rewrite the code, just to prevent the wrath of the one whom has to debug these lines somewhere in the future.
The original Developer could've prevented the bug by adding a try-catch block or by writing code that expresses it's intent a bit more. As it happens, the ASP.NET C# String Class contains a method public Boolean StartsWith(String); when he would've used the StartsWith method, he would've written less code, more clearer code and code that's not prone to an exception.
After I made sure my fix solved the bug, I rewrote the old code into the one that follows shortly. I changed his behemoth method containing at least 40 codelines into 4 distinct methods, each containing at most 15 codelines and I renamed URLChecker to RedirectToExpectedWebsiteSection. I ain't fond of static methods, especially when you won't need them but in order to only have to rewrite the current functionality only and not the whole website I kept the static method calling in the game.
{syntaxhighlighter brush: c#;} public static String GetNormalizedSubdomain() { // clean up the current domain name, remove the www. prefix ... String l_stringDomain = HttpContext.Current.Request.Url.DnsSafeHost).ToLower().Replace("www.", String.Empty); String l_stringSubdomain = l_stringDomain.Substring(0, l_stringDomain.IndexOf('.')); return l_stringSubdomain; } public static String normalizeWebsiteSection(String p_stringWebsiteSection) { // clean up the websiteSection, remove the "overzicht" postfix ... return p_stringWebsiteSection.ToLower().Replace("overzicht", String.Empty); } // The allowed websiteSection names: vacature, cursus, carriere; public static String getUrlForWebsiteSection(String p_stringWebsiteSection) { // @todo Make is configurable through a webinterface ... Hashtable l_hashTableRedirects = new System.Collections.Hashtable(); l_hashTableRedirects.Add("Cursus", "Cursuswijzer/Overzicht_cursussen"); l_hashTableRedirects.Add("Vacature", "Vacaturewijzer/Overzicht_vacatures"); l_hashTableRedirects.Add("Carriere", "Carri%C3%A8rewijzer"); // We need an Firstletter to Uppercase method ... p_stringWebsiteSection = String.Format( "{0}{1}BaseUrl", p_stringWebsiteSection.Substring(0, 1).ToUpper(), p_stringWebsiteSection.Substring(1) ); return String.Format( "http://{0}/{1}/Default.aspx", ConfigurationManager.AppSettings[p_stringWebsiteSection], l_hashTableRedirects[p_stringWebsiteSection] ); } public static void RedirectToExpectedWebsiteSection(String p_stringWebsiteSection) { String l_stringNormalizedWebsiteSection = Function.normalizeWebsiteSection(p_stringWebsiteSection); if (false == Functions.GetNormalizedSubdomain().StartsWith(l_stringNormalizedWebsiteSection)) { HttpContext.Current.Response.Redirect( Function.getUrlForWebsiteSection(l_stringNormalizedWebsiteSection) ); HttpContext.Current.Response.End(); } } {/syntaxhighlighter}