Refactor/sinch client entrypoint v2

[Dovchik]

No description provided.

[asein-sinch]

Watch out that the null-coalescing operator will not check for empty strings

[Dovchik]

done

[asein-sinch]

This implementation is different from the design that has been discussed in the confluence page referenced in the ticket description where it's written that the usage of the client will be the following:

var sinchClient = new SinchClient(new SinchClientConfig() {
    ProjectId = "id",
    Key = "key",
    Secret = "secret"
})

But now it's rather:

var sinchClient = new SinchClient(new SinchClientConfiguration() {
    SinchCommonCredentials = new SinchCommonCredentials()
    {
        ProjectId = "id",
        KeyId = "key",
        KeySecret = "secret"
    }
});

Can you clarify which one is right?

[Dovchik]

Grouping with SinchCommonCredentials is the correct one. We had agreed in the comments in the doc about the approach.
Updated the doc with correct usage

[JPPortier]

Comment from Numbers should also be applied here (#102 (comment))
(and across multiple files from PR even for Region variables if all configuration values could be empty strings.`

Why not sanitize them onto setter instead of having to duplicate validation onto each usage ?

[Dovchik]

Done adding validation.

Why not sanitize them onto setter instead of having to duplicate validation onto each usage ?

One place to validate and check data for me seems logical than to split sanitiazation and initialization.

[JPPortier]

Done adding validation

There is still other files with same pattern to be applied (e.g: https://github.com/sinch/sinch-sdk-dotnet/blob/refactor/sinch-client-entrypoint/src/Sinch/Conversation/SinchConversationConfiguration.cs)

One place to validate and check data

What do you mean; what is the difference between "validate" and "check" ?

logical than to split sanitiazation and initialization.

Seems to me it do not follow our SDKs principe and strategy:

Having the code be freely viewable by our users makes it possible for them to review/audit the underlying code for security and compliance reasons, and as such it encourages our development teams to write clean, legible, code.

So if we want to follow some clean code principles:

• do one thing: function should do one thing only but performing sanitization and business logic to resolve URL mean two.
• closed for modification: if sanitization need to be changed it will impact a business logic purpose only function
• don't repeat yourself: if sanitization need to be performed in the future for another context it will be spread across different function

So, having sanitization onto class init seems to leverage points above and then the class business logic related function can safely assume the "input" data contract satisfied

[Dovchik]

Okay, what do you suggest, throw InvalidArgumentException when setting Url Override to null or empty string?

[Dovchik]

do one thing: function should do one thing only but performing sanitization and business logic to resolve URL mean two.

The function Resolves URL based on input, that's one.

closed for modification: if sanitization need to be changed it will impact a business logic purpose only function

Only this function will be changed if any logic regarding how to handle URL resolution will be changed

don't repeat yourself: if sanitization need to be performed in the future for another context it will be spread across different function

there is IF. When this IF comes, I will think about how to refactor accordingly. Premature optimization.

I suggest we move this discussion about clean code to a more suitable place as it's a little out of scope.

[JPPortier]

there is IF. When this IF comes, I will think about how to refactor accordingly. Premature optimization.

This is "closed for modification" is related to 😃
But yes, no problem to talk about design; it was to highlight some points

[JPPortier]

Okay, what do you suggest, throw InvalidArgumentException when setting Url Override to null or empty string?

There is here some liberty, but have to be clearly documented for end user onto invalid values:

• emit an error log but fallback to default values; but end user could missed it if no 'proper' monitoring
• throw an exception assuming the end user won't have the expected configuration he was expecting

My preference is for 2nd solution:

• for URL content only: if user choose URL something related to some GDPR rules he won't have surprise to reach endpoint with different rules
• generally speaking: end user want to set a specific settings: if data is not valid, let the user be aware about a possible issue from his configuration.

Exception: for URLs we COULD imagine sanitize them by removing leading and trailing spaces automatically

[Dovchik]

Okay, makes sense
The logic of validation will be handled by Uri built in class: https://dotnetfiddle.net/2xiunp
Which means that coalesce operator is what's is working in this case. And string.IsNullOrEmpty() and coalesce ?? operator will work a little different.
The current implementation which I implemented, with Coalesce operator will do that:
if url override is empty string, or whitespaces, or invalid uri - an exception will be throw by Uri constuctor.
If url override is null, than const will be used.
Only correct values for UrlOverride will be null and valid uri