Coding Stories

Parameter names

public void class Person 
{
  …
  public bool IsSimilar(Person person2) { … }
}

What do you think about the name of parameter name ‘person2’. Names of parameters, fields like xxx1, xxx2, xxx3 are not the best choice. In this case maybe a better name would be ‘otherPerson’.
Magic numbers

public void ValidatePhone(string phoneNumber)
{
  … phoneNumber.Contains(“40”);  
}

The “40” don’t’ say nothing to a reader. What this value represent, why is used and so on. If this value is used in only one place you should at least extract a constant in the body of method. In the case the same value is used in different places of the application, you should put this constraints in a common place.

Property names and enums

public class InvitesFilter
{
  public bool SendByLetter { get; set; }
  public bool SendBySms { get; set; }
  public bool SendByMail { get; set; }
  …
}

First of all, do we really need the ‘SendBy’ prefix? We already know that it is in invitation filter. You cannot send an invitation by receiving it.
Also, if we look closely, this should not be a class. This is clear an enum. If you need to support more than one value set, than you should use the ‘Flag’ attribute.

[Flag]
public void InvitesFilter
{
  Letter = 1,
  Sms = 2,
  Mail = 4
}

Don’t forget to set numeric values to enum items that are power of 2.
Common component (assembly)
It is great to have a common component. But try to not mix the common items from UI with the rest of the classes that are common. For example we don’t want to have in this component all the classes plus some XAML. Even if the XAML is something that is used in 2,3 components, you should create a different component for this (xxx.UI.Common).

Property name
xxxCallbackArgs.IsUserCancel – this property is set to true in the moment when the given action was canceled by the user. The name of the property doesn’t transmit the same thing. If I don’t know the implementation, I would say that this property would tell me if the user was canceled. A better name would be HasCancelledByUser or IsCancelledByUser.

Singleton and static properties
There are a lot of places where I usually see something like this:

public class Foo
{
  private Foo _instance;

  public static Foo Instance
  {
    if ( _instance == null )
    {
      _instance = new Foo();
    }
  }
}

If we are in a multi-thread application, there can be a situation when two threads could be in the same time in the IF. A lock should be used after the IF check and another check need to be made – because during the period when we wait the lock another thread can create a new instance.

public class Foo
{
  private Foo _instance;
  private object _padLock = new Object();
  
  public static Foo Instance
  {
    if ( _instance == null )
    {
      lock( _padLock )
      {
        if ( _instance == null )
        {
          _instance = new Foo();
        }
    }
  }
}
}

Single Responsibility Principle
The User class has a property field. This property need to be encrypted, so the class ends up with two private methods used to encrypt and decrypt the password. Do you thing that we should have the implementation of this class defined in the User class?
I would say NO. We should have a different class that can encrypt and decrypt. Not only will the User class manage only the user information, but the encryption algorithm will be easier to test.

Read More

Code refactoring - NULL check

Part 1
Part 2
Part 3

Let’s look over the following code:

public class Foo
{
  SomeFoo _some;
  
  ...

  public int GetA()
  {
    if(_some == null)
    {
      return 0;
    }

    return _some.A;
  }

  public string GetB()
  {
    if(_some == null)
    {
      return null;
    }

    return _some.B;
  }

  public int GetTotalX()
  {
    if(_some == null)
    {
      return -1;
    }

    return _some.A + _some.C;
  }

}

We can see that the IF checks appears in more than one place. Even if is only a simple check, there can be a lot of places where it appears.
If we manage to extract this check in a generic method, than the code would be more legible. Because we don’t return each time the default value of an object we need to be able to return custom “default” value for the case when or object is null.
We can image a method that accept as parameter the object that we check if is null or not and another two parameters that represent the default value that need to be return and a generic function that return the expected value.

private TReturn ExecuteFuncOverObj<TReturn>(object obj, TReturn defaultValue, Func<TReturn> func)
{
  if( obj == null)
  {
    return defaultValue;
  }

  return func.Invoke();
}

In this way our code would look like this:

public class Foo
{
  SomeFoo _some;
  
  ...

  public int GetA()
  {
    return ExecuteFuncOverObj(_some, () => _some.A, 0);
  }

  public string GetB()
  {
    return ExecuteFuncOverObj(_some, () => _some.B, null);
  }

  public int GetTotalX()
  {

    return ExecuteFuncOverObj(_some, () => _some.A + _some.C, -1);
  }

}

If we need this behavior in other methods we could define a helper function or an extension method of Object.
Enjoy!
Part 1
Part 2
Part 3

Read More

Java Script code refactoring - hands on code

Let's see how we can create a mechanism that based on some flags; it will able to determine the status of some objects. The first version of code would look like this:

ItemStatus = {
    Status1: "Status1",
    Status2: "Status2",
    Status3: "Status3"
}();

ItemTypes = {
    Item1: "Item1",
    Item2: "Item2",
    Item3: "Item 3"
}();

var ItemConfiguration = function () {

    function ItemConfiguration() {

    }

    ItemConfiguration.prototype = function () {
        getItemConfiguration: function (itemTypes, flag1, flag2, flag3, flag4) {
            switch (itemTypes) {
                case ItemTypes.Item1:
                    if (flag1 && flag2) {
                        return ItemStatus.Status1;
                    } else if (flag3 || flag4) {
                        return ItemStatus.Status2;
                    }
                    break;
                case ItemTypes.Item2:
                    if (flag3 || flag2) {
                        return ItemStatus.Status3;
                    } else if (flag1 || flag2) {
                        return ItemStatus.Status1;
                    }
                    break;
                case ItemTypes.Item3:
                    if (flag3 || flag1 && flag3) {
                        return ItemStatus.Status1;
                    } else if (flag2 && flag4) {
                        return ItemStatus.Status3;
                    }
                    break;
            }
        }
    };
   
    return ItemConfiguration;
} ();

First problem is related to default values. If the flags combination from switch doesn’t return any value, that we should notify one way or another user about this issues. We end up throwing an expectation.

var ItemConfiguration = function () {

    function ItemConfiguration() {

    }

    ItemConfiguration.prototype = function () {
        getItemConfiguration: function (itemType, flag1, flag2, flag3, flag4) {
            switch (itemType) {
                case ItemTypes.Item1:
                    if (flag1 && flag2) {
                        return ItemStatus.Status1;
                    } else if (flag3 || flag4) {
                        return ItemStatus.Status2;
                    }
                    break;
                case ItemTypes.Item2:
                    if (flag3 || flag2) {
                        return ItemStatus.Status3;
                    } else if (flag1 || flag2) {
                        return ItemStatus.Status1;
                    }
                    break;
                case ItemTypes.Item3:
                    if (flag3 || flag1 && flag3) {
                        return ItemStatus.Status1;
                    } else if (flag2 && flag4) {
                        return ItemStatus.Status3;
                    }
                    break;
            }

            throw {
                message: "No flags matching for " + itemType + "with the flags combination"
                + "flag1: " + flag1 + ", "
                + "flag2: " + flag2 + ", "
                + "flag3: " + flag3 + ", "
                + "flag4: " + flag4 + ", "
            };
        }
    };

    return ItemConfiguration;
} ();

Almost okay we think. When we look over the requirements we notify that the user will need the status for all items. Because of this he will need to make 3 different calls. For each call he would need to add all the parameters over and over again. We can have two possible solutions. One is to create an object that has all this flags. The other option is to change our method to return the status for all our item types. I would go with the second approach.

var ItemConfiguration = function () {

    function ItemConfiguration() {

    }

    ItemConfiguration.prototype = function () {
        getItemsConfiguration: function(flag1, flag2, flag3, flag4) {
            var itemsStatus = new Object();
            itemsStatus[ItemTypes.Item1] = _getStatusForItem1(flag1, flag2, flag3, flag4);
            itemsStatus[ItemTypes.Item2] = _getStatusForItem2(flag1, flag2, flag3, flag4);
            itemsStatus[ItemTypes.Item3] = _getStatusForItem3(flag1, flag2, flag3, flag4);

            return itemsStatus;
        },

        _getStatusForItem1:function(flag1, flag2, flag3, flag4) {
            if (flag1 && flag2) {
                return ItemStatus.Status1;
            } else if (flag3 || flag4) {
                return ItemStatus.Status2;
            }

            _noStatusHandler();
        },

        _getStatusForItem2:function(flag1, flag2, flag3, flag4) {
            if (flag3 || flag2) {
                return ItemStatus.Status3;
            } else if (flag1 || flag2) {
                return ItemStatus.Status1;
            }

            _noStatusHandler();
        },

        _getStatusForItem3:function(flag1, flag2, flag3, flag4) {
            if (flag3 || flag1 && flag3) {
                return ItemStatus.Status1;
            } else if (flag2 && flag4) {
                return ItemStatus.Status3;
            }

            _noStatusHandler();
        },

        _noStatusHandler:function() {
            throw {
                message: "No flags matching for " + itemType + "with the flags combination"
                + "flag1: " + flag1 + ", "
                + "flag2: " + flag2 + ", "
                + "flag3: " + flag3 + ", "
                + "flag4: " + flag4 + ", "
            };
        }
    };

    return ItemConfiguration;
} ();

