0
\$\begingroup\$

I was rewriting some code from my colleague and I was questioning if I was improving the code at all.

My take at the code:

if (account == null || (!account.Authorizations.Any(x =>
    (x.Role.Name == RoleNames.ADMIN ||
    x.Role.Name == RoleNames.SENDER ||
    x.Role.Name == RoleNames.SENDERSADMIN) &&
    x.TopicFilter.Name.ToLower() == item.Topic.Name.ToLower()) &&
    !isCurrentUser))
{
    return StatusCode(403);
}

Versus the original code:

if (account == null)
    return StatusCode(403);

bool isAuthorized = account.Authorizations.Any(x =>
    (x.Role.Name == RoleNames.ADMIN || x.Role.Name == RoleNames.SENDER || x.Role.Name == RoleNames.SENDERSADMIN) &&
    x.TopicFilter.Name.ToLower() == item.Topic.Name.ToLower());

bool isCurrentUser = message.Author.Username.ToLower() == UID.ToLower();

if (!isAuthorized && !isCurrentUser)
    return StatusCode(403);

Even though the big linq query for "isAuthorized" is still rather bulky and there are two IF-statements with the same result, the code is still a lot more readable.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question? for examples, and revise the title accordingly. \$\endgroup\$
    – Martin R
    Commented May 19, 2020 at 15:01

3 Answers 3

5
\$\begingroup\$

I would argue that neither version is readable enough. Extract 'til you drop. I would say in the grand scheme of things, you should wind up with very small bits of code that do atomic tests and almost reads like natural language when you're done. One possible example (I've created fake types to complete the parameters to the methods):

    private static StatusCode CheckAccount(Account account, Item item, Message message, string UID)
    {
        const int HttpForbidden = 403;

        if (!ValidAccount(account))
        {
            return StatusCode(HttpForbidden);
        }

        if (!HasProperAuthorizations(account))
        {
            return StatusCode(HttpForbidden);
        }

        if (!TopicNameMatches(account, item) && !IsCurrentUser(message, UID))
        {
            return StatusCode(HttpForbidden);
        }

        // other stuff ?
    }

    private static bool ValidAccount(Account account) => account != null;

    private static bool HasProperAuthorizations(Account account) => account.Authorizations.Any(x =>
        x.Role.Name == RoleNames.ADMIN ||
        x.Role.Name == RoleNames.SENDER ||
        x.Role.Name == RoleNames.SENDERSADMIN);

    private static bool TopicNameMatches(Account account, Item item) => account.Authorizations.Any(x => string.Equals(
        x.TopicFilter.Name,
        item.Topic.Name,
        StringComparison.InvariantCultureIgnoreCase);

    private static bool IsCurrentUser(Message message, string UID) => string.Equals(
        message.Author.Username,
        UID,
        StringComparison.InvariantCultureIgnoreCase);

    private static StatusCode StatusCode(int statusCode) => new StatusCode(statusCode);
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks for the reply! Seeing this makes me kinda scratch my head why I have not thought about this myself. This makes it way more readable. \$\endgroup\$ Commented May 20, 2020 at 8:38
2
\$\begingroup\$

Your colleague code is clearly focused on readability while your code is focused on simplicity. You both have a good points (excluding the ToLower()).

You must keep in mind that some changes can't be beneficial, while others would not improve the performance much, but it would boost up code clarity and flexibility.

First take the roles :

x.Role.Name == RoleNames.SENDERSADMIN || x.Role.Name == RoleNames.ADMIN

this seems to me a general user role, where it's used in multiple places. If that's the case it would more easier if it's converted into a role class where it can be used something like this : UserRole.IsUserOrSenderAdmin(x.Role.Name) or using extension x.Role.Name.IsUserOrSenderAdmin() Another way is to have a property inside the Role property like this x.Role.IsAdmin || x.Role.IsSenderAdmin So, you are defining the general roles conditions inside the model itself. Which would be accessed directly. This would make it easier to handle roles (or user permissions) instead of repeating the same condition or having long condition like this one. This would add clarity and simplification to the used conditions.

The second part ToLower, you need to use StringComparison instead. so this :

x.TopicFilter.Name.ToLower() == item.Topic.Name.ToLower()

would be converted to this :

x.TopicFilter.Name.Equals(item.Topic.Name, StringComparison.OrdinalIgnoreCase);

This will avoid creating extra strings along with using the safe default for culture-agnostic string matching.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks! It is similar to @Jesse's approach to extrude the checks in seperate methods. The stringComparison is also something I should have thought about... \$\endgroup\$ Commented May 20, 2020 at 8:39
1
\$\begingroup\$

bool isAuthorized = account.Authorizations.Any(x => (x.Role.Name == RoleNames.ADMIN || x.Role.Name == RoleNames.SENDER || x.Role.Name == RoleNames.SENDERSADMIN) && x.TopicFilter.Name.ToLower() == item.Topic.Name.ToLower());

  1. I would place one check per line
  2. I would explicitly capture the 'item'

bool isCurrentUser = message.Author.Username.ToLower() == UID.ToLower();

if (!isAuthorized && !isCurrentUser) return StatusCode(403);

I would check isAuthorized before calculating isCurrentUser for performance sake.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.