Too many methods very similar

Question

I have many methods which are very similar as shown in the code below:

    public static void ReadFromKeyboard(string label, out int retVal)
    {
        try
        {
            Console.Write(label);
            retVal = int.Parse(Console.ReadLine());
        }
        catch (Exception)
        {
            Console.WriteLine("Please insert int value.");
            ReadFromKeyboard(label, out retVal);
        }
    }

    public static void ReadFromKeyboard(string label, out float retVal)
    {
        try
        {
            Console.Write(label);
            retVal = float.Parse(Console.ReadLine());
        }
        catch (Exception)
        {
            Console.WriteLine("Please insert float value.");
            ReadFromKeyboard(label, out retVal);
        }
    }

    public static void ReadFromKeyboard(string label, out double retVal)
    {
        try
        {
            Console.Write(label);
            retVal = double.Parse(Console.ReadLine());
        }
        catch (Exception)
        {
            Console.WriteLine("Please insert double value.");
            ReadFromKeyboard(label, out retVal);
        }
    }

By the other hand, I don't know which method I will call. I'll discorver it only at runtime.

Is there any way I could rewrite these many methods into a single method named something like "ReadFromKeyboard" which returns either an int, a float or a double depending on the type which is passed to it as a parameter?

Thank you!

Answer 1

I've tried to implement the code according to Eric Lippert recipes. The code below

1. does not use exception handling as mainline control flow
2. does not use recursion at all
3. does not use output parameters without need

.

private static void Main(string[] args)
{
    int intValue = ReadFromKeyboardInt32("enter int");
    float floatValue = ReadFromKeyboardSingle("enter float");
    double doubleValue = ReadFromKeyboardDouble("enter double");

    Console.WriteLine($"{intValue}, {floatValue}, {doubleValue}");
}

public static Double ReadFromKeyboardDouble(string label) =>
    ReadFromKeyboard(label, (text) => (Double.TryParse(text, out var value), value));

public static Int32 ReadFromKeyboardInt32(string label) =>
    ReadFromKeyboard(label, (text) => (Int32.TryParse(text, out var value), value));

public static Single ReadFromKeyboardSingle(string label) =>
    ReadFromKeyboard(label, (text) => (Single.TryParse(text, out var value), value));

public static T ReadFromKeyboard<T>(string label, Func<string, (bool, T)> tryParse)
{
    for (; ; )
    {
        Console.Write($"{label}: ");
        var result = tryParse(Console.ReadLine());
        if (result.Item1)
        {
            return result.Item2;
        }
        Console.WriteLine($"Please enter valid {typeof(T).Name} value");
    }
}

Answer 2

As other answers have shown, you can eliminate the duplicated code by a variety of techniques, all of which are horrible and you should not do them.

In particular, do not attempt to use generics to solve this "problem". Generics are for situations where the code is generic. That is why they are called generics! That is, the code operates the same on every possible type. Your example is the opposite of generic code; you have different rules for a small number of types, and the way to handle that situation is to do exactly what you have already done: implement one method per different rule.

I say "problem" in quotes because you do not actually have a problem to solve here, so stop trying to solve it. Writing half a dozen similar short methods is not a major burden on authors or maintainers.

Now, that said, your code is also not as good as it could be and you should rewrite it. The correct way to write your code is:

public static int ReadInteger(string label)
{
    while(true)
    {
        int value;
        Console.Write(label);
        string read = Console.ReadLine();
        bool success = int.TryParse(read, out value);
        if (success)
          return value;
        Console.WriteLine("Please type an integer value.");
    }
}

The problems with your original implementation are:

• Do not use exception handling as mainline control flow. Do not catch an exception if the exception can be avoided. That's what TryParse is for.
• Do not use recursion as unbounded looping. If you want an unbounded loop, that's what while(true) is for. Remember, C# is not tail recursive by default!
• Do not use out parameters without need. The method logically returns an integer, so actually return an integer. Rename it so that you do not get collisions with other read methods. There is no compelling benefit to making the caller write Read<int> over ReadInteger, and many compelling benefits to avoiding the out param.

Answer 3

Instead of listing all the possible types (which you might not know beforehand), it is possible to use the System.Convert class, specially the Convert.ChangeType() method. As a proof of concept you can use a method like this:

public static void ReadFromKeyboard<T>(string label, out T result) {
    Type targetType = typeof(T);
    Console.Write($"{label}: ");
    string input = Console.ReadLine();
    object convertedValue = Convert.ChangeType(input, targetType);
    result = (T)convertedValue;
}

You can use this method like this:

public static void Main(string[] args) {
    ReadFromKeyboard("enter a double", out double d);
    ReadFromKeyboard("enter an int", out int i);
    Console.WriteLine($"double: {d}");
    Console.WriteLine($"int: {i}");
}

This way it is possible to use any type you want (assuming it is supported by the Convert class). Obviously you can add exception handling and a do-while loop in the ReadFromKeyboard method if you like.

Answer 4

If you want to rely on overload resolution for the runtime to decide which method to call, then you must have a separate method for each type you will support. That's how it works.

On the other hand, if you can allow the user to supply at least a little type information, we can improve things a bit with generics by removing try/catch and using a real return statement. You'd call it like this:

var myNumber = ReadFromKeyboard<double>("Enter a double: ");

And the code would look like this:

public static T ReadFromKeyboard<T>(string label, int maxRetries = int.MaxValue)
{
    while (maxRetries >= 0)
    {
        Console.Write(label);

        if (typeof(T) == typeof(int))
        {
            int result;
            if (int.TryParse(Console.ReadLine(), out result)) return (T)(object)result;
        }
        if (typeof(T) == typeof(float))
        {
            float result;
            if (float.TryParse(Console.ReadLine(), out result)) return (T)(object)result;
        }
        else if (typeof(T) == typeof(double))
        {
            double result;
            if (double.TryParse(Console.ReadLine(), out result)) return (T)(object)result;
        }
        else if (typeof(T) == typeof(decimal))
        {
            decimal result;
            if (decimal.TryParse(Console.ReadLine(), out result)) return (T)(object)result;
        }
        else 
            throw new InvalidOperationException("Unsupported type");
        maxRetries--;
    }
    throw new InvalidOperationException("Too many bad inputs");
}

But you have to do some really janky casting and type checking to make it work. There is still a potential this can throw an exception, which it seems like you want to avoid, but if your user sits there for more than 2 billion attempts, I doubt they'll be very surprised.