thangchung / clean-code-dotnet

:bathtub: Clean Code concepts and tools adapted for .NET
MIT License
7.13k stars 1.1k forks source link

Only comment things that have business logic complexity section sample is not correct #32

Closed avitsidis closed 5 years ago

avitsidis commented 5 years ago

I consider the good sample as a bad sample. Here is the actual sample code

public string hashIt(string data) 
{
  var hash = 0;
  const length = data.length;

  for (var i = 0; i < length; i++) 
  {
    const char = data.charCodeAt(i);
    hash = ((hash << 5) - hash) + char;

    // Convert to 32-bit integer
    hash &= hash;
  }
}

Aside the typos (function name, const without type, ...) the comment (Convert to 32-bit integer) can be avoided with another function so that you do not need a comment anymore. According to me, a comment would be useful to explain why we choose this hashing algorythm. I'd like to propose you this version of the code:

public int Hash(string data)
{
    var hash = 0;
    var length = data.Length;

    for (var i = 0; i < length; i++)
    {
        var character = data[i];
        // use of djb2 hash algorithm as it has a good compromise
        // between speed and low collision with a very simple implementation
        hash = ((hash << 5) - hash) + character;

        hash = ConvertTo32bitInt(hash);
    }
    return hash;
}

private int ConvertTo32bitInt(int value)
{
    return value & value;
}

Here is my philosophy about comments in code: If a comment explain WHAT the code is doing, it is probably a bad comment and can be implemented with a well named variable or function. However it is hard to express by code WHY you implemented something that way. In my sample, it would be hard to express by code why the code is using djb2 instead of a sha-1 or another algorithm so a comment is acceptable.

thangchung commented 5 years ago

@avitsidis I totally agree with your suggestion. That would be great if you can do a PR about it. Or if not, I will use your own code

public int Hash(string data)
{
    var hash = 0;
    var length = data.Length;

    for (var i = 0; i < length; i++)
    {
        var character = data[i];
        // use of djb2 hash algorithm as it has a good compromise
        // between speed and low collision with a very simple implementation
        hash = ((hash << 5) - hash) + character;

        hash = ConvertTo32bitInt(hash);
    }
    return hash;
}

private int ConvertTo32bitInt(int value)
{
    return value & value;
}

How do you think?

thangchung commented 5 years ago

Closed it because we will work on #41