HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMajor

A completely repetitive console prompt, a completely repetitive console prompt

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
consolerepetitivepromptcompletely

Problem

So I wrote this for my Mandelbrot Generator, to collect and use input from the user, and I made it dynamic enough to be useful anywhere.

I'm really interested in all critiques of it, as I'm going to be using it a lot now.

The idea is to allow the user to easily show messages on the console, that require input. Once valid input is received, the method returns.

Example usage:

int numberOfCores = Environment.ProcessorCount - 1;
numberOfCores = Prompt($"Enter the number of cores to use (1 to {Environment.ProcessorCount})",
                       false,
                       numberOfCores,
                       $"The value must be between 1 and {Environment.ProcessorCount}",
                       delegate (int x) { return x >= 1 && x <= Environment.ProcessorCount; });


This will prompt the user to enter a value between 1 and Environment.ProcessorCount. But, you could also use:

ushort maxIterations = 1000;
maxIterations = Prompt("Enter the maximum number of iterations", false, maxIterations);


Which will only prompt the user for a valid ushort value.

``
///
/// This will repeatedly prompt the user with a message and request input, then return said input (if valid).
///
/// The type of input that should be returned.
/// The message to initally display to the user.
/// Whether or not to allow use of a
defaultValue.
/// The default value to be returned if a user enters an empty line (
"").
/// The message to display on a failure. If null, then the
message` parameter will be displayed on failure.
/// An optional delegate to a method which can perform additional validation if the input is of the target type.
/// The input collected from the user when it is deemed valid.
static T Prompt(string message, bool requireValue, T defaultValue = default(T), string failureMessage = null, Func validationMethod = null)
where T : struct
{
if (!requireValue)
Console.Write(string.Format(message + " [{0}]: ", defaultValue));
else

Solution

String Formatting

if (!requireValue)
    Console.Write(string.Format(message + " [{0}]: ", defaultValue));
else
    Console.Write(message + ": ");


This innocent code will break if message contains any String.Format() formatting code. It may be acceptable if it's a private function but to make it reusable you have to address this issue, you have to treat callers input as user input. It's even slightly faster but you shouldn't really care about this here:

Console.Write(string.Format("{0} [{1}]: ", message, defaultValue));


You're already using interpolated strings then you may use them here too:

Console.Write($"{message} [{defaultValue}]");


Now you'll also see you repeat same code inside your loop, we're lazy so we like to avoid repeated code, move it inside your while loop, only at its beginning. About your loop: it has to be executed at least once then do/while is more clear (about its intent) than while.

do
{
    if (requireValue) ...
} while (true);


Note I also dropped exit condition, it'll be directly handled in your code with a return statement.

Input Conversion

It's time to read input. Most obvious issue is your conversion function Retrieve(). It's prolix and it's not even complete (what about decimal and char, for example?).

Simply replace it with:

var result = Convert.ChangeType(line, typeof(T));


Let's write an helper function (compacted code for brevity):

bool TryConvert(string text, bool ignoreIfEmpty, ref T value)
{
    // null is not possible, if it happens we may want ChangeType()
    // to throw ArgumentNullException because it's an actual error...
    if (ignoreIfEmpty && String.IsNullOrWhiteSpace(text)) 
        return true;

    try
    {
        value = (T)Convert.ChangeType(text, typeof(T));
    }
    catch (InvalidCastException) { return false; }
    catch (FormatException) { return false; }
    catch (OverflowException) { return false; }

    return true;
}


Use it:

T value = defaultValue;
if (!TryConvert(Console.ReadLine(), !requireValue, out value)
{
    Console.WriteLine("Input is not valid.");
    continue;
}


Validation

Your validation code is also little bit too convoluted, it may be simplified:

if (validationMethod != null && !validationMethod(value))
{
    Console.WriteLine(failureMessage ?? "Input is not valid.");
    continue;
}


Result

Everything all together:

static T Prompt(string message, ...
{
    do
    {
        Console.Write(requireValue ? $"{message} [{defaultValue}]: " : $"{message}: ");

        T value = defaultValue;
        if (!TryConvert(Console.ReadLine(), !requireValue, out value)
        {
            Console.WriteLine("Input is not valid.");
            continue;
        }

        if (validationMethod != null && !validationMethod(value))
        {
            Console.WriteLine(failureMessage ?? "Input is not valid.");
            continue;
        }

        return value;
   } while (true);
}


Further Refinements

So far we worked on function implementation however also its interface may be improved. First of all you're using a boolean parameter, especially when arguments list is long you may want to use an enumeration:

var maxIterations = Prompt("Number of iterations", PromptOptions.Required, 1000);


Moreover your function accepts many arguments (and most of them have default value). If you like to keep default value in-place I'd suggest to add more overloads (for most common cases). Note that you may also assume that if there is a default value then requireValue is false (to provide two simplified overloads).

Last change is generic constraint you applied. Limiting T to struct doesn't stop user of your function to use user defined value types (you don't know how to manage):

struct Point { public int X; public int Y }

var result = Prompt(...


Moreover it also exclude strings (and many user inputs are plain text). Change that constraint to IConvertible, Convert.ChangeType() will use it and your function may accept any type that can be convertible from string (you may even drop constraint and leave converter to do all its dirty games with all supported esoteric conversions, just write what you expect in your function interface documentation for T).

Code Snippets

if (!requireValue)
    Console.Write(string.Format(message + " [{0}]: ", defaultValue));
else
    Console.Write(message + ": ");
Console.Write(string.Format("{0} [{1}]: ", message, defaultValue));
Console.Write($"{message} [{defaultValue}]");
do
{
    if (requireValue) ...
} while (true);
var result = Convert.ChangeType(line, typeof(T));

Context

StackExchange Code Review Q#104184, answer score: 20

Revisions (0)

No revisions yet.