This is a bad com­ment:

int i = 0;
i + 1; // adding 1 to i should work but dont work i dont know why!!!!
print(i);

For every good com­ment, I've seen hun­dreds of bad com­ments. They harm the code base and give the wrong im­pres­sion to the au­thor that he have made the code bet­ter. The fol­low­ing clas­si­fi­ca­tion may help those pro­gram­mers to un­der­stand why they should avoid writ­ing most of their com­ments. Once they un­der­stand that, learn­ing to write good com­ments wouldn't be too dif­fi­cult.

Name	Harm lev­el	Steps to re­move	In­dica­tive of pro­gram­mer lev­el
Ex­tract-friend­ly com­ments	harm­ful	refac­tor­ing	be­gin­ner
Own­er­ship com­ments	harm­ful	im­me­di­ate re­moval	new­bie
Mis­lead­ing com­ments	harm­ful	im­me­di­ate re­moval	N/A
Re­dun­dant com­ments	an­noy­ing	im­me­di­ate re­moval	new­bie
Code com­ments	an­noy­ing	im­me­di­ate re­moval	new­bie
Nam­ing com­ments	harm­ful	refac­tor­ing	be­gin­ner
Lol­cat com­ments	an­noy­ing	im­me­di­ate re­moval	be­gin­ner
Drunk­en man com­ments	harm­ful	im­me­di­ate re­moval	new­bie
Prose com­ments	an­noy­ing	re­for­mu­la­tion or re­moval	N/A
I have a prob­lem com­ments	an­noy­ing	re­for­mu­la­tion or re­moval	N/A
Ex­plana­to­ry com­ments	none	none	none

Ex­tract-friend­ly com­ments

A few months ago, I was talk­ing with a be­gin­ner pro­gram­mer who told me that he was proud of his re­cent progress, one of the en­hance­ments to his code be­ing the fact that he start­ed to put more com­ments.

I've seen his code. It looks like this:

public class Product
{
    // Other properties and methods.

    public string ComputePrice(
        Customer customer, PricingOptions options, bool includeRebate,
        decimal defaultVAT, bool includeRanges,
        List<PricingRange> pricingRanges, DateTime overrideStartTime)
    {
        // Let's get the base price.
        <<Ten to twenty lines of code which compute the base price; no comments>>

        if (includeRanges && overrideStartTime != DateTime.MinValue)
        {
            // The price depends on the princing ranges. Let's calculate it.
            foreach (PricingRange r in pricingRanges)
            {
                if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
                {
                    <<Thirty lines of difficult to understand code; no comments>>
                }
            }
        }
        else if (includeRanges && overrideStartTime == DateTime.MinValue)
        {
            // Use the start time instead of now.
            foreach (PricingRange r in pricingRanges)
            {
                if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
                {
                    <<Thirty lines of difficult to understand code which looks
                      like the code from the previous block, but has slight,
                      barely noticeable differences; no comments>>
                }
            }
        }

        // Including VAT.
        <<Ten to twenty lines of code which compute VAT; no comments>>

        if (includeRebate)
        {
            // Substract the rebate.
            <<Ten to twenty lines of code which substract the rebate; no comments>>
        }
    }

    return price;
}

Let's be fair: this pro­gram­mer did in­clude some com­ments. Are they use­ful? Sort of. Does it mean he un­der­stand ba­sics of pro­gram­ming? Nah, not re­al­ly.

There are many neg­a­tive things to say about the code above, but I want to fo­cus on a sin­gle thing: the lack of refac­tor­ing and how is this re­lat­ed to com­ments. When in­spect­ing the sig­na­ture and the body of the method, it be­comes ob­vi­ous how it grew. Orig­i­nal­ly, the method was short with few ar­gu­ments; some­thing like:

public string ComputePrice(
    Customer customer, PricingOptions options, bool includeRebate)

Then there was a need to add de­fault VAT. Then the ranges. And fi­nal­ly the abil­i­ty to over­ride the start time, at which time the method be­comes par­tic­u­lar­ly creepy.

