Skip to content
New issue

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

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

Already on GitHub? # to your account

Centralize constants #898

Closed
ChienNQuang opened this issue Jul 5, 2023 · 4 comments · Fixed by #911
Closed

Centralize constants #898

ChienNQuang opened this issue Jul 5, 2023 · 4 comments · Fixed by #911

Comments

@ChienNQuang
Copy link
Contributor

In the code, role "Administrator" and policy "CanPurge" to me are like magic strings, when it comes to modify one, we have to modify all instances. And how do I know how many roles and policies I have?
My suggestion is to centralize all constants in a place, may be src/Application/Common/Constants like this:

public abstract class Constants
{
   public abstract class Roles
   {
      public const string Administrator = "Administrator";
   }

   public abstract class Policies
   {
      public const string CanPurge = "CanPurge";
   }
}

The same goes for configuration strings in Infrastructure, but they can be addressed using Options pattern.

Any thoughts?

@ramax495
Copy link
Contributor

ramax495 commented Jul 5, 2023

I'm using enum in my projects:

namespace CleanArchitecture.Domain.Enums;

public enum UserRole
{
    Administrator,
    Customer,
    Manager,
}

And in command and query handlers:

[Authorize(Roles = nameof(UserRole.Administrator))]

@ChienNQuang
Copy link
Contributor Author

I'm using enum in my projects:

namespace CleanArchitecture.Domain.Enums;

public enum UserRole
{
    Administrator,
    Customer,
    Manager,
}

And in command and query handlers:

[Authorize(Roles = nameof(UserRole.Administrator))]

This is indeed one way to do it. I went with class because I sort of want the constants to be able to be centralized and nested into layers. And when you call them:
Constants.Role.Administrator

In my project I did the same for all exception and log messages. Centralize them all into one place and effectively reuse them.

@jasontaylordev
Copy link
Owner

Hey everyone. Thanks for the suggestions. Let's combine the approaches:

namespace CleanArchitecture.Domain.Constants;

public abstract class Roles
{
    public const string Administrator = nameof(Administrator);
}

public abstract class Policies
{
    public const string CanPurge = nameof(CanPurge);
}
[Authorize(Roles = Roles.Administrator)]

I think that removing the constants class and ensuring that the constants are within a CleanArchitecture.Domain.Constants namespace improves readability without losing context. The enum approach is nice, but using nameof reduces readability.

@ChienNQuang if you like the above approach, feel free to put together a PR and I'll push it through.

If not, let's keep the conversation going.

@ChienNQuang
Copy link
Contributor Author

Hey everyone. Thanks for the suggestions. Let's combine the approaches:

namespace CleanArchitecture.Domain.Constants;

public abstract class Roles
{
    public const string Administrator = nameof(Administrator);
}

public abstract class Policies
{
    public const string CanPurge = nameof(CanPurge);
}
[Authorize(Roles = Roles.Administrator)]

I think that removing the constants class and ensuring that the constants are within a CleanArchitecture.Domain.Constants namespace improves readability without losing context. The enum approach is nice, but using nameof reduces readability.

@ChienNQuang if you like the above approach, feel free to put together a PR and I'll push it through.

If not, let's keep the conversation going.

I personally would prefer the nested class way, due to my queries/commands and their handlers + result object being nested in one abstract class as well. But I'll use your approach to be consistent with the whole template.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants