23
\$\begingroup\$

I've just started learning C# using Rob Miles' C# Programming Yellow Book and some related lab exercises from his website. I did one of them and produced a solution that works. In Miles' book, he says that nesting if and else brackets is clumsy and introduces the switch. But switch apparently doesn't work with Boolean operators, e.g. case x < 201, so I can't use it. I could just have independent, non-nested if statements, though their conditions would have to be longer. Any other ideas?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;


class Program
{
    static int GetIntNumber(int min, int max, string prompt, string inputPrompt)
    {
        int number;

        Console.Write(prompt);
        do  //loops as long as input is not between min and max
        {
            while (!int.TryParse(Console.ReadLine(), out number)) //loops as long as the input is not an integer
            {
                Console.Write("\nYou must enter an integer number!: ");
            }
            if (number < min || number > max) Console.Write(inputPrompt); //<--| write input prompt if number out of range
        } while (number < min || number > max);

        return number;
    }

    static void printGrade(string grade)
    {
        Console.WriteLine("\nYour potato is grade " + grade);
    }

    static void Main()
    {

        do
        {
            int weight = GetIntNumber(0, 1500, "\nEnter the weight of the potato in grams: ", "\nEnter an integer between 0 and 1500: ");

            if (weight < 201)
            {
                printGrade("X");
            }
            else
            {
                if (weight < 401)
                {
                    printGrade("A");
                }
                else
                {
                    if (weight < 801)
                    {
                        printGrade("B");
                    }
                    else
                        printGrade("Z");
                }
            }
            Console.WriteLine("\nPress any key to check the grade of another potato.");
        } while (Console.ReadKey(true).Key != ConsoleKey.Escape );

        Console.Read();

    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I don't know a thing about C# which is why I'm only leaving a comment, but surely the language supports the ternary operator? printGrade( weight < 201 ? "X" : weight < 401 ? "A" : weight < 801 ? "B" : "Z" ) \$\endgroup\$ – oals Nov 7 '16 at 9:08
  • \$\begingroup\$ x < 201 is a boolean variable expression. It returns true or false or 0 or 1. It cannot be evaluated as a series of numbers. Since when is 3 in the set of 0 and 1? \$\endgroup\$ – user64742 Nov 7 '16 at 16:33
26
\$\begingroup\$

You are very much correct that switch statements cannot take boolean operators like that: switch statements are used to build jump tables, and can only use constants to do so. However, you can easily come up with a fairly robust solution to your problem using a Dictionary and some LINQ.

Since you tagged your question I'll be gentle with the explanation.

The first thing we'll do is make a Dictionary<int, string> which represents the lowest value associated with each grade.

var gradeMap = new Dictionary<int, string>
{
    [0] = "X",
    [201] = "A",
    [401] = "B",
    [801] = "Z",
};

So, this means if the value is >= 801, then it's Z, and so on.

Next, we'll simply one-line this entire operation in LINQ:

gradeMap.OrderByDescending(x => x.Key).FirstOrDefault(x => x.Key <= testWeight).Value

What's happening here? LINQ is Language Integrated Query, if you've ever worked with SQL you might recognize the Query bit, essentially, LINQ allows you to build code which looks and act's like a query against an IEnumerable object (which Dictionary is).

So if we take this step-by-step, we can start with our dictionary as follows:

0: X
201: A
401: B
801: Z

The gradeMap.OrderByDescending(x => x.Key) bit will first take the entire gradeMap dictionary, and order it by the key value in highest-to-lowest order:

801: Z
401: B
201: A
0: X

Next, we do a FirstOrDefault(x => x.Key <= testWeight), which will loop through all the elements (in the backwards order we have them) that are in the dictionary, and find the first one that matches our expression (x.Key <= testWeight). The x.Key is the reference to the key of the dictionary (our int values):

801: Z (Skipped)
401: B (Skipped)
201: A (Matched)

So, now we have the 201: A item from the dictionary, the last part is to get .Value on it which returns the string portion of it:

A

And viola, we have our value. :)


We can combine this together and get:

var testWeight = 205;
var grade = gradeMap.OrderByDescending(x => x.Key).FirstOrDefault(x => x.Key <= testWeight).Value;

So, the final thing we have to do is combine it with your original programme:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

class Program
{
    static int GetIntNumber(int min, int max, string prompt, string inputPrompt)
    {
        int number;

        Console.Write(prompt);
        do  //loops as long as input is not between min and max
        {
            while (!int.TryParse(Console.ReadLine(), out number)) //loops as long as the input is not an integer
            {
                Console.Write("\nYou must enter an integer number!: ");
            }
            if (number < min || number > max) Console.Write(inputPrompt); //<--| write input prompt if number out of range
        } while (number < min || number > max);

        return number;
    }

    static void printGrade(string grade)
    {
        Console.WriteLine("\nYour potato is grade " + grade);
    }