Each ad­di­tion was done with­out cary­ing much about the read­abil­i­ty. In­stead of keep­ing the sig­na­ture trans­par­ent, the ar­gu­ments were just ap­pend­ed to the oth­ers. To­day, with­out look­ing at the code, there is no way to even try to guess what overrideStartTime is used for. The same care­less, un­con­trolled or­gan­ic growth is clear­ly vis­i­ble in the body of the method. It looks like when the pro­gram­mer added overrideStartTime, the code was al­ready par­tic­u­lar­ly un­read­able, which ex­plains (but not ex­cus­es) the ugly copy-paste of thir­ty LOC fol­lowed by small changes in the copied code.

Let's do the ba­sic refac­tor­ing work which should have been done from the be­gin­ning. The first refac­tor­ing would con­sist of ex­tract­ing the parts of the method. This is when I should ad­mit that ac­tu­al com­ments are not to­tal­ly use­less: they show pret­ty well the lo­ca­tions where the code should be cut out. Af­ter the refac­tor­ing, those com­ments could be re­moved with no ef­fect rather than clean­ing up the code base.

public class Product
{
    // Other properties and methods.

    public string ComputePrice(
        Customer customer, PricingOptions options, bool includeRebate,
        decimal defaultVAT, bool includeRanges,
        List<PricingRange> pricingRanges, DateTime overrideStartTime)
    {
        // Ugly type here. We can't do anything with it for the moment, since
        // both the decimal and the string are used later when including the VAT
        // and the rebate.
        Tuple<decimal, string> basePrice = this.ComputeBasePrice();

        if (includeRanges && overrideStartTime != DateTime.MinValue)
        {
            // The price depends on the princing ranges. Let's calculate it.
            foreach (PricingRange r in pricingRanges)
            {
                if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
                {
                    basePrice = this.ComputeBasePriceWithinRange(
                        customer, options, pricingRanges);
                }
            }
        }
        else if (includeRanges && overrideStartTime == DateTime.MinValue)
        {
            // Use the start time instead of now.
            foreach (PricingRange r in pricingRanges)
            {
                if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
                {
                    basePrice = this.ComputeBasePriceWithinRange(
                        customer, options, pricingRanges, overrideStartTime);
                }
            }
        }

        Tuple<decimal, string> priceWithTaxes = this.IncludeVAT(
            basePrice, customer, options, defaultVAT);

        if (includeRebate)
        {
            return this.IncludeRebate(priceWithTaxes.Item1, customer);
        }

        return priceWithTaxes.Item2;
    }

    private Tuple<decimal, string> ComputeBasePrice()
    {
        <<Ten to twenty lines of code which compute the base price; no comments>>
    }

    private Tuple<decimal, string> ComputeBasePriceWithinRange(
        Customer customer, PricingOptions options, List<PricingRange> pricingRanges)
    {
        <<Thirty lines of difficult to understand code; no comments>>
    }

    private Tuple<decimal, string> ComputeBasePriceWithinRange(
        Customer customer, PricingOptions options,
        List<PricingRange> pricingRanges, DateTime overrideStartTime)
    {
        <<Thirty lines of difficult to understand code which looks like the code
          from the overload, but has slight, barely noticeable differences; no
          comments>>
    }

    private Tuple<decimal, string> IncludeVAT(
        Tuple<decimal, string> price, Customer customer, PricingOptions options,
        decimal defaultVAT)
    {
        <<Ten to twenty lines of code which compute VAT; no comments>>
    }

    private string IncludeRebate(decimal price, Customer customer)
    {
        <<Ten to twenty lines of code which substract the rebate; no comments>>
    }
}

Those thir­ty lines of du­pli­cat­ed code in ComputeBasePriceWithinRange is re­al­ly an­noy­ing. Hope­ful­ly, refac­tor­ing can solve that.

public class Product
{
    // Other properties and methods.

