Tag: readability

What Makes Good Code? – Should Every Class Have An Interface? Pt 1

What Makes Good Code? - Should Every Class Have An Interface?

What’s An Interface?

I mentioned in the first post of this series that I’ll likely be referring to C# in most of these posts. I think the concept of an interface in C# extends to other languages–sometimes by a different name–so the discussion here may still be applicable. Some examples in C++, Javaand Python to get you going for comparisons.

From MSDN:

An interface contains definitions for a group of related functionalities that a class or a struct can implement.
By using interfaces, you can, for example, include behavior from multiple sources in a class. That capability is important in C# because the language doesn’t support multiple inheritance of classes. In addition, you must use an interface if you want to simulate inheritance for structs, because they can’t actually inherit from another struct or class.

It’s also important to note that an interface decouples the definition of something from its implementation. Decoupled code is, in general, something that programmers are always after. If we refer back to the points I defined for what makes good code (again, in my opinion), we can see how interfaces should help with that.

  • Extensibility: Referring to interfaces in code instead of concrete classes allows a developer to swap out the implementation easier (i.e. extend support for different data providers in your data layer). They provide a specification to be met should a developer want to extend the code base with new concrete implementations.
  • Maintainability: Interfaces make refactoring an easier job (when the interface signature doesn’t have to change). A developer can get the flexibility of modifying the implementation that already exists or creating a new one provided that it meets the interface.
  • Testability: Referring to interfaces in code instead of concrete classes allows mocking frameworks to leverage mocked objects so that true unit tests are easier to write.
  • Readability: I’m neutral on this. I don’t think interfaces are overly helpful for making code more readable, but I don’t think they inherently make code harder to read.

I’m only trying to focus on some of the pro’s here, and we’ll use this sub-series to explore if these hold true across the board. So… should every class have a backing interface?

An Example

Let’s walk through a little example. In this example, we’ll look at an object that “does stuff”, but it requires something that can do a string lookup to “do stuff” with. We’ll look at how using an interface can make this type of code extensible!

First, here is our interface that we’ll use for looking up strings:

public interface IStringLookup
{
    string GetString(string name);
}

And here is our first implementation of something that can lookup strings for us. It’ll just lookup an XML node and pull a value from it. (How it actually does this stuff isn’t really important for the example, which is why I’m glossing over it):

public sealed class XmlStringLookup : IStringLookup
{
    private readonly XmlDocument _xmlDocument;

    public XmlStringLookup(XmlDocument xmlDocument)
    {
        _xmlDocument = xmlDocument;
    }

    public string GetString(string name)
    {
        return _xmlDocument
            .GetElementsByTagName(name)
            .Cast<XmlElement>()
            .First()
            .Value;
    }
}

This will be used to plug into the rest of the code:

private static int Main(string[] args)
{
    var obj = CreateObj();
    var stringLookup = CreateStringLookup();
    
    obj.DoStuff(stringLookup);
 
    return 0;
}
 
private static IMyObject CreateObj()
{
    return new MyObject();
}
 
private static IStringLookup CreateStringLookup()
{
    return new XmlStringLookup(new XmlDocument());
}
 
public interface IMyObject
{
    void DoStuff(IStringLookup stringLookup);
}
 
public class MyObject : IMyObject
{
    public void DoStuff(IStringLookup stringLookup)
    {
        var theFancyString = stringLookup.GetString("FancyString");
        
        // TODO: do stuff with this string
    }
}

In the code snippet above, you’ll see our Main() method creating an instance of “MyObject” which is the thing that’s going to “DoStuff” with our XML string lookup. The important thing to note is that the DoStuff method takes in the interface IStringLookup that our XML class implements.

Now, XML string lookups are great, but let’s show why interfaces make this code extensible. Let’s swap out an XML lookup for an overly simplified CSV string lookup! Here’s the implementation:

public sealed class CsvStringLookup : IStringLookup
{
    private readonly StreamReader _reader;
 
    public CsvStringLookup(StreamReader reader)
    {
        _reader = reader;
    }
 
    public string GetString(string name)
    {
        string line;
        while ((line = _reader.ReadLine()) != null)
        {
            var split = line.Split(',');
            if (split[0] != name)
            {
                continue;
            }
 
            return split[1];
        }
 
        throw new InvalidOperationException("Not found.");
    }
}

