Determining which vehicle Rob wants to buy

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 = 2000)
        return true;
    else if (d_Price = 2001)
        return true;
    else if (d_Price = 2006)
        return true;
    else if (d_Price = 2010)
        return true;
    else return false;
}

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

StackExchange Code Review Q#125637, answer score: 13