What You would do if You get the following result after running a unit test?

Assert.AreEqual failed. Expected:<2016.04.27. 8:22:52>. Actual:<2016.04.27. 8:22:52>.

My head was full with abstractions waiting to be coded, but the above result brings me in
unexpected state. Everything suspended and my eyes were scanning the two values character by
character repeatedly to find out what is the difference? Nothing!
A quick debug revealed that the values differ in milliseconds which are not shown in the message.

But what a misleading message! Maybe the difference should be emphasized somehow!
Because I lost my concentration, my flow, etc.
It was the same when You cannot work quietly because somebody always coming to You and asks
something. Anything. Applying the 8th point of the great article I found on Joel’s blog years before
to this situation: such messages are breaking down productivity and should be avoided.

During the development of many projects I tried to standardize the outlook of methods to help anybody to distinguish between some parts of them.
Check the code below:

        public int SaveNewPartner(Partner partner, Operator modifier)
        {
            if (partner != null)
            {
                if (modifier != null)
                {
                    if (partner.ID == null)
                    {
                        if (!SanityCheck(partner))
                        {
                            throw new ApplicationException("partner failed sanity check");
                        }

                        partner.ModificationTime = DateTime.Now;
                        partner.Modifier = modifier;
                        return Save(partner);
                    }
                    else
                    {
                        throw new InvalidOperationException("Already saved!"); //LOCSTR
                    }
                }
                else
                {
                    throw new ArgumentNullException("partner");
                }
            }
            else
            {
                throw new ArgumentNullException("partner");
            }
        }

I have problems with this code, and if I have them maybe others who meet it later in our project will have too. If You should determine which part is its business functionality it will be probably a shot in the dark. I really need to understand the whole method before I can point out how it works because parameter checking, control flow, exit point mixed with business part.

That’s why I wrote all my methods by following a pattern:

1. check parameters
2. define return value
3. do business functionality
4. return retval

Here is the rewritten method:

        public int SaveNewPartner(Partner partner, Operator modifier)
        {
            if (partner == null)
            {
                throw new ArgumentNullException("partner");
            }
            if (modifier == null)
            {
                throw new ArgumentNullException("modifier");
            }

            int ret = 0;

            if (partner.ID == null)
            {
                throw new InvalidOperationException("Already saved!"); //LOCSTR
            }
            if (!SanityCheck(partner))
            {
                throw new ApplicationException("partner failed sanity check");
            }

            partner.ModificationTime = DateTime.Now;
            ret = Save(partner);

            return ret;
        }

In lines 3-10 the parameter check occures. There should be parameter checking in each public entrypoint of our class (methods, properties). I check all the parameters I use in the given method directly or in private members I call from here. It is not necessary to check params which are simly handled to other public methods (we may not know all the constraints about these, it’s not our business). I neither check the business validity here (line 3 vs. line 14).

In line 12 I define the return value, which I gave always the name ‘ret’. So if You check any line of the method You can clearly identify where the retval is set and You dont need to scroll anywhere to determine what is the retval variable.

In lines 14-24 placed the business logic. All extremalities are closed asap, so no long lasting ifs and unnecessarily deep indentations happen.

In line 26 we return from here. No other inline returns in the method body so the control flow is clear: we enter at the beginning and exit at the end.

The code below is not thread-safe:

class SomethingWithLazyProperty
{
        private OtherThing otherThing = null;
        public OtherThing OtherThing
        {
            get
            {
                if (this.otherThing == null)
                {
                    this.otherThing = new OtherThing();
                }
 
                return this.otherThing;
            }
        }
}

As You may see when two threads enter the getter and the thread switching occures while
both of them are inside the if block two OtherThings will be instantiated and only
one of them will be stored in private field for later uses.

You should use some locking mechanism to avoid this race condition. The appropriate mechanism
may depend on the heaviness of instantiation. Lets choose lock to get out something more
of this example:

class SomethingWithLazyProperty
{
        private object syncRoot = new object();

        private OtherThing otherThing = null;
        public OtherThing OtherThing
        {
            get
            {
                lock(syncRoot)
                {
                    if (this.otherThing == null)
                    {
                        this.otherThing = new OtherThing();
                    }
                }
 
                return this.otherThing;
            }
        }
}

Thread-safety now in place, but the code will be really slow because of
locking every time we use this property. Thread-safety was only important
once at the begining but we slowed down the code for the whole lifecycle!
Modify a little bit the body of getter:

            get
            {
                if (this.otherThing == null)
                {
                    lock(syncRoot)
                    {
                        if (this.otherThing == null)
                        {
                            this.otherThing = new OtherThing();
                        }
                    }
                }
 
                return this.otherThing;
            }

And voilá, the code is thread-safe and as fast as at the original!

Do not use such a construct:

if (something.Count() == 0) 
{ 
   ... 
}

We are not interested in the real count of items in something.
Only the presence of items is the point of our interest.
So write instead of above this:

if (something.Any())
{
   ...
}

This will stop at the first item which will result in better performance in
cases where something contains a lot of items.