Category Archives: Code Smell

In the previous part we looked over the MainWindow class and found some improvement points. Those were common mistakes with simple fix. We skipped a big switch block, because it needed more work, and today we are going to refactor that part. We will not refactor it to its final form, though, first, we just make it better. In a second blog post, we will improve the code more.
The code to be improved is the following:

switch (sourceType)
{
    case "bin":
        {
            var parser = new BinaryParser();
            parser.Open(file);

            while (!parser.HasReachedEnd)
            {
                persons.Add(parser.GetPerson());
            } 

            parser.Close();
        } 
        break;

    case "xml":
        {
            var parser = new XmlParser(file);
            parser.StartParse();

            Person parsedPerson;
            while ((parsedPerson = parser.GetNextPerson()) != null)
            {
                persons.Add(parsedPerson);
            }

            parser.FinishParse();
        }
        break;
}

When we look at this code, what do we notice? We have two ‘case’-s with very similar structure, basically they do the same:

• create an appropriate parser
• tell the parsers to prepare for parsing
• read the data until it is possible
• tell the parsers that they are not needed anymore.

What we see is that we have two classes with very similar functionality but with different interfaces. It is a code smell from the “Refactoring” book, called “Alternative Classes with Different Interfaces”. We have the ugly switch because the parsers have different interfaces, so we need two different code to use them. If they had the same interface, it wouldn’t be necessary to duplicate the code. Fortunately, we have the source code of the parsers, so we can refactor them to have a common interface.

The common interface

To create a common interface, we need to decide what operations we need. The first two steps in both cases are creating the parser then calling a method which moves the parser into a working state. These are the Open() and the StartParse() methods. It’s a little bit like a two phase initialization, even if the method name is not Init(), so we can ask that why we need two phase initialization? Why can’t we set up everything in the constructor, so by the time it returns, the parser is in a working state?

We can tell a few examples when we may need two phase initialization, for instance if the object needs a lot of resource, like memory, or it needs a network connection, then it makes sense to move the object into a working state just before we want to use it. Or maybe the object has several configuration settings, so first we create it, then we set it up, and then we call the open or connect or other method, which moves the object into a working state.

The XmlParser and BinaryParser don’t have settings, but they use files, so we could say that they use heavy resource. On the other hand, the typical use case of these classes is that we create them, immediately parse the whole data file with them, then we discard them. We don’t plan to create a parser upfront and use it later, so it doesn’t hurt if we set the parser into a working state in the constructor, even if it involves opening a file. So for now, we get rid of the Open()and StartParse() methods, making our interface simpler.

Besides the Open()-like methods, both parsers have Close() like methods, a Close() and a FinishParse(). It makes sense, because the parsers work with files, and files must be closed when we don’t need them anymore, so we need something. But the .NET framework has a way to express if we don’t want to use an object anymore. And this is the Dispose() method of the IDisposable interface. Some classes wrap Dispose() in a method with a different name, because of other conventions, for example we have been closing files and network connections for decades, so we expect that a File class has a close() method even in .Net. In other cases we have some sort of symmetry, like when we open something, then we want to close it, or if we connect, then we will disconnect.

In case of our parsers, we’ve just gotten rid of the open(), so we don’t need close() just for the symmetry, also it is not customary to close() or stop() or disconnect() a parser, so we are happy with Dispose(). Also, probably we will use these classes with the .net “using” keyword, which calls the dispose() for us, so we don’t really need anything else.

The third part we need is to be able to read the data. Both classes have a GetPerson()-like method, although they work in a different way. The XmlParser returns a null if there is no more Person to be read, with the BinaryParser we need to ask it whether it has more data or not. So they need different type of loop to read all the data, and we need to decide which one will we use.

Let’s say that we choose to use a HasReachedEnd or HasNext property. In this case we still need to handle when no more data is available and the GetNextPerson() method gets called. We could throw an exception, I think this would be the correct implementation, that we force the caller to check whether there is more data or not, and we don’t allow to call GetNextPerson(), when it doesn’t make sense.

