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\$

closed as unclear what you're asking by t3chb0t, Vogel612, Mast, Donald.McLean, Graipher Nov 23 '17 at 8:16

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 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\$ – Adriano Repetti Nov 16 '17 at 6:42
  • \$\begingroup\$ I got some reference from stackoverflow.com/questions/268132/… and jetbrains.com/resharper \$\endgroup\$ – MichaelMao Nov 16 '17 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 Nov 16 '17 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\$ – Raimund Krämer Nov 16 '17 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\$ – Toby Speight Nov 17 '17 at 10:10
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\$
  • \$\begingroup\$ Sorry about naming I just want to show the process so I do not take care the naming \$\endgroup\$ – MichaelMao Nov 16 '17 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 Nov 17 '17 at 9:25

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