For each item type we extracted a method that calculates the item status. In the case for the input flags we cannot retrieve a status we throw an exception. In our public method, we create an object that contains our item types and status. In this way we removed the switch and the code looks better.
The thing that has a smell is the 4 parameters that are send each time in our private method. We could create an internal object with these 4 parameters and send it as parameter for each private method. Another option is to create 4 private fields and each private method can use them. Because we don’t have any problem with thread concurrency in JavaScript we can go with the second approach without any problem.

var ItemConfiguration = function () {

    var flag1, flag2, flag3, flag4;
   
    function ItemConfiguration() {

    }

    ItemConfiguration.prototype = function () {
        getItemsConfiguration: function(flag1, flag2, flag3, flag4) {
            this.flag1 = flag1;
            this.flag2 = flag2;
            this.flag3 = flag3;
            this.flag4 = flag4;
           
            var itemsStatus = new Object();
            itemsStatus[ItemTypes.Item1] = _getStatusForItem1();
            itemsStatus[ItemTypes.Item2] = _getStatusForItem2();
            itemsStatus[ItemTypes.Item3] = _getStatusForItem3();

            return itemsStatus;
        },

        _getStatusForItem1:function() {
            if (this.flag1 && this.flag2) {
                return ItemStatus.Status1;
            } else if (this.flag3 || this.flag4) {
                return ItemStatus.Status2;
            }

            _noStatusHandler();
        },

        _getStatusForItem2:function() {
            if (this.flag3 || this.flag2) {
                return ItemStatus.Status3;
            } else if (this.flag1 || this.flag2) {
                return ItemStatus.Status1;
            }

            _noStatusHandler();
        },

        _getStatusForItem3:function() {
            if (this.flag3 || this.flag1 && this.flag3) {
                return ItemStatus.Status1;
            } else if (this.flag2 && this.flag4) {
                return ItemStatus.Status3;
            }

            _noStatusHandler();
        },

        _noStatusHandler:function() {
            throw {
                message: "No flags matching for " + itemType + "with the flags combination"
                + "flag1: " + flag1 + ", "
                + "flag2: " + flag2 + ", "
                + "flag3: " + flag3 + ", "
                + "flag4: " + flag4 + ", "
            };
        }
    };

    return ItemConfiguration;
} ();

It seems that the code looks cleaner in this way and is easier to read. For the private method, I would let the user to specify all the parameters because is more clear for him and he don’t need to create a new object to send the parameters and use it only in one place.
One thing that came to my mind is to create an object or a dictionary that contains the private functions for each item type. But it would be to complex and I don’t know if is worth it.
What do you think? Do you see another way to implement it?

Last edit: Another option is to create a mapping based on the flags (for example each combinations of flag has a unique key that point to an object that has our item types configuration –ex: flag1*2^0+flag2*2^1+flag3*2^3 in base 10 or we could use a mapping in base 2). What is my concern for this solution is the complexity of the code increase, even if the numbers of line are less and we don’t have the IF statements anymore.

 

Read More

Don't name your class "XXXManager"

How to you name your classes? How many classes you have in your current project named "Manager" and "Repository"? In the last period of time I saw a lot of classes named "Manager", so may that I begin to think that this is a suffix for any class that do some actions.
In this blog post I will talk about some suffixed that can be used when we need to name a class. First of all let why I don't like the "Manager" suffix? Because you cannot understand what the class do. Basically, "Manager" is so general that you can put almost anything there. When I read the name of the class I expect to understand what it does. For example a class with "Writer" suffix will tell me that this class is used to write data.
In the next part of the post I will enumerate some common suffix and when we can use them:

• XXXFactory - Create objects
• XXXWritter - Write data to a specific location
• XXXReader - Read data from a specific location
• XXXProtocol - Define a specific protocal
• XXXConverter - A coverter that is used to convert from one type to another
• XXXShepherd - Manage the lifetime of objects from creation to unload (delete)
• XXXCoordinator - Coordinates a specific action or object
• XXXBuilder - Is able to create new instances of objects
• XXXContainer - A container of elements
• XXXHandler - An handler for some a functionality
• XXXController - Define a controller for an object
• XXXEntity - Define an entity
• XXXView - Define a view for a specific class.
• XXXTarget
• XXXSynchronizer
• XXXBucket
• XXXAttribute
• XXXType
• XXXEditor
• XXXBase
• XXXNode
• XXXItem
• XXXInfo
• XXXHelper
• XXXProvider
• XXXException
• XXXService (you can have the same problem as with Manager)

And so on...
There are so many ways how we suffix a class that we don't need to stuck only on one or two names. For example if you see "FooManager" will you be able to know that this class synchronize content with a remote server? If you name your class "FooSynchronizer" any programmer that will read the name of the class will know what it does.
In conclusion try don't stuck only one XXXManager and XXService. The "Clean Code" book is a good starting point.

Read More

Code refactoring - Create base class/interface when is needed

When I made the last code review on a project I found the following lines of code:
Original version