The other option is that we return null when GetNextPerson() is called and no more data is available. Returning a null is a little bit ugly though, the user of our class needs to check this case, and if they don’t do it, usually the application doesn’t blow up immediately, it carries around the null reference and will throw an exception later. So usually it is not a good practice to return null, this is why many would like to see (Non) Nullable reference types in C#.

On the other hand its customary to return null when we cannot return real data. Many developers expect this behavior, it is a very common practice that we put a read() or next() in a ‘while’ loop, and check for null value there, so the null value will break the loop.

We are going to choose the way developers like to use classes like ours, even if it is less elegant, so we are going to return null if there is no more data. In this case we don’t need HasReacedEnd or HasNext property, those would just double the functionality of detecting the end of the data.

The interface we need becomes very simple. We agreed that we fully initialize the object in the constructor, so we don’t need init() or open(). We also agreed that we release the resources, like the file we read, using the IDisposable interface, so we don’t need close() or similar. The only thing we need is a GetNext() method.

interface IParser : IDisposable
{
    Person GetNext();
}

One may call the GetNext() method GetNextPerson() or NextPerson(), but it is not needed to use the “Person” word in the method name. The only thing we can return is a Person instance, so it is obvious that the Next thing we are requesting is a Person. GetNext() or Next() is perfectly fine.

The IParser name maybe too general, so it wouldn’t be good in a big application, but in our small program it works well, it is clear what parsers we are talking about.

Refactoring the parsers

The refactored parsers look like the following:

internal class XmlParser : IParser
{
    private static readonly string ItemTitle = "Person";
 
    private string path;
    private XmlReader xmlReader;
    private XmlSerializer xmlSerializer;
 
    public XmlParser(string path)
    {
        this.path = path;
 
        this.xmlReader = XmlReader.Create(this.path);
        this.xmlReader.MoveToContent();
        this.xmlReader.ReadToDescendant(XmlParser.ItemTitle);
 
        this.xmlSerializer = new XmlSerializer(typeof(Person));
    }
 
    public void Dispose()
    {
        this.xmlReader.Close();
    }
 
    public Person GetNext()
    {
        return this.xmlSerializer.Deserialize(this.xmlReader) as Person;
    }
}

The binary parser is a bit more complex:

internal class BinaryParser : IParser
{
    private FileStream fileStream;
    private BinaryReader binaryReader;
 
    internal BinaryParser(string path)
    {
        this.fileStream = File.OpenRead(path);
        this.binaryReader = new BinaryReader(fileStream);
    }
 
    public void Dispose()
    {
        this.binaryReader.Close();
    }
 
    public Person GetNext()
    {
        if (this.HasReachedEnd)
        {
            return null;
        }
 
        return GetPerson();
    }
 
    private Person GetPerson()
    {
        var result = new Person
        {
            LikedPersons = new List<string>()
        };
 
        result.Name = this.binaryReader.ReadString();
 
        var contactCount = this.binaryReader.ReadInt32();
        for (var i = 0; i < contactCount; i++)
        {
            result.LikedPersons.Add(
                this.binaryReader.ReadString());
        }
 
        return result;
    }
 
    private bool HasReachedEnd
    {
        get
        {
            return this.binaryReader.PeekChar() < 0;
        }
    }
}

The two parsers are not complete yet, for example if we try to read a disposed instance, we will get error from the lower layer.

Refactoring the switch

As a first step, we can transform the switch to the following form:

var filesToProcess = this.GetFileList();
var sourceType = this.GetSelectedSourceType();
 
var persons = new List<Person>();
 
foreach (var file in filesToProcess)
{
    IParser parser;
 
    switch (sourceType)
    {
        case "bin": parser = new BinaryParser(file); break;
        case "xml": parser = new XmlParser(file); break;
 
        default:
            throw new NotSupportedException(
                string.Format("Not supported format: {0}", sourceType));
    }
 
    using (parser)
    {
        Person person;
 
        while ((person = parser.GetNext()) != null)
        {
            persons.Add(person);
        }
    }
}

