-3
\$\begingroup\$

I don't quite know where to post this but C# is not my first language and I am hoping to get someone to nod their head or correct me :)

This works but seems counter intuitive - mostly because I would like to make a list and then say list contains oneCharacter but, for cases like this, it would be excessively wordy.

The only purpose of this sample code is to evaluate whether oneCharacter is in the string "YN" and to ask if there is a better (or more beautiful) way to do this. It is not any more complicated than that.

    void MainGameLoop()
  {
     while(true)
     {
         Console.WriteLine("Randomizing new string ...");
         var randomizedString = RandomizeString(); 
         Console.WriteLine("Please input the value, only first letter of the input will be taken into account");
         var userInput = Console.ReadLine();
         var userCharacter = userInput[0].ToString();

         // Here is the code I'm hoping to sort out.  Is this a reasonable approach within the If condition?
         if(!randomizedString.Contains(userCharacter))
         {
             Console.WriteLine("Sorry, wrong guess.");
         }
         // end code I'm hoping to sort out

         else
         {
             Console.WriteLine("You guessed it!");
         }
     }

  }
\$\endgroup\$
12
  • 1
    \$\begingroup\$ For me, usage of Contains is okay. It says exactly what you intend to check. In case you are curious, underneath it actually calls (inyour case)"YN".IndexOf(oneCharacter, StringComparison.Ordinal) >=0which is a search within an array. \$\endgroup\$ – MaLiN2223 Sep 14 '18 at 20:15
  • 2
    \$\begingroup\$ It's hard to tell what exactly this example code must accomplish, since you didn't specify. Is the string oneCharacter guaranteed to contain exactly one character? (What if it is the string "YN", or an empty string?) \$\endgroup\$ – 200_success Sep 14 '18 at 20:15
  • 3
    \$\begingroup\$ This being Code Review, we review code; we don't give generic advice on best practices. This is still a very sketchy example. Please don't simplify your code at all — we need to see your real code, as per the help center. Also see How to Ask. \$\endgroup\$ – 200_success Sep 14 '18 at 20:31
  • 3
    \$\begingroup\$ what is this code used for? \$\endgroup\$ – Malachi Sep 14 '18 at 20:33
  • 4
    \$\begingroup\$ Stack Overflow does not require questions to be about non-working code, but it does require you to ask about a specific programming issue to be solved; asking "what do you think of this?" would not be allowable there. Here on Code Review, asking for opinions is fine, but we require you to include enough context to make it concrete. Therefore, this question, as it is currently posed, is not good for any Stack Exchange site. \$\endgroup\$ – 200_success Sep 14 '18 at 20:44
-1
\$\begingroup\$

I like this question (for no particular reason) so I will answer with an example (OP can put it into the question so it doesn't get closed).

Say, we want to create a game which will randomize a string and user is expected to guess one letter within that string.

 void MainGameLoop()
 {
    while(true)
    {
        Console.WriteLine("Randomizing new string ...");
        var randomizedString = RandomizeString(); 
        Console.WriteLine("Please input the value, only first letter of the input will be taken into account");
        var userInput = Console.ReadLine();
        var userCharacter = userInput[0].ToString();
        // OP's code starts here
        if(!randomizedString.Contains(userCharacter))
        {
            Console.WriteLine("Sorry, wrong guess.");
        }
        // OP's code ends here
        else
        {
            Console.WriteLine("You guessed it!");
        }
    }

 }

Now, my answer:

For me, usage of Contains is okay. It says exactly what you intend to check. In case you are curious, underneath it actually calls (in your case) randomizedString.IndexOf(userCharacter, StringComparison.Ordinal) >=0 which is a search within an array. Additionally, I think you should change the order of logical conditions, using ! is not needed in this case:

if(randomizedString.Contains(userCharacter))
{
        Console.WriteLine("You guessed it!");
}
else
{
        Console.WriteLine("Sorry, wrong guess.");
}

EDIT: One could also argue that in this case it would be better to use IndexOf(Char) since OP needs only one character to check but for me, if performance is not an issue .Contains(string) is more readable than doing

if (randomizedString.IndexOf(someCharacter) != -1) { }

NOTE: if performance would be an issue, one would probably use for loop instead.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ @Missy I got two down votes so far :) To people who down vote: it would be more constructive if you explained in the comments why do you think I am incorrect \$\endgroup\$ – MaLiN2223 Sep 15 '18 at 12:44
  • 5
    \$\begingroup\$ The downvotes are likely a response to answering what appears to be an off-topic question. It's perhaps heavy handed, but the idea is to encourage high-quality questions by not supporting answers to off-topic questions. If the answerer thinks he can legitimately improve the question - without changing it completely - then editing is the right course of action. \$\endgroup\$ – VisualMelon Sep 15 '18 at 14:46
  • \$\begingroup\$ @MaLiN2223 -- I'm sorry that this has happened to you. I will fix it for you right now :) \$\endgroup\$ – Missy Sep 19 '18 at 19:56

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