    public string ComputePrice(
        Customer customer, PricingOptions options, bool includeRebate,
        decimal defaultVAT, bool includeRanges,
        List<PricingRange> pricingRanges, DateTime? overrideStartTime)
    {
        // Ugly type here. We can't do anything with it for the moment, since
        // both the decimal and the string are used later when including the VAT
        // and the rebate.
        Tuple<decimal, string> basePrice = this.ComputeBasePrice();

        if (includeRanges)
        {
            foreach (PricingRange r in pricingRanges)
            {
                if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
                {
                    basePrice = this.ComputeBasePriceWithinRange(
                        customer, options, pricingRanges, overrideStartTime);
                }
            }
        }

        Tuple<decimal, string> priceWithTaxes = this.IncludeVAT(
            basePrice, customer, options, defaultVAT);

        if (includeRebate)
        {
            return this.IncludeRebate(priceWithTaxes.Item1, customer);
        }

        return priceWithTaxes.Item2;
    }

    private Tuple<decimal, string> ComputeBasePrice()
    {
        <<Ten to twenty lines of code which compute the base price; no comments>>
    }

    private Tuple<decimal, string> ComputeBasePriceWithinRange(
        Customer customer, PricingOptions options,
        List<PricingRange> pricingRanges, DateTime? overrideStartTime = null)
    {
        <<Thirty lines of difficult to understand code; no comments>>
    }

    private Tuple<decimal, string> IncludeVAT(
        Tuple<decimal, string> price, Customer customer, PricingOptions options,
        decimal defaultVAT)
    {
        <<Ten to twenty lines of code which compute VAT; no comments>>
    }

    private string IncludeRebate(decimal price, Customer customer)
    {
        <<Ten to twenty lines of code which substract the rebate; no comments>>
    }
}

An­oth­er par­tic­u­lar­ly an­noy­ing thing is the lack of ap­pro­pri­ate types, which be­came only worse with the in­tro­duc­tion of tu­ples dur­ing the refac­tor­ing.

public class Product
{
    // Other properties and methods.

    public Price ComputePrice(
        Customer customer, PricingOptions options, bool includeRebate,
        decimal defaultVAT, RangeOptions rangeOptions)
    {
        var basePrice = this.ComputeBasePrice();

        if (rangeOptions.IncludeRanges)
        {
            foreach (PricingRange r in rangeOptions.PricingRanges)
            {
                if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
                {
                    basePrice = this.ComputeBasePriceWithinRange(
                        customer, options, pricingRanges,
                        rangeOptions.OverrideStartTime);
                }
            }
        }

        var priceWithTaxes = this.IncludeVAT(
            basePrice, customer, options, defaultVAT);

        return includeRebate ?
            this.IncludeRebate(priceWithTaxes, customer) :
            priceWithTaxes;
    }

    private Price ComputeBasePrice()
    {
        <<Ten to twenty lines of code which compute the base price; no comments>>
    }

    private Price ComputeBasePriceWithinRange(
        Customer customer, PricingOptions options,
        List<PricingRange> pricingRanges, DateTime? overrideStartTime = null)
    {
        <<Thirty lines of difficult to understand code; no comments>>
    }

    private Price IncludeVAT(
        Price price, Customer customer, PricingOptions options, decimal defaultVAT)
    {
        <<Ten to twenty lines of code which compute VAT; no comments>>
    }

    private Price IncludeRebate(Price price, Customer customer)
    {
        <<Ten to twenty lines of code which substract the rebate; no comments>>
    }
}

Do all those meth­ods need to know what they know from the pa­ra­me­ters? Prob­a­bly not.

public class Product
{
    // Other properties and methods.

    public Price ComputePrice(
        Customer customer, PricingOptions options, bool includeRebate,
        decimal defaultVAT, RangeOptions rangeOptions)
    {
        var basePrice = this.ComputeBasePrice();

        if (rangeOptions.IncludeRanges)
        {
            foreach (PricingRange r in rangeOptions.PricingRanges)
            {
                if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
                {
                    basePrice = this.ComputeBasePriceWithinRange(
                        customer.RegisterDate, options.EnableVipStatus,
                        pricingRanges, rangeOptions.OverrideStartTime);
                }
            }
        }

        var priceWithTaxes = this.IncludeVAT(
            basePrice, customer.vatOverride, options.taxMap, defaultVAT);

        return includeRebate ?
            this.IncludeRebate(priceWithTaxes, customer.RetrieveRebateFor(this)) :
            priceWithTaxes;
    }

