31
\$\begingroup\$

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.

/// <summary> /// This will repeatedly prompt the user with a message and request input, then return said input (if valid). /// </summary> /// <typeparam name="T">The type of input that should be returned.</typeparam> /// <param name="message">The message to initally display to the user.</param> /// <param name="requireValue">Whether or not to allow use of a `defaultValue`.</param> /// <param name="defaultValue">The default value to be returned if a user enters an empty line (`""`).</param> /// <param name="failureMessage">The message to display on a failure. If null, then the `message` parameter will be displayed on failure.</param> /// <param name="validationMethod">An optional delegate to a method which can perform additional validation if the input is of the target type.</param> /// <returns>The input collected from the user when it is deemed valid.</returns> static T Prompt<T>(string message, bool requireValue, T defaultValue = default(T), string failureMessage = null, Func<T, bool> validationMethod = null) where T : struct { if (!requireValue) Console.Write(string.Format(message + " [{0}]: ", defaultValue)); else Console.Write(message + ": "); bool pass = false; T result = default(T); while (!pass) { string line = Console.ReadLine(); if (requireValue) { pass = Retrieve(line, out result); if (pass && validationMethod != null) pass = validationMethod(result); } else { if (line != "") { pass = Retrieve(line, out result); if (pass && validationMethod != null) pass = validationMethod(result); } else { pass = true; result = defaultValue; } } if (!pass) { Console.WriteLine("Invalid value [{0}]", line); if (failureMessage != null) { if (!requireValue) Console.Write(string.Format(failureMessage + " [{0}]: ", defaultValue)); else Console.Write(failureMessage + ": "); } else { if (!requireValue) Console.Write(string.Format(message + " [{0}]: ", defaultValue)); else Console.Write(message + ": "); } } } return result; } private static bool Retrieve<T>(string line, out T resultValue) where T : struct { var type = typeof(T); resultValue = default(T); bool pass = false; if (type == typeof(short)) { short result = 0; pass = short.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(int)) { int result = 0; pass = int.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(float)) { float result = 0f; pass = float.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(double)) { double result = 0f; pass = double.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(sbyte)) { sbyte result = 0; pass = sbyte.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(byte)) { byte result = 0; pass = byte.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(ushort)) { ushort result = 0; pass = ushort.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(uint)) { uint result = 0; pass = uint.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(long)) { long result = 0; pass = long.TryParse(line, out result); resultValue = (T)(object)result; } else if (type == typeof(ulong)) { ulong result = 0; pass = ulong.TryParse(line, out result); resultValue = (T)(object)result; } return pass; }

Additional notes: if anyone would like to use this in your projects, you are more than welcome to.

\$\endgroup\$

5 Answers 5

Reset to default
20
\$\begingroup\$

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<T>(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<T>(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<Point>(...

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).

\$\endgroup\$
12
\$\begingroup\$

You can remove all your if-statements and use Convert.ChangeType() instead. Alternatively, use T4 to generate the code for you and turn it into a partial class. Anything to avoid duplicating like that.

For an example of this Convert.ChangeType() implementation, take a look at Adriano Repetti's answer.

\$\endgroup\$
2
  • \$\begingroup\$ Could you show what replacing the ifs with Convert.ChangeType() looks like, just to make it easier for future students? \$\endgroup\$ Commented Sep 9, 2015 at 10:47
  • 1
    \$\begingroup\$ @deworde: there are a few other answers that incorporate it already (for example here) so that will suffice. I'll link to it in my answer. \$\endgroup\$ Commented Sep 9, 2015 at 10:55
12
\$\begingroup\$ 
while (condition) { if (condition) { /* more nesting */ } }

All this nesting is getting to be a bit much to navigate. I counted 5 levels of nesting in a few places.

I would escape the loop and reduce my nesting like this:

if (!condition) { break; }

Then, you know that condition still evaluates to true.

Right here, you duplicate certain if conditions:

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

I would write that as:

if (!requireValue) { var message = string.Format("{0} [{1}]: ", failureMessage ?? message, defaultValue); Console.Write(message); } else { var message = string.Format("{0}: ", failureMessage ?? message); Console.Write(message); }

Why aren't you using braces? It is known that not using braces summons raptors (Or was that using gotos? They are both bad.).

In your Retrieve<T>(string line, out T resultValue) method, you are duplicating the logic of your code all over the place, defeating the purpose of using the generic. You should split that into a bunch of overloaded methods with the type specifically designated in the signature.

\$\endgroup\$
0
11
\$\begingroup\$

I don't think Retrieve is that great a name; TryGetValue would be closer to what it does.

In several places your if/else starts with a negative check, e.g. if (line != "") and if (!requireValue). I'd invert those: positive checks are easier to read.

Also, avoid using "" when you can use string.Empty. Matter of fact, why aren't you using string.IsNullOrEmpty()?

That very long and very repetitive if/else if can probably be replaced by something much shorter. Here's something I used recently which you should be able to convert to something that's applicable in your case:

private T GetValue<T>(DataRow dataRow, string columnName) { var value = dataRow[columnName]; if (value == null || value == DBNull.Value) return default(T); var targetType = typeof(T); if (targetType.IsGenericType && targetType.GetGenericTypeDefinition() == typeof(Nullable<>)) targetType = targetType.GetGenericArguments().First(); return (T)Convert.ChangeType(value, targetType); }
\$\endgroup\$
10
\$\begingroup\$

One principle of code design is DRY -- Don't Repeat Yourself. This code repeats itself in several places, with a pattern similar to

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

This suggests that this code should be put into a new method which takes requireValue, message, and defaultValue.

Another duplicated code is

pass = Retrieve(line, out result); if (pass && validationMethod != null) pass = validationMethod(result);

which should similarly be extracted to a method.

Finally in Retrieve() we see:

if (type == typeof(short)) {

Why isn't this a switch? But also the code is repeated; there should be some way to factor this out.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Unfortunately: "A switch expression or case label must be a bool, char, string, integral, enum, or corresponding nullable type". \$\endgroup\$ Commented Sep 9, 2015 at 2:50
  • 1
    \$\begingroup\$ Not that I'm recommending this (Jeroen's answer almost certainly the correct way), but typeof().ToString() would get you your switchable, with no major loss of information. \$\endgroup\$ Commented Sep 9, 2015 at 10:51

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.