ansaurus

Question

How can I write my program without so many if statements?

Answer 1

+1 A:

(speed >= 50) || (speed <= 54)

If you want this to mean "speed is between 50 and 54", your logic is faulty. You want and, not or.

I don't know what the etiquette is around here in supplying an actual answer as opposed to guiding someone there, but this is the kind of thing you should aim for. I haven't tested it personally though.

static double CalculateFine(int speed, int year)
{
    const double COST_PER_5_OVER = 87.50;
    const int SPEED_LIMIT = 15;
    const double INITIAL_FEE = 75.00;
    double fine = 0;

    if(speed <= SPEED_LIMIT)
    {
        return 0; // No fine imposed
    }

    fine = INITIAL_FEE;

    // Adjust for the different years
    switch(year) {
        case 1:
            fine -= 50;
            break;
        case 2:
            // nowt
            break;
        case 3:
            fine += 50;
            break;
        case 4:
            fine += 100;
    }

    // Add the remaining fine for each 5 miles over the limit
    // XXX: This is slightly different from yours, in that past 125,
    // it'll still keep adding COST_PER_5_OVER
    int perFiveOver = (int)Math.Floor((speed - SPEED_LIMIT) / 5);
    fine += (perFiveOver * COST_PER_5_OVER);

    return fine;

}

Turnor 2009-04-05 19:24:54

I tried this, it doesn't work... I don't know Math.Floor i am gettin an error that says call is ambigious between the methods of System.Math.Floor(double) , (decimal)

Brent 2009-04-05 21:18:32

You can qualify which one you mean with a cast:int perFiveOver = (int)Math.Floor((double)((speed - SPEED_LIMIT) / 5));

Turnor 2009-04-05 21:20:33

Answer 2

A:

Hey, I'm a little confused, should it be

((year == 1) && (speed >= 50) && (speed <= 54))

?

Colin Niu 2009-04-05 19:25:11

Answer 3

A:

I think you're confusing your operator precedence and semantics.

First of all, you want to say (speed >=50) && (speed <= 54)

Second, in C the && has a higher precedence, so your expression as it is means: If (year==1 AND speed>=50) OR (if my speed is lower than 54)...

Third, you have an extra pair of () which suggests that you intended to prioritize things differently.

Finally, I'll mention that code that has a bunch of similar looking if statements is fairly poorly written. A better way to implement it would be either with tables or data structures to represent the ranges, or at least a better organization.

====

Update: Now that you've posted the code, it really looks like you're working too hard on this... First of all, if or switch based on year (there should only be about 4 years). For each year, call a separate function to do the calculation, that will already be cleaner. For example:

if(year==1) return calculateFineForYear1(speed);
if(year==2) return ...

(I don't know C#, but there should be some sort of switch statement)

Now you can manage each year separately. Since your conditions are in an ascending order of speed, you could do something like:

if(speed<50) return 0;
if(speed<55) return ...;
if(speed<60) return ...;
if(speed<65) return ...

It is still far from perfect, but it will already be a lot cleaner and manageable.

Now, looking at your ifs, it looks like you don't even need that many ifs, because you are paying 50 + C*diff, so why not just calculate the diff (speed-50) and then just return the result of the calculation?

For example:

Int total_above_limit = (speed – SPEED_LIMIT);
Int increments_of_5_above_limit = (total_above_limit)/5
Return ( (INITIAL_FEE – 50) + COST_PER_5_OVER*increments_of_5_above_limit))

(I don't know C#, I'm just guessing at the syntax)

Uri 2009-04-05 19:26:08

ok, well I understand that the structure is horrid. I do honestly, I am looking for other ways to write the code. Can I have more suggestions or options ...?

Brent 2009-04-05 19:30:12

Uri can u help me out??

Brent 2009-04-05 19:39:56

@Brent - Calculate how many 5 overs there are, and multiply it by your COST_PER_5_OVER. Then add a modifier based on year to INITAL_FEE.

Mark Brackett 2009-04-05 19:48:24

Hi Brent. Shoot me an email and I'll try to help. [email protected]

Uri 2009-04-05 20:00:08

Uri is a nice guy.

James McMahon 2009-04-05 20:53:26

Answer 4

A:

Not sure I'm 100% sure of what your code is trying to do, but is the problem down to the precedence of the logical operators i.e. should it be:

if (((year == 1) && ((speed >= 50) || (speed <= 54)))) fine = (INITIAL_FEE - 50.00) + (COST_PER_5_OVER * 7);

Note the extra brackets around the speed inequalities.

dommer 2009-04-05 19:26:43

Answer 5

A:

