This post is part of longer series about code smell and antipatterns in tests automation you can see more here. Today we will take a look at abuse of try catch.

Following example is not from test automation code, but actual application code. I have seen so many of similar cases in test automation to make it worth mentioning.

[csharp]
public override int Save()
{
  DoSomeLogic();
  try
  {
base.Save();
}
catch(Exception ex)
{
}
return base.Save();
}
[/csharp]

click arrows for an explanation of c# parts[expand]
override – is used in inheritance to cover parent method with your new implementation.
the base is also connected to inheritance. It calls parent implementation of given methods. [/expand]

Try Catch and ignore

The author wants to make operation, but he is aware that something can go wrong. So he put it in try catch block.
So far so good. Problem is nothing happens in that catch.
That is a huge problem. Here are few good reasons:
– There is an exception that is ignored; we don’t know what and why.
– We don’t know what the state operation is.
– it shows that developer seen some exception and didn’t know what to do.

There are cases when we need to catch and do nothing with the exception. For example due to flakiness. But leaving empty catch is wrong. At very least put there some logging information to know that something happens. If you can’t-do it – put a comment there explaining why are you putting the code in try with the empty catch.

Save twice

Authors way of thinking was if first save worked second will also work. [expand] that is self may not be safe bet since he knows the function is flacky[/expand]
But what happens when someone changes base save? Let say due to performance problems it won’t perform save when there are no changes? And it will return status „NoNewChanges”?

The Fix.

The goal of this series is small fixes without significant change in architecture. Doing proper fix on this one is little out of the scope for now. But that doesn’t mean we can’t-do some band-aid here.

Let’s remove unnecessary repetition.

[csharp]
public override int Save()
{
DoSomeLogic();
try
{
return base.Save();
}
catch(Exception ex)
{
return base.Save();
}
}
[/csharp]

See ? Now if first save succeeds we are done. And if second fails exception will be thrown anyway.

[csharp]
public override int Save()
{
DoSomeLogic();
try
{
return base.Save();
}
catch(SaveFailedException ex)
{
return base.Save();
}
}
[/csharp]

Can you spot the difference? Yup, we have dealt with catching all exception. This is also commonly referred as Pokemon Exception Handling (cause gonna catch them all). Of course, if you don’t know what errors throws save method you can’t-do it.

[csharp]
public override int Save()
{
DoSomeLogic();
try
{
return base.Save();
}
catch(SaveFailedException ex)
{
// due to xxxx and yyyy Save operation is a bit flacky
// if it fails first time we try to repeat it before throwing an
// error.
Logger.LogInfo($"Save for {object} failed retrying")
return base.Save();
}
}
[/csharp]

Ok, but the bright user can ask this question can we implement this in base save? Since it is flack, it will probably affect more than this class, and we will avoid needles code repetition.

Well if we can yes. But it is possible that base class is something to which we don’t have access. But in that case, i would suggest doing some wrapper class to isolate this issue there.

[csharp]
Class X : A
{
public override int Save()
{
DoSomeLogic();
return base.Save();
}
}

///<summary>
///Save operation in RootWeCantChange is flacky and can’t do anything with it
/// this is wrapper to give as some more isloation reting save before failing it.
///</summary>
Class A : RootWeCantChange
{
public override int Save()
{
try
{
return base.Save();
}
catch(SaveFailedException ex)
{
// due to xxxx and yyyy Save operation is a bit flacky
// if it fails first time we try to repeat it before throwing an
// error.
Logger.LogInfo($"Save for {object} failed retrying")
return base.Save();
}
}
}
[/csharp]

There is one more thing that bothers me int as result of Save operation. But that kind of architectural change is too big to address in this post.