Answered Please critique my method

WB1975

Well-known member
Joined
Apr 3, 2020
Messages
87
Programming Experience
Beginner
This method asks for a string between min and max characters

and then asks for confirmation on entering a valid string

please let me know if it can be done more efficiently etc etc
C#:
public static string GetLimitedStringWithConfirmation(string prompt, int min, int max)
{
    do
    {
        Console.WriteLine(prompt);

        var userString = Console.ReadLine();

        if (userString.Length >= min && userString.Length <= max)
        {
            Console.Write($"You entered {userString} Do you accept [Y/N]: ");

            char ch = Console.ReadKey(true).KeyChar;
            ch = char.ToUpper(ch);
            switch (ch)
            {
                case 'Y':
                    return userString;
                case 'N':
                    Console.WriteLine("\nThe entry was rejected...");
                    break;
            }
        }
    } while (true);
}
 
Last edited by a moderator:
Given that it's pretty much a copy and paste of what another member provided in another thread, I'm not sure exactly what you're expecting. Also, the code does what it does so, from that perspective, it's perfect. If you were to provide an explanation of your aim, rather than expecting us to work it out from the code, then we might be able to spot some issue. There's nothing obviously major wrong with the code but I'd be inclined to rename the last two parameters to indicate what the really mean, i.e. they are specifically the min and max LENGTH but there's no indication of that anywhere outside the method. It's a small thing but I'd also be inclined to combine these two lines into one:
C#:
char ch = Console.ReadKey(true).KeyChar;
ch = char.ToUpper(ch);
Finally, it's an even smaller thing but I'd also be inclined to call Console.WriteLine twice here, rather than including a line break in the text:
C#:
Console.WriteLine("\nThe entry was rejected...");
It's just a bit clearer because there's no literal line breaks anywhere else so it may be missed.
 
I forgot to mention that there's some punctuation missing here too:
C#:
Console.Write($"You entered {userString} Do you accept [Y/N]: ");
I may or may not use a question mark but I'd definitely go at least this far:
C#:
Console.Write($"You entered \"{userString}\". Do you accept [Y/N]: ");
 
I would refactor 'get confirmation' code to a separate method. Also, what happens if user press a different char than Y/N?
 
Consider inserting a default case for your switch statement so it doesn't just fall through if a condition is not met.


Why do you need to write the prompt? If this is not needed, just set the var userString to prompt
 
Thanks all,

yes i did string code together but did have to change it for a specific purpose, im just trying to learn correct practices from the get go, i hope you understand.

JohnH: I would refactor 'get confirmation' code to a separate method.
i actually have a yes no method, dont know what i was thinking.

JohnH : what happens if user press a different char than Y/N? yes its acting like n/N which is not correct, must look at that too.

jmcilhinney: will look at that too!

Sheepings: will add a default to my switch, thanks for the advice.

can anybody else think of any other useful methods that could be added to this utility class?

a menu? 1 to 5 and q to quit?

Mod edit. No need to bold. It's a bit of an eye sore
 
Last edited by a moderator:
The one that I invariably always end up writing for a console program is a menu of choices. E.g. I would call it with something like:
C#:
var favoriteArtist = ChooseOne<string>("Select your favorite artist:", "Enya", "Queen", "Dire Straights", "OMD", "John Williams");
var rouletteColor = ChooseOne<Color>("Place your bets on:", Color.Red, Color.Black);
which would show up in the UI as:
C#:
Select your favorite artist:
1 - Enya
2 - Queen
3 - Dire Straights
4 - OMD
5 - John Williams
> 1
C#:
Place your bets on:
1 - Red
2 - Black
> 2

Or if you are really adventurous, do the same as Lotus 123 style menus:
C#:
var favoriteArtist = ChooseOne<string>("Select your favorite artist: ", "&Enya", "&Queen", "&Dire Straights", "&OMD", "John &Williams");
var rouletteColor = ChooseOne<Color>("Place your bets on:", Color.Red, Color.Black, (color => color.ToString()[0]) );
which would show up as:
C#:
Select your favorite artist: [E]nya, [Q]ueen, [D]ire Straights, [O]MD, John [W]illiams
> e
C#:
Place your bets on: [R]ed, [B]lack
> b
 
Last edited:
C#:
using System;
using System.Collections.Generic;
using System.Text;

namespace FootballLeague
{
    public static class ConsoleManager
    {

        public static int GetInt32(string prompt)
        {
            Console.WriteLine(prompt);

            var isValid = int.TryParse(Console.ReadLine(), out var number);

            while (!isValid)
            {
                Console.WriteLine("Please enter a valid number.");
                Console.WriteLine(prompt);

                isValid = int.TryParse(Console.ReadLine(), out number);
            }

            return number;
        }

        public static double GetDouble (string prompt)
        {
            Console.WriteLine(prompt);

            var isValid = double.TryParse(Console.ReadLine(), out var number);

            while (!isValid)
            {
                Console.WriteLine("Please enter a valid number.");
                Console.WriteLine(prompt);

                isValid = double.TryParse(Console.ReadLine(), out number);
            }

            return number;
        }

        public static bool GetYesNo(string prompt)
        {
            Console.Write($"{prompt} [Y/N]: ");
            while (true)
            {
                char ch = Console.ReadKey(true).KeyChar;
                ch = char.ToUpper(ch);
                switch (ch)
                {
                    case 'Y':
                    case 'N':
                        Console.WriteLine(ch);
                        return ch == 'Y';
                }
            }
        }

        public static string GetLimitedString(string prompt, int minCharacters, int maxCharacters)
        {
            do
            {
                Console.WriteLine(prompt);

                var userString = Console.ReadLine();

                if (userString.Length >= minCharacters && userString.Length <= maxCharacters)
                {
                    return userString;
                }
            } while (true);
        }
    }
}
 
Here is another thing I have noticed

i made the class
public static class ConsoleManager

does this class need to be static?
 
Here is another thing I have noticed

i made the class
public static class ConsoleManager

does this class need to be static?
The point of declaring a class static is to FORCE every member to also be static, which has the effect of preventing the class being instantiated. If you would use a module in VB then you should use a static class in C#. You can declare every member of a regular class static but it can still be instantiated via the default constructor. If a type SHOULD NOT be instantiated then you should declare it static. One example is a class in which extension methods are declared. Such a class MUST be static.
 
Do you think my class should be static?
Im guessing I never want it instantiated...so yes?

Whether or not this class should be static or not is more of a design question I would think. It depends on how you end up using the class. I think if you begin to tire passing the same prompt to these methods that might inspire you to approach it differently.
 
Back
Top Bottom