Monday, November 29, 2010

Smelly code

Original, smelly code
public bool isLocked(string s)
{
    if (!File.Exists(s)) return false;
    try { FileStream f = File.Open(s, FileMode.Open, FileAccess.ReadWrite); f.Close(); }
    catch { return true; }
    return false;
}

Objections and Comments
The code abve has in my eyes several problems:

  1. Method name starts with lower case
  2. The name of the input parameter is meaningless
  3. Logical code blocks are not separated by a new line
  4. You need some time to understand what this 4 lines of code do
  5. Multiple instructions on the same line
  6. The "return false;" statement should be inside the try block because it is logically associated to it
  7. Missing Method documentation


Proposed Refactoring
/// <summary>
/// Checks if a file is in use or not.
/// </summary>
/// <param name="filePath">The path to the file that should be checked.</param>
/// <returns>True if the file is locked; false otherwise</returns>
public bool IsLocked(string filePath)
{
    if (!File.Exists(filePath))
    {
        return false;
    }

    try
    {
        FileStream fileStream = File.Open(filePath, FileMode.Open, FileAccess.ReadWrite);
        fileStream.Close();
        return false;
    }
    catch
    {
        return true;
    }
}