    static void Main()
    {
        do
        {
            var gradeMap = new Dictionary<int, string>
            {
                [0] = "X",
                [201] = "A",
                [401] = "B",
                [801] = "Z",
            };

            int weight = GetIntNumber(0, 1500, "\nEnter the weight of the potato in grams: ", "\nEnter an integer between 0 and 1500: ");

            printGrade(gradeMap.OrderByDescending(x => x.Key).FirstOrDefault(x => x.Key <= weight).Value);

            Console.WriteLine("\nPress any key to check the grade of another potato.");
        } while (Console.ReadKey(true).Key != ConsoleKey.Escape );

        Console.Read();
    }
}

The next best thing would be to extract the LINQ logic into a new method:

public string GetGrade(int weight)
{
    var gradeMap = new Dictionary<int, string>
    {
        [0] = "X",
        [201] = "A",
        [401] = "B",
        [801] = "Z",
    };

    return gradeMap.OrderByDescending(x => x.Key).FirstOrDefault(x => x.Key <= weight).Value;
}

Then our Main method becomes:

static void Main()
{
    do
    {
        int weight = GetIntNumber(0, 1500, "\nEnter the weight of the potato in grams: ", "\nEnter an integer between 0 and 1500: ");

        printGrade(GetGrade(weight));

        Console.WriteLine("\nPress any key to check the grade of another potato.");
    } while (Console.ReadKey(true).Key != ConsoleKey.Escape );

    Console.Read();
}

Now if you need to add a new grade, for whatever reason, it's trivial.


Also: if you're not using C#6.0, you will have to replace var gradeMap with the following:

var gradeMap = new Dictionary<int, string>
{
    {0, "X"},
    {201, "A"},
    {401, "B"},
    {801, "Z"}
};

Good work so far, hopefully you continue improving and learn more regarding C# and the paradigms it holds. :)


The answer by t3chb0t is also very good, I recommend reading it thoroughly as well (especially the bit about method overloading).

\$\endgroup\$
  • \$\begingroup\$ While I have no technical objections to this this is someone just starting out, I don't really think this is an appropriate answer for their situation. \$\endgroup\$ – Loren Pechtel Nov 7 '16 at 1:57
  • 2
    \$\begingroup\$ @LorenPechtel While it may not be the best answer for this specific user, we should still strive to give those starting out a glimpse (at the very least) into the advanced tools they may encounter in the future, to allow them to see where exploration of a topic may lead. If there's a better way to solve a problem (which this is an attempt to present) then it should be presented for, at the very least, future readers. \$\endgroup\$ – Der Kommissar Nov 7 '16 at 2:00
  • \$\begingroup\$ Hey, this is very interesting, thanks for the reply. I understand more or less how it works. The challenge for me as a begginer of course is the amount of syntax there is to memorize... but I'll try to keep it in mind in the future. \$\endgroup\$ – S. Elliot Perez Nov 7 '16 at 7:50
  • 2
    \$\begingroup\$ @S.ElliotPerez Everyone starts out as a beginner at some point, the trick is to not lose interest. Hopefully you're well on your way to becoming a strong C# programmer. :) \$\endgroup\$ – Der Kommissar Nov 7 '16 at 16:47
19
\$\begingroup\$

You can put else if together.
I doubt that this is any faster, but to me it is cleaner and I like even numbers

if (weight <= 200)
{
    printGrade("X");
}
else if (weight <= 400)
{
    printGrade("A");
}
else if (weight <= 800)
{
    printGrade("B");
}
else
{
    printGrade("Z");
}
\$\endgroup\$
13
\$\begingroup\$

What can be improved?

You can improve your code in several places. Let's start with GetIntNumber.

  • Use helper variables for repeated conditions.
  • Use helper variables to give a condition a meaning. It's better then writing comments.
  • Use positive conditions and negate them if necessary. They're usually easier to understand.

With good helper variables you will not need comments, because now the variable names already explain how long the while is going to run. The improved code could look like this:

static int GetIntNumber(int min, int max, string prompt, string inputPrompt)
{
    int number;
    Console.Write(prompt);
    bool inputInRange;

    do
    {
        while (!int.TryParse(Console.ReadLine(), out number)) //loops as long as the input is not an integer
        {
            Console.Write("\nYou must enter an integer number!: ");
        }

        inputInRange = number >= min && number <= max;

        if (!inputInRange)
        {
            Console.Write(inputPrompt);
        }
    } while (!inputInRange);

    return number;
}

Method overloading

Now, let's do something about the Main, it currently contains important logic.

You can create another PrintGrade method, but this one will require an int. It will encapsulate the print-grade logic and use returns to avoid nesting or ugly if/elses.

Other advice:

