1
\$\begingroup\$

I have two style of code but don't know which is better or maybe you could recommend the better one.

The good part of first one is every if else branches return something and the process will stop at there but the bad part is even the Captcha is invalid I still need to query the DisalbeUser() and BlackList()

        bool isValidateCaptcha = ValidateCaptcha();

        bool isDisableUser = DisableUser();

        bool isInblackList = BlackList();

        if (isValidateCaptcha)
        {
            return Ok(new
            {
                IsSuccess = false,
                InvalidCode = LoginInvalidCode.CaptchaInvalid
            });
        }
        else if (isDisableUser)
        {
            return Ok(new
            {
                IsSuccess = false,
                InvalidCode = LoginInvalidCode.DisableAccountAndInDisableGroup
            });
        }
        else if (isInblackList)
        {
            return Ok(new
            {
                IsSuccess = false,
                InvalidCode = LoginInvalidCode.InLoginBlackList
            });
        } 
        else
        {
            return Ok(new
            {
                IsSuccess = true,
            }); 
        }

And the second style is like below

        bool isValidateCaptcha = ValidateCaptcha();

        if (isValidateCaptcha)
        {
            return Ok(new
            {
                IsSuccess = false,
                InvalidCode = LoginInvalidCode.CaptchaInvalid
            });
        }

        bool isDisableUser = DisableUser();

        if (isDisableUser)
        {
            return Ok(new
            {
                IsSuccess = false,
                InvalidCode = LoginInvalidCode.DisableAccountAndInDisableGroup
            });
        }

        bool isInblackList = BlackList();

        if (isInblackList)
        {
            return Ok(new
            {
                IsSuccess = false,
                InvalidCode = LoginInvalidCode.InLoginBlackList
            });
        }

        return Ok(new
        {
            IsSuccess = true,
        });
\$\endgroup\$
5
  • 3
    \$\begingroup\$ The "good" part of the first one is also present more clearly in the second one. What you really need to change are method names (also note that variable name and method name have an opposite meaning, assuming variable is the right one then your methods are really misleading) \$\endgroup\$ Commented Nov 16, 2017 at 6:42
  • \$\begingroup\$ I got some reference from stackoverflow.com/questions/268132/… and jetbrains.com/resharper \$\endgroup\$
    – MichaelMao
    Commented Nov 16, 2017 at 7:32
  • 2
    \$\begingroup\$ The reduce nesting quick-fix from R# is one of the most stupid suggestions that I always disable because it hurts my eyes seeing if(!somethig) \$\endgroup\$
    – t3chb0t
    Commented Nov 16, 2017 at 8:00
  • \$\begingroup\$ return Ok(new { IsSuccess = false, ... }); Is this valid syntax? Does this create a new anonymous class? Did you intend to write return new Ok { IsSuccess = true, ... }; \$\endgroup\$ Commented Nov 16, 2017 at 9:20
  • 2
    \$\begingroup\$ Welcome to Code Review! This question is incomplete. To help reviewers give you better answers, please edit to add sufficient context to your question. The more you tell us about what your code does, the easier it will be for reviewers to help you. Questions should include a description of what the code does. \$\endgroup\$ Commented Nov 17, 2017 at 10:10

1 Answer 1

7
\$\begingroup\$

Concerning

which one is better

I would prefer the first one, as you can see your values from the beginning, then follow the if-else logic.

However, none if these are very readible!

Naming

Your variable and method names are very misleading. BlackList() looks like you call a constructor. Method names should be verbs (or start with one).

Your variable naming scheme would actually fit for your methods, as IsInBlackList() is the typical naming for methods that return boolean (true/false) values. The variables could then just be called var inBlackList = IsInBlackList().

Logic

It is not clear whether your validation cases are mutually exclusive. Can a captcha be valid and still the user be in the blacklist?

Concerning this code: if (isValidateCaptcha) ... IsSuccess = false, ... Your variable name implies that it is true if the captcha is valid (which it should, however a better name might be isCaptchaValid). But then you set IsSuccess to false, which makes it seem like your method returns false if the captcha is true, making the logic very hard to understand without looking into the method.

Conciseness

Do you even need these variables? Your variable names are almost identical to your method names (semantically, considering the changes suggested above).

Consider this:

if (IsCaptchaValid()) // replaced the variable with the method call
{
    return Ok(new
    {
        IsSuccess = false,
        InvalidCode = LoginInvalidCode.CaptchaInvalid
    });
}

This way your code becomes shorter and more readible, and your method names should show their intent anyways.

Redundance

This part

return Ok(new
{
    IsSuccess = ...
    InvalidCode = ...
});

appears multiple times with very minor differences. You could increase readability by merging it into one.

For example

LoginInvalidCode invalidCode;
if (!IsCaptchaValid())
{
    invalidCode = LoginInvalidCode.CaptchaInvalid;
}
else if (IsUserDisabled()) {
    invalidCode = LoginInvalidCode.DisableAccountAndInDisableGroup;
}
else if (IsInBlackList()) {
    invalidCode = LoginInvalidCode.InLoginBlackList;
}

var success = invalidCode != null;

return Ok(new { IsSuccess = success, InvalidCode = invalidCode });
\$\endgroup\$
2
  • \$\begingroup\$ Sorry about naming I just want to show the process so I do not take care the naming \$\endgroup\$
    – MichaelMao
    Commented Nov 16, 2017 at 9:13
  • \$\begingroup\$ @MichaelMao And that's exactly why you shouldn't post example code on Code Review. Post actual code, code you're using in your code base. Please take a look at the help center before posting your next question. \$\endgroup\$
    – Mast
    Commented Nov 17, 2017 at 9:25

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