    private Price ComputeBasePrice()
    {
        <<Ten to twenty lines of code which compute the base price; no comments>>
    }

    private Price ComputeBasePriceWithinRange(
        DateTime customerRegisterDate, bool enableVipStatus,
        IEnumerable<PricingRange> pricingRanges, DateTime? overrideStartTime = null)
    {
        <<Thirty lines of difficult to understand code; no comments>>
    }

    private Price IncludeVAT(
        Price price, decimal customerVAT, TaxMap vatMap, decimal defaultVAT)
    {
        <<Ten to twenty lines of code which compute VAT; no comments>>
    }

    private Price IncludeRebate(Price price, Rebate customerRebate)
    {
        <<Ten to twenty lines of code which substract the rebate; no comments>>
    }
}

The ComputePrice method is now read­able enough for this ar­ti­cle, but let's just push the refac­tor­ing a bit fur­ther by in­tro­duc­ing a builder pat­tern.

internal class PriceBuilder
{
    public PriceBuilder(Product product)
    {
        this.
    }

    public FindBasePrice()
    {
        <<Ten to twenty lines of code which compute the base price; no comments>>
    }

    public FindBasePriceWithinRange(
        RangeOptions rangeOptions, DateTime customerRegisterDate,
        bool enableVipStatus, IEnumerable<PricingRange> pricingRanges,
        DateTime? overrideStartTime = null)
    {
        if (!rangeOptions.IncludeRanges)
        {
            return this;
        }

        foreach (PricingRange r in rangeOptions.PricingRanges)
        {
            if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
            {
                <<Thirty lines of difficult to understand code; no comments>>
            }
        }
    }

    private PriceBuilder IncludeVAT(
        decimal customerVAT, TaxMap vatMap, decimal defaultVAT)
    {
        <<Ten to twenty lines of code which compute VAT; no comments>>
    }

    private PriceBuilder IncludeRebate(Rebate customerRebate)
    {
        if (customerRebate)
        {
            return this;
        }

        <<Ten to twenty lines of code which substract the rebate; no comments>>
    }

    private Price Build()
    {
        // Return the price.
    }
}

public class Product
{
    // Other properties and methods.

    public Price ComputePrice(
        Customer customer, PricingOptions options, bool includeRebate,
        decimal defaultVAT, RangeOptions rangeOptions)
    {
        var builder = rangeOptions.IncludeRanges ?
            new PriceBuilder().FindBasePriceWithinRange() :
            new PriceBuilder().FindBasePrice();

        return builder
            .IncludeVAT(customer.vatOverride, options.taxMap, defaultVAT)
            .IncludeRebate(includeRebate ? customer.RetrieveRebateFor(this) : null)
            .Build();
    }
}

If we look at the way the base price is com­put­ed and think twice about ob­ject ori­ent­ed pro­gram­ming, we can't avoid notic­ing that this is the clas­si­cal case where if log­ic can be re­placed by in­her­i­tance. For that, we make PriceBuilder ab­stract and cre­ate two chil­dren: OrdinaryPriceBuilder and RangePriceBuilder. The chain can then be re­placed by:

return builder
    .FindTaxFreePrice()
    .IncludeVAT(customer.vatOverride, options.taxMap, defaultVAT)
    .IncludeRebate(includeRebate ? customer.RetrieveRebateFor(this) : null)
    .Build();

sim­pli­fy­ing even more the code.

The ugli­est part is now the fol­low­ing piece of code:

foreach (PricingRange r in rangeOptions.PricingRanges)
{
    if (r.Start >= DateTime.Now && r.End <= DateTime.Now)
    {
        <<Thirty lines of difficult to understand code; no comments>>
    }
}

This is solv­able as well. First, PricingRange may in­clude a method Contains which takes a date as its sin­gle pa­ra­me­ter. With cor­rect­ly re­named vari­ables, the code now be­comes:

