0
\$\begingroup\$

I want to see if there is a cleaner way to do this logic. I have a working sample:

Same fiddle.net

public static Boolean passwordchecker(string userName)
    {
        string[] group = new string[] { "a", "b" };
        if (group[0] == userName || group[1] == userName)
        {
            return true;
        }
        else
        {
            return false;
        }
    }
\$\endgroup\$
3
  • 5
    \$\begingroup\$ It's unclear which specific elements of the logic you're interested in, or what the specific requirements are. Will there always be two strings, or could there be more? How many more? The naming here suggests that the elements in group will be coming most likely from a database table or some other dynamic source, and depending on the application there could be anywhere from a handful of users to tens or hundreds of millions. These considerations should guide your design decisions. Can you state the significant requirements? \$\endgroup\$
    – phoog
    Commented Mar 17, 2022 at 8:03
  • \$\begingroup\$ If the parameter refers to a password, it should be char[] or byte[], never string. \$\endgroup\$
    – mafu
    Commented Mar 19, 2022 at 6:26
  • 2
    \$\begingroup\$ I know I late to this game, but you call a method passwordChecker where you compare a username against predefined groups???? I mean what??? \$\endgroup\$ Commented Mar 21, 2022 at 21:36

4 Answers 4

8
\$\begingroup\$

Your code has three issues:

  1. The name passwordchecker: good method names are almost always a verb because they do things. I’d use CheckPassword (the capitalisation follows standard C# naming conventions). And, while we’re talking about names: the variable name group does not describe what the variable does, and, as a consequence, it took me unnecessary time to understand what your code is supposed to do. And, lastly, the parameter you call userName isn’t a user name; it’s a password, isn’t it? If so, you need to fix it. If it’s correct, you need to fix your method name because apparently it checks user names, not passwords.

  2. The code uses the anti-pattern if (expr) return true; else return false;. This simply never makes sense, and it never makes for clearer code when compared to the alternative: return expr;. In fact, outside of direct variable initialisation, there’s virtually no use for the literals true and false.

  3. There’s an array, but the code uses its values separately. As it stands, there is no reason for this to be an array (rather than separate variables), and it’s (very slightly) error-prone since a future maintainer might be tempted to extend the array, but forget to adjust its usage. AlanT’s answer shows one way of checking whether a value can be found inside an array. Personally, I’d be tempted to use the System.Linq.Enumerable.Contains method instead, because it doesn’t require using a predicate:

    public static bool CheckPassword(string password)
    {
        var passwords = new[] { "a", "b" };
        return passwords.Contains(password);
    }
    

    This requires using System.Linq;. Without that, you’ll have to cast the array explicitly to IList<string> because arrays only implement the IList<T> interface privately.

Furthermore, as shown in AlanT’s answer, I’d be tempted to make passwords a private readonly static variable rather than a local variable.

\$\endgroup\$
9
  • \$\begingroup\$ Downvoted for #3. A cast is a last resort in C#; you should almost never see it. Here you are doing it in order to get an interface you don't need (IList is for adding/removing) to use an uncommon method, instead of the simple, canonical IEnumerable.Any. Trying to avoid the predicate is anachronistic for no reason; modern C# uses Funcs and arrow functions. \$\endgroup\$ Commented Mar 17, 2022 at 19:03
  • 2
    \$\begingroup\$ @JounceCracklePop I really fail to see how Any is more canonical than Contains. Avoiding a predicate has nothing to do with anachronism: Contains is simply the more appropriate method here, and it’s less code (the cast adds code, but it’s at declaration, not use). At any rate the cast is used as a last resort here: it’s purely to work around an API issue: arrays should really implement IList<T> publicly. \$\endgroup\$ Commented Mar 17, 2022 at 19:19
  • 1
    \$\begingroup\$ @JounceCracklePop There’s more than one valid mental model, and the relevant mental model in this case is simply “sequence container”. The mental model you mention is correct but irrelevant, a pure implementation detail. We choose an array here merely because C# happens to provide a convenient construction syntax for arrays. You’re getting hung up on completely the wrong detail. If you dislike it, construct a List<string> instead of an array. \$\endgroup\$ Commented Mar 17, 2022 at 22:40
  • 3
    \$\begingroup\$ With using System.Linq you can avoid the cast, since Enumerable.Contains exists. (For those micro-optimizers out there: Yes, the code of Enumerable.Contains is optimized to check for the ICollection<T> interface and just forward the call to ICollection<T>.Contains. Not that it matters for this use case...) \$\endgroup\$
    – Heinzi
    Commented Mar 18, 2022 at 9:53
  • 2
    \$\begingroup\$ @Heinzi Of course, how could I forget that. Yes, that’s a much better solution. \$\endgroup\$ Commented Mar 18, 2022 at 9:59
11
\$\begingroup\$

Some of the naming seems off - it is a password checker but you are passing in and comparing a userName - but if you want a linq-ish solution you might try

private readonly static string[] Group = { "a", "b"};

private static bool PasswordChecker(string password)
{
    return Group.Any(g => g==password);
}

Notes:
I would generally make names of methods PascalCased in C# rather than all lower case.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Slightly shorter and slightly more readable: Group.Contains(password) \$\endgroup\$
    – Heinzi
    Commented Mar 17, 2022 at 6:32
  • 1
    \$\begingroup\$ @Heinzi, Yes, that works better. The original version I had was doing case-insensitive comparisons (input was userName, which shouldn't be case sensitive). I changed back to match the original code I should have switched over to Contains \$\endgroup\$
    – AlanT
    Commented Mar 17, 2022 at 8:35
  • \$\begingroup\$ The inconsistent indentation could also be mentioned. \$\endgroup\$ Commented Mar 17, 2022 at 19:20
  • \$\begingroup\$ There is also Tools for format and error checking in your programming language. \$\endgroup\$ Commented Mar 17, 2022 at 19:50
5
\$\begingroup\$

Definitely some shorter/more performant ways to write that:

private static readonly string[] group = new string[] { "a", "b" };

public static bool passwordchecker(string userName) => group[0] == userName || group[1] == userName;

for example.

\$\endgroup\$
3
  • 7
    \$\begingroup\$ This is unlikely to be more performant. \$\endgroup\$
    – phoog
    Commented Mar 17, 2022 at 6:45
  • 2
    \$\begingroup\$ Anything that uses an array will be less performant than just username == "a" || username == "b" \$\endgroup\$
    – Sean Reid
    Commented Mar 17, 2022 at 7:51
  • \$\begingroup\$ The array is not recreated (the GC'd) on every call to the method like the OP version. That is more performant. \$\endgroup\$ Commented Mar 17, 2022 at 13:40
3
\$\begingroup\$

I'd suggest something closer to this:

A method's name should describe what it's doing.
When a method returns a bool, it's often best to start the method name with a verb like "is" so that it asks a question.

If statements should be used sparingly.
Else statements should be used even less.

    public static bool IsUserNameValid(string userName) =>
        new List<string> { "a", "b" }
        .Any(value => string.Equals(value, userName, StringComparison.InvariantCultureIgnoreCase));
\$\endgroup\$

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