Azure-Samples / Azure-Samples/ms-identity-ciam-dotnet-tutorial

Consider removing the Exception<TException> helper class

Open
#149 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
PowerShell
Stars
48
Forks
35
PR merge metrics
No merged PRs in 30d

Description

Consider removing the `Exception` helper class from the ms-identity-ciam-dotnet-tutorial.

``` c#
public static class Exception where TException : Exception, new()
{
public static void ThrowOn(Func predicate, string message = null)
{
if (predicate())
{
TException toThrow = Activator.CreateInstance(typeof(TException), message) as TException;
throw toThrow;
}
}
}
```

This helper class aims to simplify pre-condition checking, but actually makes things worse, because it complicates the sample project with noise and distracts the developer from the task at hand, which is to learn how to integrate this code into their own project.

Especially for such a sample project it's best to keep things as simple as possible and stick with common practices. In this case the common practice is to do the checking in the calling method itself rather than passing the condition to a helper method. For instance, instead of doing this:

``` c#
Exception.ThrowOn(() => this.PublicClientApplication == null, PCANotInitializedExceptionMessage);
```

This is easier to follow, especially (again) in the context of such sample project:

``` c#
if (this.PublicClientApplication == null) throw new ArgumentException(PCANotInitializedExceptionMessage);
```

Besides complicating the sample code base, this piece of code is riddled with problems, such as:

* The code will throw an exception in case the TException type does not contain a ctor with exactly one argument of type string. This will cause a completely different exception (thrown by `Activator.CreateInstance`) to be thrown, losing the original exception information. There are several exception types in the BCL that don't have a `ctor(string)` constructor and custom exceptions are easily written that lack such ctor.
* While the `ThrowOn` method contains a `new()` constraint, the code itself creates an exception via its `ctor(string)` constructor, not the default constructor.
* Not all exception classes that contain a ctor with a single string argument actually expect the exception message to be passed. Calling `Exception.ThrowOn` for instance, will cause an exception to be thrown where the message becomes the `paramName`.

If desired I can create a pull request with this change.

Contributor guide

No contributing guide indexed for this repository

Research direction

Search the repository for the Exception helper and its ThrowOn usages, then read each calling method to understand the existing checks. Replace those usages with direct exception checks and remove the helper; done means the sample builds and retains the same precondition behavior without the helper.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.