foreach (PricingRange range in rangeOptions.PricingRanges)
{
    if (range.Contains(DateTime.Now))
    {
        <<Thirty lines of difficult to understand code; no comments>>
    }
}

The prop­er­ty IsCurrent may then be cre­at­ed with­in the PricingRange:

public bool IsCurrent
{
    get
    {
        return this.Contains(DateTime.Now);
    }
}

and, un­less the code was ob­ject of pro­fil­ing and op­ti­miza­tion, the fur­ther read­abil­i­ty en­hance­ment should be done by re­mov­ing one lev­el of in­den­ta­tion:

foreach (PricingRange range in rangeOptions.PricingRanges.Where(r => r.IsCurrent))
{
    <<Thirty lines of difficult to understand code; no comments>>
}

Op­tion­al­ly, one can push this even fur­ther, by en­com­pass­ing the list of pric­ing ranges with­in a class such as class PricingRanges : IEnumerable<PricingRange>, and cre­ate a spe­cif­ic method which se­lects only the cur­rent ranges. The foreach then be­comes:

foreach (PricingRange range in rangeOptions.PricingRanges.Current)
{
    <<Thirty lines of difficult to understand code; no comments>>
}

What we ob­tain now? A mix be­tween clear, ob­ject ori­ent­ed code, and blocks of ten to thir­ty lines of not al­ways ob­vi­ous code. Each of those blocks should be refac­tored in the same way we did above. There are few chances to have a few lines which are too ob­scure and prac­ti­cal­ly im­pos­si­ble to make bet­ter through refac­tor­ing. Then and only then, com­ments may be used to en­hance the read­abil­i­ty.

We have seen on this ex­am­ple one type of com­ment: the one which tries to ex­plain what the fol­low­ing block of code is do­ing, but most­ly cries for refac­tor­ing. I call it ex­tract-friend­ly com­ment. What are oth­er types of com­ments?

Let's study the method which sub­tracts the re­bate in the fol­low­ing sec­tions.

private PriceBuilder IncludeRebate(Rebate customerRebate)
{
    if (customerRebate)
    {
        return this;
    }

    var result = this.Clone();
    foreach (var productRebate in this.Product.Rebates)
    {
        // Ensure the rebate applies to this product.
        if (productRebate.Id == customerRebate.Id)
        {
            // Ensure the rebate is gold and applies globally.
            if (productRebate.AppliesGlobally ||
                (customerRebate.IsGold &&
                 customerRebate.CanEnlistFor(this.Product)))
            {
                var errors = customerRebate.Enlist(this.Product);
                if (!errors.Any())
                {
                    //rebate.MarkAsApplied();
                    //if (!rebate.IsApplied)
                    //{
                    //    throw new Exception("Not working!");
                    //}
                    result.Price.Amount -= customerRebate.Amount;
                }
            }
            else
            {
                // Changed on 2013-10-08 by Jeff.
                if (this.Product.CanAcceptExceptionalRebates() &&
                    (customerRebate.IsSilver || customerRebate.IsGold))
                {
                    // In the current case, the rebate can apply to the products
                    // with the price superior or equal to $20.
                    if (result.Price.Amount > customerRebate.Amount &&
                        result.Price.Amount > 20)
                    {
                        // Substracting the amount.
                        result.Price.Amount -= customerRebate.Amount;
                    }
                }
            }
        }
    }

    return result;
}

Own­er­ship com­ments

The com­ment // Changed on 2013-10-08 by Jeff. is an own­er­ship com­ment. In­stead of help­ing un­der­stand­ing the code, it pol­lutes it with use­less in­for­ma­tion and shows that the au­thor doesn't un­der­stand or doesn't use ver­sion con­trol.

If the au­thor doesn't use ver­sion con­trol, there is no point talk­ing about use­ful­ness of com­ments: there are more im­por­tant things to do, i.e. start­ing to use tools which are manda­to­ry for every team of one or more pro­gram­mers. If the au­thor uses ver­sion con­trol, this com­ment is ei­ther re­dun­dant (the log al­ready shows the au­thor and the date), or wrong (for ex­am­ple if the code was copy-past­ed by Nick in Jan­u­ary 2014).

