Is it good practice to extract the exception throwing logic into your own method?
In our codebase, we repeat the following:
var isUpdateAllowed = await this.security.IsUpdateAllowedAsync(userId, postId);
if (!isUpdateAllowed)
{
throw new UnauthorizedException(string.Format(
"Not allowed to update post. User={0} Post={1}",
userId,
postId));
}
We've already had to change the exception type once, which is due to the change in many places, so repeating the above seems to violate DRY.
One suggested solution is that we extract the above validation into our own method. It can exist alongside an existing method IsUpdateAllowedAsync
. We can call it AssertUpdateAllowedAsync
. Then the calling site becomes:
await this.security.AssertUpdateAllowedAsync(userId, postId);
I'm personally torn: on the one hand, we get DRY, but on the other hand, I can't remember how many methods other than Assert.
MSTest use the above convention.
source to share
Anytime you find yourself copying code, it's a good idea to use it in a helper method. The biggest reason for this - as you've seen - refactoring becomes much smoother when the code only needs to be changed in one place, rather than many.
The reason you don't see Assert methods often is because frequent exception execution is done as part of the method, and availability checks such as permissions are done using the t20 check method. This is because StackTrace
throwing exceptions is costly due to the gathering of potentially useful debug information such as the current one - so it is most often undesirable in production code to throw exceptions other than an actual, honest and confirmed error, and even then it is mostly to tell you what to come back and fix.
source to share
Yes, that's a good idea.
The code that throws exceptions is no different from any other code in terms of reuse, in which case it certainly seems reasonable.
If you are not interested in a name, then how about ThrowIfUpdateNotAllowedAsync
or CheckUpdateAllowedAsync
, although I prefer the name you have already chosen.
The biggest criticism with refactoring this logic in a method is that you are using exceptions for control flow - while refactoring into a separate method is a simple, straightforward step in the right direction, I'll probably be looking to see if I can refactor to exclude this exception entirely.
source to share