5
\$\begingroup\$

Rob is looking to buy a used pickup truck. He will spend a maximum of $10,000 but would like to spend as little as possible. He will spend up to $3000 for a 2000 or older model, $5000 for a 2001-05 model, $7000 for a 2006-09 model, $9000 for a 2010 or newer model. Rob will not buy a red truck but he will pay 20% more for a white truck.

Given the information above and using C# or VB.net, write a function using nested if/else statements that receives the following variables:

d_Price decimal

s_Color string

i_Year integer

...and returns a Boolean value to determine whether a truck is one Rob wants to buy.

I don't think is a good approach. How can this be improved?

 //Function to decide if the car is what Rob wants
static bool Rob_Wants(decimal d_Price, string s_Color, int i_Year)
{
    decimal [] max_array = {3000, 5000, 7000, 9000};  //max prices

    //if/else to check the condition
    if (s_Color == "Red" || d_Price>10000)
 return false;
    else if (s_Color == "White")
    {
        for (int i = 0; i < max_array.Length; i++)
            max_array[i] = max_array[i]+((max_array[i] * 20) / 100);
    }
    if (d_Price <= max_array[0] && i_Year >= 2000)
        return true;
    else if (d_Price <= max_array[1] && i_Year >= 2001)
        return true;
    else if (d_Price <= max_array[2] && i_Year >= 2006)
        return true;
    else if (d_Price <= max_array[3] && i_Year >= 2010)
        return true;
    else return false;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ This does't use any nested statements. \$\endgroup\$ – Der Kommissar Apr 14 '16 at 4:30
13
\$\begingroup\$
decimal [] max_array = {3000, 5000, 7000, 9000};  //max prices

Let's try something:

int[] maxPrices = {3000, 5000, 7000, 9000};

Or even:

var maxPrices = new[] {3000, 5000, 7000, 9000};

The cutoff prices in the problem statement are all integers, so you don't need to bother with a decimal and floating-point errors here. And since you're using an array initializer to initialize a local variable with a handful of integers, the type of maxPrices is obviously int[].

Notice also, that you stop needing the comments when the code can speak for itself: the informative and useful part of max_array is max - the rest of the useful meaning is hidden in a comment. Seeing // max prices next to a variable declaration tells me one thing: that this comment wouldn't need to be there if the variable was called maxPrices (notice camelCase).

Convention for locals and parameters, is camelCase. Your code suffers from a severe Notationis Hungarianum - symptoms being that d_ for "decimal", that s_ for "string" and that i_ for "integer". Don't do that. Here's why:

vUsing adjHungarian nnotation vmakes nreading ncode adjdifficult.


write a function using nested if/else statements

Your function is structured like this:

  • if
  • else if
    • for
  • if
  • else if
  • else if
  • else if
  • else

I see no nested if conditions. And you know what? That's awesome: nested ifs are usually not pretty, you'd normally want to avoid resorting to them.

Nested conditions would look like this:

  • if
    • if
      • if
      • else
    • else if
      • if
      • else
    • else
  • else

If doing that exercise should teach you one thing, it's to recognize that shape in your code. That shape has a name: it's arrow code, and it's usually not the code you want to be maintaining.

So, what then?


He will spend:

  • up to $3000 for a 2000 or older model
  • up to $5000 for a 2001-05 model
  • up to $7000 for a 2006-09 model
  • up to $9000 for a 2010 or newer model

Also, he:

  • will not buy a red truck
  • will pay 20% more for a white truck
  • will pay no more than $10,000

I think I'd use the built-in Color enum, just because it's there and appropriate for this.

I'd first eliminate the red truck, and return early:

if (color == Color.Red || price > 10000) { return false; }

And your code does exactly that. Well done!

Now the fun part. You went with arrays, and a if..else if construct. The problem is that when you read the code you can't help but notice the [0], the [1], the [2] and the [3] one under the other, all in order.

If I could map a year to the maximum price I'm willing to pay for, my life would be much simpler!

var maxPrices = new Dictionary<int,int> 
{
    { 2000, 3000 },
    { 2005, 5000 },
    { 2009, 7000 },
    { 9999, 9000 },
};

Then we can iterate the dictionary entries, return true and break out when the parameters compare favorably against the Key (year) and the Value (maximum price)

var result = false;
foreach(yearPrice in maxPrices.OrderBy(item => item.Key)) // note: item.Key is the year
{
    var maxPrice = yearPrice.Value;
    if (color == Color.White) 
    { 
        maxPrice *= 1.2; 
    }

    if (year <= yearPrice.Key && price <= maxPrice)
    {
        result = true;
        break;
    }
}
return result;

I don't understand the requirement for nested if statements. Specs usually tell you what to do, not how to do it. And imposing nested constructs (or anything really) sounds like some kind of "coding game" - however if you want to be writing professional code, that isn't the kind of code you want to be writing. Or rather, maintaining.

| improve this answer | |
\$\endgroup\$
  • 6
    \$\begingroup\$ Wow, I learned more from your comment than a semester of school :) thank you a lot \$\endgroup\$ – david Apr 14 '16 at 5:31
  • \$\begingroup\$ @david my pleasure - note that an unsaid takeaway here, is that code gets much simpler when it's separated from the data. \$\endgroup\$ – Mathieu Guindon Apr 14 '16 at 19:29