Those if statements make my eyes bleed - and they're inconsistent as well. You have an error in the speed for the first range - being year 1 and having a speed of 15 would get you fined. In some you use a large line of if statements, in others if/elseif statements. There is a difference - especially when you're testing whether speed <= some number. (A bunch of if statements with your || or clause will each evaluate to true, an if/else statements will only evaluate the 1st one). You likely meant && (and) instead of || (or) - but you should replace those if statements for everybody's sanity. ;)

Figure out the formula, and code that - not the results. It looks like it's[1]:

    const double COST_PER_5_OVER = 87.50;
    const int SPEED_LIMIT = 15;
    const double INITIAL_FEE = 75.00;

    if (speed <= SPEED_LIMIT)
    {
         return 0;
    }

    double yearSurcharge;
    switch (year)
    {
        case 1:
           yearSurcharge = -50;
           break;
        case 2:
           yearSurcharge = 0;
           break;
        case 3:
            yearSurcharge = 50;
            break;
        case 4:
            yearSurcharge = 100;
            break;
        default:
            yearSurcharge = 0;
            break;
    }

    const int NUMBER_OF_FIVE_OVER = 21;
    int numberOfFiveOver = Math.Min((speed - SPEED_LIMIT) % 5; MAX_NUMBER_OF_FIVE_OVER)

    return INITIAL_FEE + yearSurcharge + (numberOfFiveOver * COST_PER_5_OVER);

which could be simplified to the following:

    if (speed > SPEED_LIMIT) {
         return INITIAL_FEE 
                + (year == 1 ? -50 : year == 3 ? 50 : year == 4 ? 100 : 0) 
                + Math.Min((speed - SPEED_LIMIT) % 5, MAX_NUMBER_OF_FIVE_OVER) * COST_PER_5_OVER;
    } else {
         return 0;
    }

though some will quibble over the nested ternaries for the year portion.

[1] Technically, you have a $0 fine for anything over 125 - but I don't think that's really what you wanted.

Mark Brackett 2009-04-05 20:10:21

Answer 6

+13 A:

What others have said about operator precedence seems to be correct, but I think there is a bigger issue here. You really shouldn't need zillions of if-statements to model this problem. I'm sure you've found out by now that this method is not maintainable. Trying to change anything in that pile of ifs is going to be a real pain and very error prone.

First thing I would do is separate the calculation for the discount from the calculation for the fine. This way you do not have to manually calculate every possible combination of discount and fine. Here's what I'm talking about:

static double CalculateFine(int speed, int year)
{
    const double COST_PER_5_OVER = 87.5;
    const int SPEED_LIMIT = 15;
    const double INITIAL_FEE = 75;

    // The discount is 50 for each year, scaled so that year 1 is
    // -50, year 2 is 0, and so on.
    double discount = year * 50 - 100;

    // This calculation assumes that speed >= SPEED_LIMIT
    int feeMultiplier = (speed - SPEED_LIMIT) / 5;
    double fine = feeMultiplier * COST_PER_5_OVER + INITIAL_FEE;

    return discount + fine;
}

In the end, the discount and fine are combined only one time. Really, this is just figuring out what formula is used to calculate the fine and then implementing that. If the fine were defined in a more arbitrary manner then perhaps a table would be useful.

Whisty 2009-04-05 20:37:29

The case to int is unnecessary since "speed", "SPEED_LIMIT", and 5 are ints.

Davy8 2009-04-06 00:06:17

I meant cast not case

Davy8 2009-04-06 00:07:08

Duly noted and changed. Thanks.

Whisty 2009-04-06 00:26:09

Answer 7

+1 A:

Well this isn't a direct answer (I think other people have covered that pretty well) you may want to check out the book Code Complete by Steve McConnell. It contains best practices to avoid code like you have above. It could help you out for future projects.

James McMahon 2009-04-05 20:48:28

Answer 8

A:

A table driven approach should work well here. Define the values in a table, then iterate over them with to find the one that matches, then use that for making the computation. This works well and you can easily add/edit/remove entries without touching the code - at all.

You can start with a simple statically allocated and defined table. If needed, you could then make the table load dynamically from another source, such as an XML file.

Foredecker 2009-04-06 01:02:56

The table approach is overkill, as the core fine is proportional to the speed. The maintenance of the table is almost as much work as the maintenance of the original poster's code, and then there is the overhead of loading the XML file and looking things up from the table. The solution supplied by wintermute is shorter and requires less maintenance.

belugabob 2009-08-08 11:34:21

Answer 9

A:

Usually you would use a switch statement if an If statement has atleast one or more 'nested' If statements...

baeltazor 2009-08-08 11:27:23

ansaurus

tags:

views:

answers:

How can I write my program without so many if statements?

related questions