Now to leverage this class, we only need to modify ONE line of code from the original posting! Just modify CreateStringLookup() to be:

private static IStringLookup CreateStringLookup()
{
    return new CsvStringLookup(new StreamReader(File.OpenRead(@"pathtosomefile.txt")));
}

And voila! We’ve been able to extend our code to use a COMPLETELY different implementation of a string lookup with relatively no code change. You could make the argument that if you needed to modify the implementation for a buggy class that as long as you were adhering to the interface, you wouldn’t need to modify much surrounding code (just like this example). This would be a point towards improved maintainability in code.

“But wait!” you shout, “I could have done the EXACT same thing with an abstract class instead of the IStringLookup interface you big dummy! Interfaces are garbage!”

And you wouldn’t be wrong about the abstract class part! It’s totally true that IStringLookup could instead have been an abstract class like StringLookupBase (or something…) and the benefits would still apply! That’s a really interesting point, so let’s keep that in mind as we continue on through out this whole series. The little lesson here? It’s not the interface that gives us this bonus, it’s the API boundary and level of abstraction we introduced (something that does string lookups). Both an interface and abstract class happen to help us a lot here.

Continue to Part 2


Code Smells – Issue Number 3

Code Smells – Issue Number 3 (Image by http://www.sxc.hu/)

Code Smells

Welcome to the third edition of Code Smells! Periodically I’ll be posting about how to detect code smells and what they mean in terms of the big picture of your code. The previous installment can be found right here.

What’s a code smell? Wikipedia says it perfectly:

In computer programming, code smell is any symptom in the source code of a program that possibly indicates a deeper problem. Code smells are usually not bugs—they are not technically incorrect and don’t currently prevent the program from functioning. Instead, they indicate weaknesses in design that may be slowing down development or increasing the risk of bugs or failures in the future.

These code smells are often based on my own opinion and experience with programming. If you disagree with what I’m saying in my post, please don’t hesitate to post a comment. I’d love to clarify anything I may have worded poorly and discuss your perspective–especially if you have a completely different take on things!

The Stink List

Code Smell #7: Using exceptions to control logical flow. This is a pretty nasty path to get into and a bad code smell to stumble upon. Luckily, it’s generally relatively easy to improve. Using exception handling to control logical flow is, in general, misleading. It’s relying on a mechanism used to catch unexpected errors in order to direct the flow of your program. Often times we can use things like if statements to check for these conditions before throwing exceptions.

A common scenario where I see this is parsing. I’ll illustrate this code smell with a little C# example:


var input = "This is not a number";
try
{
  var parsed = int.Parse(input);
  DoSuccessfulStuff(parsed);
}
catch (Exception)
{
  DoFailStuff();
}

It seems a bit contrived, but I’ve seen lots of code written like this–and you know what? This works. It gets the job done. However, there are other mechanisms built-in to .NET that let us do parsing a little bit nicer:


var input = "This is not a number";
int parsed;

if (int.TryParse(input, out parsed))
{
  DoSuccessfulStuff(parsed);
}
else
{
  DoFailStuff();
}

The second set of code doesn’t need to catch exceptions to know that the parsing wasn’t successful. It may not be obvious from this example but throwing and catching exceptions can be quite expensive compared to a few logical sanity checks instead (and before getting into a debate on this, just know the impact it has on your program. I’ve had things go from taking several seconds to multiple minutes, but there are certainly cases where performance will be negligible).

The additional kicker in my contrived example is using the base Exception class to create what a colleague of mine refers to as Pokemon Exception Handlers. Thus, even if you didn’t want to restructure your code, using a specific exception type would:

  • Indicate to other programmers what you’re trying to accomplish
  • Not swallow other potential problems and have them go unseen

Now, this code smell isn’t always possible to avoid entirely. If you’re interfacing with third party components, sometimes you do have to rely on catching exceptions that you can’t otherwise check for ahead of time. If you don’t have the code, you can’t know for every path how/when/why exceptions will be thrown. The same thing could be said when running within an environment where state cannot be guaranteed. Sometimes it’s just necessary. In this case, I would suggest that instead of using Pokemon Exception Handlers, you try to catch the specific exceptions you know you need to watch out for.

Takeaway:

  • If a simple logic check can be used instead of throwing/catching exceptions, it’s likely a better bet.
  • Try to avoid exception handlers that catch all exceptions. Something nasty might sneak by as a result of it.
  • Interfacing with some code or working working in certain environments means you have to rely on exception logic. Take a deep breath and move on.

Code Smell #8: Having an object hierarchy that requires many very light weight classes. Object oriented programming and how object hierarchies are structured are pretty complicated topics of discussion, so I’m not about to try and over simplify it with discussion of this code smell. This is mostly something I’ve gathered from my own programming experience, so I’ll try to illustrate with examples that parallel things I would have come across.

When I’m building an object hierarchy, sometimes it’s not really apparent just how big and complicated it might get. I might start off with a base class and two child classes of it. Over time, the top three levels in my hierarchy have all turned into some sort of class abstraction, and I don’t hit concrete implementations until a few levels down hte hierarchy. Not a big deal–Sometimes it’s just hard to tell where things will go. When things get to the point where in order to introduce a new class and functionality all I need to do is inherit a class and override a single property or method, that’s a bit of a red flag for me.

On the surface, this seems pretty cool. The hierarchy is apparently solidified enough that extending it is really simple if all I need is a single property or method replacement. So why is this a code smell?

In my opinion, it has to do with the duplication of code. If I end up having many child classes (where child in this case represents the child-most class of my mature object hierarchy) that differ only by a single property or method, then I should look at how these classes are being constructed. I wrote recently about how I used lambda expressions to refactor similar code with classes that differed by a single method. My solution, in this case, was to examine the factory that created my classes. Instead of having 10’s of different child classes with a bunch of boilerplate code, I had one factory that could specify the code that differentiated each class.

The benefit of this?

  • Keep class hierarchies from ballooning our of control. Changing an API down the road can mean making changes in many spots.
  • Reduce duplication of boilerplate code. This might be the code required to define a simple constructor or override a getter property.

This is only one small example, but if you get into this situation in your class hierarchy, I’d recommend investigating to see if you can refactor in a similar approach. Maybe your class hierarchy is incredibly mature and isn’t changing much. If that’s the case, you may not even want to touch it. So be it. If you’re still actively adding classes to your hierarchy, it may be better to try analyzing it sooner rather than later.

Takeaway:

  • Object oriented design is a complex topic of discussion.
  • There’s no one perfect way to make your class hierarchies.
  • Having many child classes that differ by a property/method or two may be worth checking for refactoring opportunities.

Summary

I hope you enjoyed this issue of Code Smells. As always, it’d be great to open the floor to discussion. I don’t believe in absolutes, so identifying code smells is not meant to be me preaching some made up laws of programming. Let me know your thoughts on these code smells or share code smells of your own!


How and Why to Avoid Excessive Nesting

Background

This probably sounds really nit-picky or OCD, but I think it’s an issue worth addressing. Excessive nesting of logic within code can make things nightmarish to read. Even a few of years ago I never thought anything of this. I mean, how much could it really affect someone reading it? He/she must be a complete newb to not be able to read my logic. Fast forward to a co-op placement where this was more closely moderated by my managers, and I began to pay more attention to it…

Why?

Alright, so all that you know so far about my opinion on this is that excessive nesting bothers me. So far, my mission is accomplished. Everything else is just extra. The first issue with excessive nesting is that it actually makes logic hard to follow. If you’re doing code reviews or revisiting your old code, large methods that have lots of nested if statements and loops actually become a tangled mess of logical workflows. You don’t need to believe me yet, but I’m hoping by the end of this you might change your mind.

The next thing, and it’s related, is that it makes refactoring code quite tricky. If you have lot’s of deeply nested if statements, switching up the behaviour of a function even a little bit could have your mind warping with how to tackle all the logical branches. Have fun. Remember that one monolithic function that nobody wanted to go back and refactor? Well, it turns out you need to pass in another parameter now and handle it in all of your separate logical paths. Hold back the tears when you’re trying to recall the logic once you’re 10+ levels deep into nested if statements.

Another key point I’d like to mention is that, in my opinion, the larger the vertical separation between a conditional check and it’s bodies (i.e. the if block and the else block) the more difficult it becomes to read the code. Of course, this may not be a law or an all-the-time thing, but it’s certainly a decent guideline. Think about it though. If you have an enormous block of code for your if statement body, by the time you finish understanding that, you have to go back up to the if statement condition and invert the whole thing to beign to understand what your else block does.

The Offender

Let’s have a look at some real offensive code. Who knows what it does really… Well, nobody does. Why? Because it’s completely contrived to illustrate my point. And that’s that. Behold the horror!

        private void DoStuff()
        {
            foreach (thing in thisList)
            {
                if (condition1)
                {
                    if (condition2)
                    {
                        DoThis(thing);
                    }
                    else
                    {
                        if (condition3)
                        {
                            continue;
                        }
                        else
                        {
                            if (condition4)
                            {
                                continue;
                            }
                            else
                            {
                                if (condition5)
                                {
                                    if (condition6)
                                    {
                                        continue;
                                    }
                                    else
                                    {
                                        if (condition7)
                                        {
                                            continue;
                                        }
                                        else
                                        {
                                            if (condition8)
                                            {
                                                DoThis(thing);
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
                else
                {
                    DoThis(thing);
                }
            }
        }

Pretty filthy, right? In all honesty, anyone who has worked in production code is guaranteed to have seen code that nests much much deeper… veering right off the developer’s window (and some of us code with multiple monitors). It’s a scary world out there, and this example doesn’t even begin to illustrate how bad it can get. I mean, this particular example actually fit on my narrow blog window.

 

A Better Way?

Well, fixing this type of thing is nice and easy:

        private void DoStuff()
        {
            foreach (thing in thisList)
            {
                if (!condition1)
                {
                    DoThis(thing);
                    continue;
                }

                if (condition2)
                {
                    DoThis(thing);
                    continue;
                }
                
                if (condition3 || condition4 || !condition5 || condition6 || condition7)
                {
                    continue;
                }
                
                if (condition8)
                {
                    DoThis(thing);
                }
            }
        }

That’s so much prettier. So what’d I do there? A handful of techniques:

  • Invert logical blocks if they can reduce your nesting. For condition1, I had an if/else block where DoThis(thing) resided in the bottom else block… farrrrr farrrr away from the check itself. I simply inverted this check and moved the else block up. Of course, I then had to put a continue statement there to go back up to the next iteration.
  • For condition2, by simply placing a continue right after the method call in the body, I was able to completely eliminate the else block and reduce nesting by a whole level. This works well with if/else blocks with returns too.
  • Next up was a whole pile of combinations for checking when I’m not going to be calling DoThis(thing). That reduced nesting by a bajillion levels, approximately.
  • The final block there for condition8 was still necessary. Of course, it could have be written to be the inverse check (so, if NOT condition8) with a continue inside the block, followed by DoThis(thing) outside of the if block. To me this would have been a bit overkill.

 

Did You Catch That?

Something extremely important to remember when changing logical flows like this is that the order you check your conditions is EXTREMELY important. Notice how in my refactored version the condition checks are still in the same order that they originally appeared? This is on purpose.
Consider if I move condition8 up to the if statement that tests condition1 and say if NOT condition1, OR condition8. Now this is technically not equivalent to the initial implementation. Why? Because the initial implementation says that for one of the logical paths that call DoThis(thing) the following must be met:
  • condition1 = true
  • conditon2 = false
  • conditon3 = false
  • condition4 = false
  • condition5 = true
  • conditon6 = false
  • condition7 = false
  • condition8 = true

Thus, by combining the condition8 check with the condition1 check, how have I guaranteed all those other conditions?  Additionally, how do I know that skipping those condition checks (i.e. pretend they are method calls) has not altered state elsewhere in the class? This optimization actually may not make the code incorrect in certain situations (because it really depends what those conditions are), but it’s important to note that the checks would not be equivalent to the original. It’s just something to pay attention to, but who knows, you may even find that you can optimize some of those checks away depending on your situation!

 

Summary

Don’t excessively nest your code because it makes me cry at night.

  • Nick Cosentino

    Nick Cosentino

    I work as a team lead of software engineering at Magnet Forensics (http://www.magnetforensics.com). I'm into powerlifting, bodybuilding, and blogging about leadership/development topics over at http://www.devleader.ca.

    Verified Services

    View Full Profile →

  • Copyright © 1996-2010 Dev Leader. All rights reserved.
    Jarrah theme by Templates Next | Powered by WordPress