2
\$\begingroup\$
  1. Can I make this:

    if (oneFour == 1 || oneFour == 2 || oneFour == 3 || oneFour == 4)
    

    shorter, especially if I'm gonna need a wider range of integers?

  2. Do I need to use a string _warning twice like I did?

static string _warning = "Please, enter a number from 1 to 4!";

static int oneToFour()
{
    int oneFour = 0;
    bool isError = true;
    while (isError)
    {
        try
        {

            oneFour = Convert.ToInt32(Console.ReadLine());
            if (oneFour == 1 || oneFour == 2 || oneFour == 3 || oneFour == 4)
                isError = false;
            else
                Console.WriteLine(_warning);
        }
        catch { Console.WriteLine(_warning); }
    }

    return oneFour;
}
\$\endgroup\$
3
\$\begingroup\$

You should start using comparison operators,

for a range from 1 to 4, use this:

if (oneFour >= 1 && oneFour <= 4)

This reads naturally as "if variable is more than or equal to 1 and variable is less then or equal to 4", which is the same thing as "if variable is in range 1 to 4".

Other improvements:

  • Instead of setting the isError value, you can return oneFour; directly.
  • Using the above advice, you can change your loop to while (true), eliminating your isError totally.
  • Using the above advice, you can make the scope for the oneFour variable smaller, which is a good thing (when possible).
  • Braces. Always use braces.

Resulting code will look something like this:

static int oneToFour()
{
    while (true)
    {
        try
        {

            int oneFour = Convert.ToInt32(Console.ReadLine());
            if (oneFour == 1 || oneFour == 2 || oneFour == 3 || oneFour == 4)
            {
                return oneFour;
            }
            else
            {
                Console.WriteLine(_warning);
            }
        }
        catch { Console.WriteLine(_warning); }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you very much, sir! But I guess I should still keep this line int oneFour = 0; at the beginning, right? \$\endgroup\$ – Serg Z. Jul 26 '15 at 21:47
  • \$\begingroup\$ @SergZ. See the edit I just did. You can make the scope for that variable smaller now. \$\endgroup\$ – Simon Forsberg Jul 26 '15 at 21:51
  • \$\begingroup\$ And what if I declare it as static int (equal to zero) outside the method? Why is it worse? \$\endgroup\$ – Serg Z. Jul 26 '15 at 22:14
  • \$\begingroup\$ @SergZ. Because then your variable uses space even when it is not needed. When this method is not running, you don't need that variable. It also opens up to a risk of leaving the variable in a state and then accidentally reading that old value when you shouldn't do so. Also, as you learn how to do more advanced things in Programming, you will learn about multithreading, which is a very advanced concept, but having a single static int variable would be horrible then. \$\endgroup\$ – Simon Forsberg Jul 26 '15 at 22:18
3
\$\begingroup\$

I agree with everything Simon has suggested. But to add a slightly different approach (albiet a bit over the top maybe for this question), I would consider removing the need for the exception altogether and introducing a couple of methods.

Even being simple, by moving some things into methods I believe it helps with readability as well as abstracting away some of the method details so that the main method can concentrate on flow, not the rules within that flow.

The resultant code might look something like:

private static string _warning = "Please, enter a number from 1 to 4!";

public static int GetValueFromUser()
{
    return PromptForValue();
}

public static int GetValueFromUser(Func<int> promptForUserInput)
{
    while (true)
    {
        var oneFour = promptForUserInput();
        if (IsInRange(oneFour))
        {
            return oneFour;
        }

        // I might also consider making this a call-back Func<string>
        // which could completely remove the need for console and provide
        // another unit test option
        Console.WriteLine(_warning);                
    }
}

private static int PromptForValue()
{
    int enteredValue;

    return Int32.TryParse(Console.ReadLine(), out enteredValue) ? enteredValue : -1;
}

private static bool IsInRange(int enteredValue)
{
    return enteredValue >= 1 && enteredValue <= 4;
}

You might notice two methods for GetValueFromUser(). I did this as I wanted to do some unit testing on the methods, but to setup that I didn't want to have to enter in a value.

My unit tests included:

[TestMethod]
public void InRange_ExpectValueBack()
{
    // Arrange
    const int expectedValue = 4;
    //  var cls = new CodeReviewClass(); 

    // Act
    var result = CodeReviewClass.GetValueFromUser(() => expectedValue);

    // Assert
    result.Should().Be(expectedValue);
}

[TestMethod]
public void ToLow_ExpectRePrompt()
{
    // Arrange
    const int valueTooLow = 0;
    const int expectedValue = 4;
    const int expectedAttempts = 1;

    //var cls = new CodeReviewClass();
    int attempts = 0;

    // Act
    var result = CodeReviewClass.GetValueFromUser((() =>
    {
        if (attempts == 0)
        {
            attempts++;
            return valueTooLow;
        }

        return expectedValue;
    }));

    // Assert
    attempts.Should().Be(expectedAttempts);
    result.Should().Be(expectedValue);
}

[TestMethod]
public void ToHigh_ExpectRePrompt()
{
    // Arrange
    const int valueTooHigh = 10;
    const int expectedValue = 4;
    const int expectedAttempts = 1;

    //var cls = new CodeReviewClass();
    int attempts = 0;

    // Act
    var result = CodeReviewClass.GetValueFromUser((() =>
    {
        if (attempts == 0)
        {
            attempts++;
            return valueTooHigh;
        }

        return expectedValue;
    }));

    // Assert
    attempts.Should().Be(expectedAttempts);
    result.Should().Be(expectedValue);
}

[TestMethod]
public void NotANumber_ExpectRePrompt()
{
    // TODO:  I would love to have this unit test as well?
}

[TestMethod]
public void NumberWithSpaces_ExpectRePrompt()
{
    // TODO:  I would love to have this unit test as well?
}

NOTE: I would have loved to test for the not a number scenario, but wasn't sure how to generate that given the definition of my parameter Func.

Also, initially I remove the need for a static class but reverted that to keep in line with your implementation. hence the commented out var cls = new CodeReviewClass();

\$\endgroup\$
  • 1
    \$\begingroup\$ I believe this answer is ahead of accepted one, since it directs you into a better approach. More prefereble. \$\endgroup\$ – Tolga Evcimen Jul 28 '15 at 13:38
1
\$\begingroup\$

Some points not previously mentioned

  • Consider following recommend C# naming convetions i.e the method name oneToFour should be OneToFour
  • oneFour is not a good name for a variable because it leaks data into the name result would be better
  • Using int.TryParse is a good way to avoid the use of _warning twice and removes the need for the exception handler.

Let's see how the method might look after a refactor.

    static int OneToFour()
    {
        while (true)
        {
            var input = Console.ReadLine();
            int result;

            if (int.TryParse(input, out result))
            {
                if (result >= 1 && result <= 4)
                    return result;
            }

            Console.WriteLine(_warning);
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Won't TryParse throw exception if a user just presses enter without typing anything? \$\endgroup\$ – Serg Z. Jul 28 '15 at 14:15
  • \$\begingroup\$ From the MSDN documentation: "the TryParse method does not throw an exception if the conversion fails" and "The conversion fails if the s parameter is null or String.Empty, is not of the correct format, or represents a number less than MinValue or greater than MaxValue". So no I don't think it's a problem, but of course it would need to be tested. \$\endgroup\$ – craftworkgames Jul 29 '15 at 1:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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