Such com­ments are harm­ful and should be killed im­me­di­ate­ly.

Mis­lead­ing com­ments

Some com­ments may sim­ply be wrong. They are par­tic­u­lar­ly harm­ful, since they can lead to bugs and force pro­gram­mers to spend more time read­ing code than they need to. This is the case of the com­ment // Ensure the rebate is gold and applies globally.. The con­di­tion which fol­lows doesn't check for that. It checks whether the prod­uct re­bate ap­plies glob­al­ly, or the cus­tomer re­bate has a gold sta­tus or can be en­list­ed for the spe­cif­ic prod­uct. This has noth­ing to do with the com­ment.

The com­ment // In the current case, the rebate can apply to the products with the price superior or equal to $20. falls into this cat­e­go­ry as well, and is even more dan­ger­ous. While it's easy to see that the first one is mis­lead­ing, it takes some time to no­tice the slight mis­take in the sec­ond one: the price is not su­pe­ri­or or equal, but just su­pe­ri­or. Com­ments which con­tain sub­tle er­rors are the most dan­ger­ous.

Such com­ments are harm­ful and should be killed im­me­di­ate­ly.

Re­dun­dant com­ments

Lots of com­ments I see when in­spect­ing code bases are sim­ply re­peat­ing code. Even when they don't fall into the pre­vi­ous cat­e­go­ry, they add too much noise to de­serve any screen space.

// Subtracting the amount., for ex­am­ple, is a re­dun­dant com­ment. It doesn't bring any­thing use­ful, be­cause any pro­gram­mer who knows the mean­ing of -= un­der­stands what the fol­low­ing line is do­ing. The com­ment // In the current case, the rebate can apply to the products with the price superior or equal to $20., aside be­ing mis­lead­ing, is re­dun­dant as well. It tries to prove its use­ful­ness by spec­i­fy­ing the cur­ren­cy, but would be­come com­plete­ly use­less af­ter ba­sic refac­tor­ing which will re­place the mag­i­cal 20 by ei­ther a prop­er­ty or a con­stant such as MinProductPriceForExceptionalRebate.

Such com­ments are use­less and make too much noise. They should be re­moved.

Code com­ments

Com­ment­ed code is an­oth­er source of noise cur­rent­ly seen in the code of new­bie pro­gram­mers.

//rebate.MarkAsApplied();
//if (!rebate.IsApplied)
//{
//    throw new Exception("Not working!");
//}

Is prob­lem­at­ic for sev­er­al rea­sons:

• It doesn't help un­der­stand­ing the rest of the code.

• No­body knows why this piece of code was kept.

• The in­for­ma­tion is re­dun­dant with ver­sion con­trol.

• The code is ob­so­lete and can­not be used any­way. In the cur­rent case, it also has im­por­tant is­sues, which may in­di­cate that the code was only tem­po­rary, but may also mean that the au­thor doesn't know what he does.

• Com­ment­ed code usu­al­ly wastes an im­por­tant amount of place. I've seen blocks of code span­ning hun­dreds of lines.

Such com­ments should be treat­ed sim­i­lar­ly to re­dun­dant com­ments: be re­moved.

Nam­ing com­ments

The oth­er com­ments which in­vite refac­tor­ing are the com­ments which are here to ex­plain a vari­able or a method with a name which is not clear enough. The refac­tor­ing is ob­vi­ous: name the vari­able or the name dif­fer­ent­ly and re­move the com­ment.

Ex­am­ple:

// Finding the highest rebate for the current product to be used as primary rebate.
var pr = this.BestMatch();

should be refac­tored into:

var primaryRebate = this.FindHighestRebate();

Lol­cat com­ments

Some com­ments, while be­ing writ­ten with a good in­ten­tion, are sim­ply too bad­ly writ­ten to make the code eas­i­er to read.

I al­ready gave a few ex­am­ples of such com­ments in my an­swer to Should san­i­ty be a prop­er­ty of a pro­gram­mer or a pro­gram? ques­tion. In gen­er­al, those com­ments in­clude:

