# 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 < 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; }  • This does't use any nested statements. Apr 14, 2016 at 4:30 ## 2 Answers 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.

• Wow, I learned more from your comment than a semester of school :) thank you a lot Apr 14, 2016 at 5:31
• @david my pleasure - note that an unsaid takeaway here, is that code gets much simpler when it's separated from the data. Apr 14, 2016 at 19:29

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;
}
}

• String.Equals(String, String, StringComparison) + StringComparison.CurrentCultureIgnoreCase >>>>>>>>>>>>>> String.ToLower(). Equals with a comparison is so much more expressive and precise than lowering before comparing. Apr 14, 2016 at 15:19
• @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? Apr 14, 2016 at 16:22
• "WHITE".ToLower()=="white" in the Turkish locale Apr 14, 2016 at 16:40
• @Johnbot I wouldn't consider that to be a failure, though. Apr 14, 2016 at 16:42