5
\$\begingroup\$

Your code is wrong: it rejects any car that is older than year 2000.

The max_array isn't providing any benefit to you. All it does is make the code more cryptic, with max_array[0], etc. It would be better to write the code in a way that more closely associates the each price limit with its corresponding age.

The spec does not specify whether the color will start with an uppercase or lowercase letter. I would normalize it to lowercase just in case.

Dropping braces is a dangerous software engineering practice. If you really want to omit braces, put the statement on the same line to help prevent coding accidents.

Here is a function that conforms to the spec:

static bool Rob_Wants(decimal d_Price, string s_Color, int i_Year)
{
    if (d_Price > 10000 || s_Color.ToLower() == "red")
    {
        return false;    // over budget or wrong color
    }
    else
    {
        decimal colorPremium = (s_Color.ToLower() == "white") ? 1.2m : 1.0m;

        if      (i_Year <= 2000) return d_Price <= colorPremium * 3000;
        else if (i_Year <= 2005) return d_Price <= colorPremium * 5000;
        else if (i_Year <= 2009) return d_Price <= colorPremium * 7000;
        else                     return d_Price <= colorPremium * 9000;
    }
}

I'm not at all a fan of the onerous Hungarian prefixes d_, s_, and i_ in the spec, and recommend dropping them altogether.

You have wisely chosen to ignore the nested if/else statements part of the problem statement. Personally, I would ignore that part of the spec too. In fact, I would prefer using a ternary conditional and a switch instead.

The underscore in the function name is unconventional.

With those considerations, I would further rewrite the code:

static bool RobWants(decimal price, string color, int year)
{
    if (price > 10000) return false;              // Over budget
    decimal maxPrice = (year <= 2000) ? 3000 :
                       (year <= 2005) ? 5000 :
                       (year <= 2009) ? 7000 :
                                        9000;
    switch (color.ToLower())
    {
    case "red":
        return false;
    case "white":
        return price <= 1.2m * maxPrice;
    default:
        return price <= maxPrice;
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ String.Equals(String, String, StringComparison) + StringComparison.CurrentCultureIgnoreCase >>>>>>>>>>>>>> String.ToLower(). Equals with a comparison is so much more expressive and precise than lowering before comparing. \$\endgroup\$ – Johnbot Apr 14 '16 at 15:19
  • \$\begingroup\$ @Johnbot In the general case, you're right that String.ToLower() is not a good way to do case-insensitive comparisons. However, here we are looking for fixed strings. Is there any case where this fails? \$\endgroup\$ – 200_success Apr 14 '16 at 16:22
  • \$\begingroup\$ "WHITE".ToLower()=="white" in the Turkish locale \$\endgroup\$ – Johnbot Apr 14 '16 at 16:40
  • \$\begingroup\$ @Johnbot I wouldn't consider that to be a failure, though. \$\endgroup\$ – 200_success Apr 14 '16 at 16:42

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.