• Ex­ces­sive punc­tu­a­tion, es­pe­cial­ly the ex­cla­ma­tion mark. I'm hap­py for the orig­i­nal au­thor to be so ex­cit­ed about the com­ment, but I would ap­pre­ci­ate more if he took more care writ­ing the com­ment it­self.

  Ex­am­ple: // Got the final result!!!!

• All cap­i­tals or the lack of cap­i­tals when need­ed. This doesn't in­clude words and names which are not ob­vi­ous to write (is it jQuery or JQuery? Is it LESS and Sass or Less and SASS?), but is more about a com­plete lack of at­ten­tion to cap­i­tal­iza­tion rules.

  Ex­am­ple: // the transaction is about to be COMMITTED! if it fails, the counters will not be affected and the ajax call will fail with http 304.

• Gram­mar and spelling.

  Ex­am­ple: // in some case there are no enouh 3 space on harddisk so file cannot save

• Mix of lan­guages. This is prop­er to for­eign pro­gram­mers. I had to read code of many French pro­gram­mers and found some mag­nif­i­cent sen­tences. While many be­gin­ners don't un­der­stand that it is manda­to­ry to know Eng­lish to do pro­gram­ming, peo­ple who know Eng­lish of­ten use this ar­gu­ment to ex­plain why are they mix­ing two lan­guages to­geth­er. They are not be­ing hon­est; the fact is, they are too lazy to write a com­plete sen­tence in ei­ther Eng­lish or French, and mix­ing mul­ti­ple lan­guages en­ables them to not mak­ing any ef­fort to for­mu­late the sen­tence in the first place.

  Ex­am­ple: // Le 2ème product sera traité seulement si enabled est à true.

Lol­cat com­ments give a bad im­age of the code base and dis­tract the pro­gram­mers who read them. Such com­ments should be trans­formed into well for­mu­lat­ed, cor­rect­ly writ­ten pieces of text or re­moved.

Drunk­en man com­ments

Drunk­en man com­ments are sim­i­lar to lol­cat com­ments, but com­bine usu­al­ly all the el­e­ments of lol­cat com­ments in­side a bad­ly con­struct­ed, usu­al­ly mean­ing­less sen­tence. It looks like the au­thor was writ­ing it at 3 AM and is not clear why he did it or for whom.

Some­times, those com­ments are fruit of peo­ple who re­al­ly don't speak Eng­lish. Some­times, they are in­deed writ­ten at 3 AM af­ter six bot­tles of beer.

Ex­am­ple: //not doin' but since same value it still swap!!

Such com­ments are harm­ful and should be killed im­me­di­ate­ly.

Prose com­ments

Some pro­gram­mers are too afraid writ­ing lol­cat com­ments, so they do the op­po­site. They start telling too much, in­stead of be­ing con­cise.

Ex­am­ple:

// Since both values (the value of “original” and the value of “current”) can be compared together, we can start comparing them in order to know if the value was changed since the last time the entity was loaded. This comparison will work correctly at this point, since we already compared the types before, which means that we just have to compare the values themselves. Once this is done, we can decide whether we should continue or not.

A pres­ence of a com­ment in­di­cates that the piece of code is not as clear as it could be, i.e. a flaw. There is no need to fo­cus the at­ten­tion of the read­er on this flaw by mak­ing long com­ments. The com­ment should be as short as pos­si­ble: it should con­tain all the in­for­ma­tion the read­er could need right now, but only this in­for­ma­tion. Any ad­di­tion­al ver­bosi­ty is harm­ful.

In the same way, there is no need to use fig­ures of speach or rich vo­cab­u­lary.

If a pro­gram­mer wants to show things he learnt tak­ing writ­ing lessons, he can share his thoughts in a blog post or a book. Com­ments are not a good place for that, and any­thing not es­sen­tial and not con­struc­tive should be re­moved.

I have a prob­lem com­ments

A few pro­gram­mers don't write prose or po­et­ry be­tween lines of code, but still love talk­ing about their prob­lems. While it might be help­ful, we don't re­al­ly care about their prob­lems, so the for­mu­la­tion is not the best one.

