Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a new Helper class #65

Merged
merged 3 commits into from
Nov 30, 2021
Merged

Added a new Helper class #65

merged 3 commits into from
Nov 30, 2021

Conversation

MrDave1999
Copy link
Contributor

@MrDave1999 MrDave1999 commented Nov 16, 2021

With this new functionality the value of the environment variable can be retrieved through the indexer:

var reader = new EnvReader();
var key1 = reader["KEY1"];
var key2 = reader["KEY2"];

The advantage of this is that you can use dependency injection, either manually or by using a service container such as Microsoft.Extensions.DependencyInjection.

For example:

class ExampleController
{
    private IEnvReader reader;
	
    public ExampleController(IEnvReader reader)
    {
         this.reader = reader;
    }
	
    public void MethodExample()
    {
         var key1 = reader["KEY1"];
         var key2 = reader["KEY2"];
    }
}

The IEnvReader interface also has some auxiliary methods to retrieve the value of an environment variable in a specific format (integer, double, etc.) and warns the client when it does not find the variable. It is true that currently the library already has this functionality but it does not warn the client when it does not find an environment variable.

Methods that do not start with Try (as GetBoolValue, etc.) throw an exception when the environment variable is not found. Methods that start with Try (as TryGetBoolValue), however, return false if the variable is not found, so the client has two options for handling the error.

@MrDave1999
Copy link
Contributor Author

@rogusdev Could you check this PR? Thanks.

@rogusdev
Copy link
Collaborator

I see where you are going with this, and why. Honestly, I am a bit concerned about introducing something so different from how env vars are normally collected. Generally my personal recommendation is to have a class that collects all such values that you need in your application, and pass a version of that around as a mock/fake, instead of mocking out the env vars functionality of the language itself. That said, I can understand. So, my main questions is: are you sure about the index access? It seems like the get methods are the real way people should access such things.

@MrDave1999
Copy link
Contributor Author

@rogusdev The indexer is just a simpler way to access the environment variable. It would be interesting that the library has a feature like this, it's like in PHP for example, you access the environment variables in this way: $_ENV['KEY1'];.

{
get
{
if (instance == null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not thread safe. You should read up on how to make a singleton thread safe, if you really must have a singleton at all -- I strongly discourage it, personally.

Copy link
Contributor Author

@MrDave1999 MrDave1999 Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rogusdev To avoid complications, I have removed the property.

@MrDave1999 MrDave1999 requested a review from rogusdev November 29, 2021 00:43
@rogusdev rogusdev merged commit 0914ce9 into tonerdo:master Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants