This anti-pattern is named after the shape that most code makes when you have many conditionals, switch statements, or anything that allows the flow to take several paths. You’ll commonly see these nested within each other over and over, thus creating the arrow or triangle shape. This anti-pattern doesn’t always rely this arrow shape to be created by the nested blocks, but can have just as many paths through the code as it would if the arrow shape was replaced using guard clauses where applicable. The main problem we’re trying to solve by eliminating this, is the number of paths (cyclomatic complexity) that the code contains.
Take the following example (from http://c2.com/cgi/wiki?ArrowAntiPattern):
if get_resource
if get_resource
if get_resource
if get_resource
do something
free_resource
else
error_path
endif
free_resource
else
error_path
endif
free_resource
else
error_path
endif
free_resource
else
error_path
endif
Not the most elegant code in the world kind of a pain to mentally walk through, huh? Oftentimes I’ll put conditionals into guard clauses, this makes for a more readable codebase for starters. The following code is attempting to add a role to a user:
public void AddRole(Role role)
{
if (role != null)
{
if (!string.IsNullOrEmpty(role.Name))
{
if (!IsInRole(role))
{
roles.Add(role);
}
else
{
throw new InvalidOperationException("User is already in this role.");
}
}
else
{
throw new ArgumentException("role does not have a name", "role");
}
}
else
{
throw new ArgumentNullException("role");
}
}
Likely, you see the path that this is going down. We have all these checks, which aren’t bad by any means, to ensure that the role is appropriate to add for this user. If we refactor to use guard clauses, it’s nicer to read.
public void AddRole(Role role)
{
if (role == null)
throw new ArgumentNullException("role");
if (string.IsNullOrEmpty(role.Name))
throw new ArgumentException("role does not have a name", "role");
if (IsInRole(role))
throw new InvalidOperationException("User is already in this role.");
roles.Add(role);
}
That’s a lot better, but I think we can do more here. If we have access to something like a UserRoleValidation class, we’ll just use that.
public void AddRole(Role role)
{
userRoleValidator.Validate(role);
if (IsInRole(role))
throw new InvalidOperationException("User is already in this role.");
roles.Add(role);
}
I can find countless other ways to improve this sample code, but these simple tasks are good ways to get rid of that arrowhead that forms when several conditions all must be in tune for your code to work the way you expect it to. The real key is breaking your code apart into smaller pieces that do one thing, and do it well. When you see these “arrowheads” form, it’s likely because the code is trying to do too much.
[See more Anti-Patterns and Worst Practices]
Posted
May 27 2009, 08:00 AM
by
Chris Missal