This code looks much better than the original, but we still have that switch. The role of the switch is clear however, its only purpose is that according to a type information, it creates an appropriate parser. It may sound familiar, the Parameterized Factory Method, which is a variant of the Factory Method Design Pattern, does the same. If we have a factory method, we can write the following:

foreach (var file in filesToProcess)
{
    using (var parser = ParserFactory.Create(file, sourceType))
    {
        Person person;
 
        while ((person = parser.GetNext()) != null)
        {
            persons.Add(person);
        }
    }
}

The switch seems to be disappeared, but actually that switch just moved to the implementation of the factory method:

internal static class ParserFactory
{
    internal static IParser Create(string path, string sourceType)
    {
        switch (sourceType)
        {
            case "bin": return new BinaryParser(path);
            case "xml": return new XmlParser(path);
 
            default:
                throw new NotSupportedException(
                    string.Format("Not supported format: {0}", sourceType));
        }
    }
}

In the next part…

The code after the refactor looks quite good, but it can be even better. Its hard to notice the issue in this solution because it is so ubiquitous, that we have gotten used to it and it looks normal. But the parsers violate the Single Responsibility design principle, because they do two things. They open the data source, and process the data. These two things can change independently. Now the datasource is always file, later it could be a network source.
Today we have two parsers, soon we are going to create a third one, and if we need to switch from file to network, we need to make the same modification at three different places. This shows that something is not right with the responsibilities. So in the next post, we are going to improve the code further.

In this part we are going to take a look at the source code and try to answer the question that usually sounds like “what problems can you see at first look” or “what do you dislike when you look at this code” or “how would you change the code to make it better”.

The keyword here that many interviewers like to hear is “code smell”. I won’t go into what code smell is, because almost everybody knows it, or if not, lots of information is available online. A google search or wikipedia helps to understand the concept.

One file with everything in it

We saw earlier that most of the important code is in the MainWindow.cs. Now, it’s a code smell by itself, a single source file has many important parts – clearly a violation of the Separation of Concerns design principle – another keyword they like to hear on an interview. If somebody doesn’t know the concept, again, wikipedia helps to start.

Even if we do not know WPF, we can understand that this source, this class called MainWindow, is something related to the user interface, it belongs to the presentation, and still the next few lines work something with the file system and DatabasePath, which is something about persistence. Also we can find some calculation in the middle of the source, which is business logic.
So everything seems to be mixed into a user interface code, and this architectural style actually has a name, it’s called Smart UI. Today Smart UI is considered an anti-pattern – another good word for an interview. Once we tell the interviewer our concerns about the architecture of the application, we can be sure that they will ask back that what other styles we know. So its worth to review some architectural styles, to be able to talk about layers and tiers, clients and servers, and maybe services and message bus-es. A good starting point is this website.

Method order

There is an other problem with GetFileList(). It doesn’t seem to be an important method, at least it doesn’t show much about the purpose of the MainWindow class. Still this is the very first method we can see.

Different developers like to order the methods within a class in a different way, and there is no best way to do this. But in general, the order of the methods should help other developers to understand the purpose of the class. And what helps to understand is the higher level logic that we can call on a class. So it is worth to put the public methods first – these are the methods we can call on a class – and those which implement the higher level logic.

Big methods

The MainWindow class doesn’t have public methods and doesn’t really have higher level logic, almost everything is implemented in a single method. This is an other problem, the button_click() method is too big. Again, different developers have different idea about how long a method should be at maximum, but without any number, we can tell that this method is too big, just because it does too many things.

A sign that this method does too many things is that it has section comments. A section comment is a code smell, it shows the beginning of a new logical block, which could be moved into a separate method:

Another way to find parts to move into separate methods if we look for different abstraction levels.
From the three abstraction levels we can make three methods:

static IEnumerable<PersonNameWithLikeCount> 
CalculateLikeCountForEach(IEnumerable<Person> persons)
{
    var counts = new List<PersonNameWithLikeCount>();
 
    foreach (var person in persons)
    {
        var count = CalculateLikeCountFor(person, candidates: persons);
 
        counts.Add(
            new PersonNameWithLikeCount()
            {
                Name = person.Name,
                LikeCount = count
            });
    } 
 
    return counts;
} 
 