The dan­ger in those com­ments is that while some re­flect prob­lems with­in the code, oth­ers are not con­struc­tive and un­re­lat­ed to code.

• Ex­am­ple of a con­struc­tive but bad­ly for­mu­lat­ed com­ment:

  // I can't find a way to make this work for values inferior or equal to 0.
  if (quantity <= 0)
  {
      throw new NotImplementedException();
  }
  

  This com­ment in­di­cates there is an is­sue with­in the code: it doesn't work for some in­put val­ues. The next pro­gram­mer would find such com­ment help­ful, and may re­act to it by in­spect­ing the re­quire­ments (should neg­a­tive or zero quan­ti­ty be a valid in­put?) and even­tu­al­ly the code (why the code fails for val­ues in­fe­ri­or or equal to zero?)

  While use­ful, the com­ment may be re­for­mu­lat­ed to be more neu­tral:

   // For values inferior or equal to 0, the code fails for an unknown reason.
  

  This helps dif­fer­en­ti­at­ing it bet­ter from the sec­ond type of "I have a prob­lem" com­ments: in­con­struc­tive ones.

• Ex­am­ple of an in­con­struc­tive com­ment:

  // There is a second argument, but I don't understand what it does.
  this.Validate(action, 0);
  

  This is not help­ful. No­body cares that the orig­i­nal au­thor un­der­stands or not the code, and usu­al­ly, the lack of un­der­stand­ing is vis­i­ble enough through the code qual­i­ty to re­quire ad­di­tion­al com­ments from the au­thor.

Ex­plana­to­ry com­ments

The fi­nal cat­e­go­ry rep­re­sents the com­ments which ex­plain in a con­struc­tive and con­cise way things which are not ex­plic­it in the cur­rent code and can't be made ex­plic­it through easy refac­tor­ing.

Ex­am­ple:

// When using Unicode, strings normalized differently would be considered different, while users would expect them to be considered equal. For that reason, the strings are denormalized first.

This is the only cat­e­go­ry of com­ments which should be kept. Well, un­til the code is refac­tored fur­ther, or the lan­guage or frame­work evolves to en­able more ex­pres­sive­ness with­in the source.

How to avoid bad com­ments in the code base?

Code re­views are a pre­cious way to get rid of bad com­ments and teach be­gin­ners why those com­ments are bad. The pri­ma­ry ques­tion which should be asked dur­ing the code re­view when look­ing specif­i­cal­ly for bad com­ments is:

What does the com­ment bring to the clar­i­ty of the code, and why would it be over­whelm­ing to change the code in a way that it would be self-doc­u­ment­ing?

This would lead to four sta­tus­es:

• Use­less com­ments. Those ones can sim­ply be re­moved im­me­di­ate­ly with no loss of in­for­ma­tion for the read­ers of the code. Most cat­e­gories are de­scribed be­low are use­less com­ments.

• Com­ments which can be re­moved af­ter chang­ing code. Those ones are use­ful right now, but won't be once the code is prop­er­ly refac­tored. In­stead of be­ing tru­ly use­ful, they are more in­dica­tive of the is­sues with­in the code it­self.

• Use­ful com­ments which are for­mu­lat­ed bad­ly. Those ones are use­ful in a con­text of a rea­son­ably writ­ten piece of code, but their form is not ap­pro­pri­ate for a com­ment. The cur­rent for­mu­la­tion may lead the re­view­ers to think that the com­ment is use­less, but clos­er in­spec­tion will show that it can't be sim­ply re­moved.

• Use­ful, cor­rect­ly phrased com­ments. Those ones are the rare com­ments which are short, sharp and con­struc­tive com­ments which bring light to an ob­scure piece of code which can't be refac­tored eas­i­ly.

Aside code re­views, show­ing ex­am­ples of bad and good com­ments may be help­ful. I strong­ly rec­om­mend look­ing at .NET source, since it is an ex­cel­lent ex­am­ple of high-qual­i­ty com­ments. In­spect­ing dif­fer­ent open source pro­jects writ­ten by ex­pert de­vel­op­ers may be help­ful as well.