7
\$\begingroup\$

A method that I'm working on in a C# project includes the following:

if (isDailyRateRequired) 
{
    CalculatedDailyRate = dailyRate;
}

if (isWeeklyRateRequired) 
{
    CalculatedDailyRate = weeklyRate / 7;
}

if (isMonthlyRateRequired) 
{
    CalculatedDailyRate = monthlyRate / 30;
}

There has to be a cleaner implementation than this. I could compress it to one line with a set of nested ternary operators, but that would be pretty much unreadable.

Other suggestions?

\$\endgroup\$
2
  • \$\begingroup\$ I could compress it to one line with a set of nested ternary operators, but that would be pretty much unreadable. - Not necessarily. Please see my answer for a similar question. \$\endgroup\$
    – seand
    Jan 4 '12 at 16:23
  • \$\begingroup\$ Show the code please where the boolean variables is set I mean isDailyRateRequired etc. Show more code or explain what you want to do at all. I think it is need to redesign not only this method with if statements. \$\endgroup\$
    – Serghei
    Jan 5 '12 at 11:37
12
\$\begingroup\$

By looking at your code it appears to me that your basic problem is that isDailyRateRequired, isWeeklyRateRequired and isMonthlyRateRequired are distinct boolean variables instead of an enum. If you just turned them all into an enum things would be a lot more simple, there would be no possibility of inconsistencies between their values, and you would not even have this multiple-choice issue.

\$\endgroup\$
2
  • \$\begingroup\$ Good thought. However, these values are coming out of a DataSet and are stored as three columns in a database. Did I mention I'm working on a legacy app? :) \$\endgroup\$
    – Josh Earl
    Jan 4 '12 at 16:45
  • 1
    \$\begingroup\$ @JoshEarl - yikes! Perhaps you could add a calculated column to your DataTable. You could use the IFF function in the expression in order to convert those 3 boolean values into a single Enum value, or you could define the expression to derive the correct rate directly. \$\endgroup\$ Jan 4 '12 at 18:26
8
\$\begingroup\$

Since those seem to be mutually exclusive conditions, you may not want to execute all 3 ifs every time by putting an else in between:

if (isDailyRateRequired) 
{
    CalculatedDailyRate = dailyRate;
}
else if (isWeeklyRateRequired) 
{
    CalculatedDailyRate = weeklyRate / 7;
}
else if (isMonthlyRateRequired) 
{
    CalculatedDailyRate = monthlyRate / 30;
}
\$\endgroup\$
6
\$\begingroup\$

If these conditions are exclusive conditions you should not use booleans. Use an enumeration with three named constants (or states): DailyRate, WeeklyRate and MonthlyRate. It prevents invalid states where more than one (or zero) boolean are true. (As @MikeNakis already suggested.)

If these three if statements repeating in the code replace the conditional with polymorphism. (See Replacing the Conditional Logic on Price Code with Polymorphism in Refactoring: Improving the Design of Existing Code by Martin Fowler).

Since these booleans comes from a legacy database maybe you want to throw an exception or at least log an error message when more than one (or zero) flags are true.

\$\endgroup\$
0
3
\$\begingroup\$

How about adding a calculated column to your DataTable?

The expression is not exactly simpler, but doing it this way might provide it's own advantages:

new System.Data.DataColumn("CalculatedDailyRate", typeof(decimal), "IIF(isDailyRateRequired, dailyRate, IIF(isWeeklyRateRequired, weeklyRate / 7, IIF(isMonthlyRateRequired, monthlyRate / 30, null)))")

Multi-line version that might be easier to read:

        new System.Data.DataColumn("CalculatedDailyRate", typeof(decimal), @"
            IIF(isDailyRateRequired, dailyRate,
            IIF(isWeeklyRateRequired, weeklyRate / 7,
            IIF(isMonthlyRateRequired, monthlyRate / 30,
            null)))")
\$\endgroup\$
1
  • 1
    \$\begingroup\$ This hides the messiness inside the thing that's causing the messiness. :) Kinda liking this option. The only downside is that it seems like it might be hard to find later. We have a ~1 million line codebase, so discoverability is a consideration. \$\endgroup\$
    – Josh Earl
    Jan 4 '12 at 19:17
2
\$\begingroup\$

You could create a class Interval with subclasses DailyInterval, WeeklyInterval, and MonthlyInterval and then do:

interval = isDailyRateRequired? new DailyInterval(dailyRate):
           isWeeklyRateRequired? new WeeklyInterval(weeklyRate):
           isMonthlyRateRequired? new MonthlyInterval(monthlyRate):
           /* throw exception */;

calculatedDailyRate = interval.DailyRate();

...where the method DailyRate() automatically returns the appropriate rate based on the internally-known interval duration.

Of course, this only makes sense if you'll be using intervals in lots of places.

\$\endgroup\$

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.