static int CalculateLikeCountFor(Person person, IEnumerable<Person> candidates)
{
    var count = 0;
 
    foreach (var candidate in candidates)
    {
        if (CandidateLikesPerson(candidate, person))
        {
            count++;
        } 
    } 
 
    return count;
} 
 
static bool CandidateLikesPerson(Person candidate, Person person)
{
    return candidate.LikedPersons.Contains(person.Name);
} 

It is not always easier to understand a code when it is structured using several small methods. Many developers try to put the little pieces back to its place in their minds. But small methods still have benefits.

Look at the second ‘foreach’ in the original code. If we want to understand what it does, we need to understand every individual line one by one, even if we wanted to understand only the big picture. When it has its own method, and it has a right name, we can see that it “CalculateLikeCountFor” a given person with given candidates. Or look at the ‘if’ statement in the middle of the original method. It is not impossible to understand, but the refactored code has “if (CandidateLikesPerson…”. It looks almost like an English sentence.

A typical problem with big methods is that their logical parts start to get intermingled. The parts start sharing variables, program lines sneak in blocks where they don’t belong to. The figure below shows logical parts with colors, and the structure of a big method:

If we need to change a method like this, sometimes it is hard to predict how a change at the top will affect the rest of the code. Also, if we want to move a block out, like the pink on the figure, it is hard not to break the green code, or bring something unnecessary with pink.

When we have separate small methods, that gives a good boundary to the code, they cannot be intermingled. Also, it is easier to see how the blocks communicate to each other, because we can see it on the method parameters.

It is easier to reuse small methods, and also easier to test them. In the original code, if we want to test if the algorithm calculates the right ‘like count’ for a given person, we need to drive the control through the first ‘foreach’. There are several lines there in which we are not interested but we need to understand how they use the parameters, so by the time the control reaches the second ‘foreach’ it will have the right data to test with. Also, the return value is in a different form than we need. We need only a number, not a list of something, and it makes checking the result of the test harder.

The switch

This is the worst part of the code. The ‘switch’ has two almost identical ‘case’-es. We will fix this part in a separate post.

Magic Numbers

At the end of MainWindow.cs we can see the following:

This 10 here is a so called magic number. It is very typical and there are different types of magic numbers. Sometimes we do not know what gives this value. In other cases it is clear what the number means, but the same number is scattered through the code. If we want to modify it, we need to do it on several places. It is easy to miss one, or accidentally modify something with the same value but different meaning. This 10 is the same type, we know that it is ten because we want to see the 10 most liked people on the screen. But if we want to see 15, we don’t know that it is enough to modify the number here, or we need to modify it somewhere else, too.

Catch everything

At the end of the code, we can see an exception handler which catches everything but then does nothing. It may seem a good idea, this hides ugly error messages from the user and prevents crushing the application. On the other hand, what the user expects – finish an operation – will not happen, and no feedback about it. It can be really confusing. Also it is possible that the program gets into an inconsistent state with inconsistent data, and further operations and user actions may cause even more serious trouble.

An important rule of error handling is that we should catch only those errors we can handle somehow. Handling doesn’t mean to solve the problem, usually we cannot solve problems like permission problems or network problems from code. Handling means that we reset the program into a consistent state, we clean things, we roll back half changes, and then let the program run like nothing bad happened. If we cannot ensure a consistent state after an error, we must let the exception bubbling up, maybe a higher level can handle it. Or if not, letting the program die is usually better than letting it run in an inconsistent state, where it may become unpredictable.

Sometimes we catch exceptions even if we cannot handle them, because we want to add more details to the error. For example if the application has no permission to read a file, the Unauthorized Access Exception itself will not help a lot to understand the problem. On a higher level of the code, when we know that what operation was about to execute, we can catch this exception and re-throw a more useful exception.

It is also a problem that the try block includes different operations, and no way that a single exception handler could reset from every possible error. It is not even possible to write a good log entry, because we don’t know what operation caused the problem.

In the next part we are going to eliminate the ‘switch’ block.