public abstract class FooBase
{
  Person _person;
  public void string PersonId()
  {
    if(_person is Student)
    {
      return ((Student)_person).Id;
    }
    if(_person is Worker)
    {
      return ((Worker)_person).Id;
    }

    return _someDefaultValue;
  }
  public void string ScreenName()
  {
    if(_screen is MainScreen)
    {
      return ((MainScreen)_screen).Name;
    }
    if(_screen is SettingsScreen)
    {
      return ((SettingsScreen)_screen).Name;
    }

    ((DefaultScreen)_screen).Name ;
  }
}

After some new functionality was added:

public abstract class FooBase
{
    Person _person;
    public void string PersonId()
    {
        if(_person is Student)
        {
            return ((Student)_person).Id;
        }
        if(_person is Worker)
        {
            return ((Worker)_person).Id;
        }
        if(_person is Vampire)
        {
            return ((Vampire)_person).Id;
        }

        return _someDefaultValue;
    }
    public void string ScreenName()
    {
        if(_screen is MainScreen)
        {
            return ((MainScreen)_screen).Name;
        }
        if(_screen is SettingsScreen)
        {
            return ((SettingsScreen)_screen).Name;
        }
        if(_screen is TimeScreen)
        {
            return ((TimeScreen)_screen).Name;
        }

        ((DefaultScreen)_screen).Name;
    }
}

If you ask you’re self if the Person class contains the Id property, the response is not. The original team didn’t look over the code and add common items to the base class.
What we can observe in the above code?
First of all, the screens and persons could have a base class or at least a base interface.
The changes are made without trying to improve the code and design. Extracting a base class (interface) is a mandatory think to do before marking a task as done.
There are times when the developer don’t want to make changes to the code because is afraid that he can brake something. Maybe, if the code is covered with strong unit tests than the developer would feel more comfortable to make changes. If you are a developer and see that the code is not covered with test and because of this you cannot improve the design that you should begin to write some test first. After that you should refac. this methods.
After we make the refac our FooBase class should look something similar to this:

public abstract class FooBase
{
    // Add the Id property to the base class (Person)
    Person _person;
    public void string PersonId()
    {
        if(_person == null)
        {
            return _someDefaultValue;
        }
        return _person.Id
    }
    public void string ScreenName()
    {
    // Define a base Screen class that contains the Name property
    return _screen.Name;
    }
}

As a developer, don’t be afraid to improve the code. If the first team that implemented this class would made the refac. we would never had this problem. But in the same time, the second developer that made the changes should look over the code and try to improve it.

Read More

What Is Clean Code?

Reciteam "Clean Code" zilele astea și am dat peste definițiile date de către mai multi programatori a cea ce înseamna un cod curat. Mai jos găsiți aceste citate:

I like my code to be elegant and efficient. The logic should be straightforward to make it hard for bugs to hide, the dependencies minimal to ease maintenance, error handling complete according to an articulated strategy, and performance close to optimal so as not to tempt people to make the code messy with unprincipled optimizations. Clean code does one thing well.

Bjarne Stroustrup

Clean code is simple and direct. Clean codereads like well-written prose. Clean code neverobscures the designer’s intent but rather is fullof crisp abstractions and straightforward linesof control.

Grady Booch

Clean code can be read, and enhanced by adeveloper other than its original author. It hasunit and acceptance tests. It has meaningfulnames. It provides one way rather than manyways for doing one thing. It has minimal dependencies,which are explicitly defined, and providesa clear and minimal API. Code should beliterate since depending on the language, not allnecessary information can be expressed clearlyin code alone.

Dave Thomas

I could list all of the qualities that I notice inclean code, but there is one overarching qualitythat leads to all of them. Clean code alwayslooks like it was written by someone who cares.There is nothing obvious that you can do tomake it better. All of those things were thoughtabout by the code’s author, and if you try toimagine improvements, you’re led back towhere you are, sitting in appreciation of thecode someone left for you—code left by someonewho cares deeply about the craft.

Michael Feathers

In recent years I begin, and nearly end, with Beck’srules of simple code. In priority order, simple code:
• Runs all the tests;
• Contains no duplication;
• Expresses all the design ideas that are in thesystem;
• Minimizes the number of entities such as classes,methods, functions, and the like.
Of these, I focus mostly on duplication. When the same thing is done over and over,it’s a sign that there is an idea in our mind that is not well represented in the code. I try tofigure out what it is. Then I try to express that idea more clearly.

Ron Jeffries

You know you are working on clean code when eachroutine you read turns out to be pretty much whatyou expected. You can call it beautiful code whenthe code also makes it look like the language wasmade for the problem.

Ward Cunningham

Care este definiția voastră a "Clean Code"? Sau ce ar trebuii sa conțină un cod ca sa respecte aceasta definiție?

Read More