  • Use PascalCase for methods.
  • Use constants instead of magic numbers.

Example:

static class WeightGradeMaxValue
{
    public const int X = 200;
    public const int A = 400;
    public const int B = 800;
    public const int Z = -1; // -1 might indicate no limit
}

static void PrintGrade(int weight) 
{

    if (weight <= WeightGradeMaxValue.X)
    {
        PrintGrade("X");
        return;
    }

    if (weight <= WeightGradeMaxValue.A)
    {
        PrintGrade("A");
        return;
    }

    if (weight <= WeightGradeMaxValue.B)
    {
        PrintGrade("B");
        return;
    }

    PrintGrade("Z");
}

Your Main is now nicely short:

static void Main()
{
    do
    {
        int weight = GetIntNumber(0, 1500, "\nEnter the weight of the potato in grams: ", "\nEnter an integer between 0 and 1500: ");

        PrintGrade(weight);

        Console.WriteLine("\nPress any key to check the grade of another potato.");
    } while (Console.ReadKey(true).Key != ConsoleKey.Escape);

    Console.Read();
}

More advanced solution

Even with all the suggested improvements there is still room for more.

The grade weights and symbols can be encapsulated in a new type:

class Grade
{
    public Grade(string symbol, int minWeight) 
    {
        Symbol = symbol;
        MinWeight = minWeight;
    }

    public string Symbol { get; }

    public int MinWeight { get; }

    public bool Includes(int weight)
    {
        return weight > MinWeight;
    }   
}

This new type is now able to check if a weight is within its range. This will help you to search for a particular grade.

Positive logic is usually esier to understand so I flipped it. Instead of having a max-weight I turned it into a min-weight so that we can use all grades and the logic becomes more obvious because you now check if the weight is in this grade a not if it's not in the other.

Creating new grades is now very easy and we pre-sort the array in descending order by MinWeight so that you don't have to do it each time.

private static readonly Grade[] Grades = new Grade[]
{
    new Grade("X", 0),
    new Grade("A", 200),
    new Grade("B", 400),
    new Grade("Z", 800),
}
.OrderByDescending(x => x.MinWeight)
.ToArray();

Searching for a grade is now as simple as to write a single LINQ expression - it's also easy to read because the logic is encapsulated in the Includes method:

static Grade GetGrade(int weight)
{
    return Grades.FirstOrDefault(x => x.Includes(weight)) ?? Grades.Last();
}

where the ?? (the null-coalescing operator) means that if the first part returns null in case nothing were found then use the second part or more technically:

It returns the left-hand operand if the operand is not null; otherwise it returns the right hand operand.

The last step is to exchange the old

PrintGrade(weight);

for the new one

PrintGrade(GetGrade(weight).Symbol);
\$\endgroup\$
  • \$\begingroup\$ Hey, thanks for the reply. I'm a bit confused as to why the PrintGrade method (in your second bit of code) contains instances of itself. Also, as defined, it takes an integer as a value, but you give it a string as an argument... \$\endgroup\$ – S. Elliot Perez Nov 7 '16 at 8:23
  • \$\begingroup\$ @S.ElliotPerez It's called method overloading. You can have multiple methods with the same name as long as their arguments are different. \$\endgroup\$ – t3chb0t Nov 7 '16 at 8:24
5
\$\begingroup\$

A few suggestions:

  • Why calculate it twice (number < min || number > max)
  • You can avoid the if-else if you extract a formula for the requirement i.e if (weight-1)/200 == 0 => 'X'
    == 1 => 'A'
    == 2 => 'B'
    ==3 =>'B'
    others =='Z'

So, you can have an array

grades[5] = {'X','A','B','B','Z'}
factor = (weight-1)/200;
if(factor>=4) { factor = 4;}
return grade[factor];

I know its a bit incomprehensible but its short :)

  • Other option is switch cases

Just realized a simpler solution. - Use a Dictionary with keys representing the upper limit of each range and values having the value for those range i.e

   valuesForRange[200] = 'X';
   valuesForRange[400] = 'A';
   valuesForRange[800] = 'B';
   valuesForRange[1500] = 'Z';

Loop through the keys (like an array) and wherever it matches, get the value.

NOTE: You will be misusing dictionary to act like a 2D array for different datatypes.

\$\endgroup\$
  • \$\begingroup\$ Hey, I'm going through all these in order of length/complexity. Your solution is really nice and compact, thank you. And with some comments, it would be more comprehensible... the only problem is that it becomes more difficult to use when the range values aren't at nice even intervals as in this case. E.g., if it were something like 'X' = 0-257 gm, 'A' = 258-420 gm, 'B' = 421-545 gm, etc., I might have to find another solution. \$\endgroup\$ – S. Elliot Perez Nov 7 '16 at 7:31
  • \$\begingroup\$ "I know its a bit incomprehensible but it's short" - You should always favour comprehensibility over brevity. You gain nothing from code being "short" and incomprehensible, apart from wondering "what the hell does this do again?" when you view it a few days later. \$\endgroup\$ – 404 Nov 7 '16 at 13:38
  • \$\begingroup\$ You should never compromise self-explanatory code over brevity and I conform to that. This solution is to suit the requirement placed :) @Elliot: I have added another solution to make it explanatory and terse. \$\endgroup\$ – thepace Nov 7 '16 at 16:01

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.