Multiple if statements in a single method to enable/disable feature

Question

I am sure this is not ideal code, basically I am checking a condition at a time and exiting if any of them are true. This will then disable refund functionality.

public bool ShouldChangeBookingBeDisabled(bool isLoggedIn, bool hasRefundedBookingsAtCinema, VistaBooking booking, SitecoreCinema cinema, ODataAttribute experience)
 {
 if (hasRefundedBookingsAtCinema || !isLoggedIn) return true;
 if (booking.Payments.Any(x => x.CardType != VALID_PAYMENT_TYPE)) return true;
 if (BookingHasVouchers(booking)) return true;
 if (booking.LoyaltyPointsCost != null && booking.LoyaltyPointsCost.Any(x => x.Points > 0)) return true;
 if (DateTime.Now > booking.Tickets[0].SessionDateTimeOffset.AddHours(-cinema.Settings.ChangeBookingGracePeriod)) return true;
 if (experience == null) return false;
 bool isValidAttribute = cinema.Settings.ChangeBookingExperienceFilter.Any(x => x.VistaAttributeCode == experience.ShortName);
 return !isValidAttribute;
 }

Answer 1

Rather than having a lots of if - return statements you can separate the two.

You can define a collection of conditions:

var conditions = new Func<bool>[]
{ 
 () => hasRefundedBookingsAtCinema || !isLoggedIn,
 () => booking.Payments.Any(x => x.CardType != VALID_PAYMENT_TYPE),
 () => BookingHasVouchers(booking),
 () => booking.LoyaltyPointsCost != null && booking.LoyaltyPointsCost.Any(x => x.Points > 0),
 () => DateTime.Now > booking.Tickets[0].SessionDateTimeOffset.AddHours(-cinema.Settings.ChangeBookingGracePeriod)
};

• Since we have defined them as functions that's why they can be lazily evaluated
• For example: Evaluate the first, if it false then evaluate the second and so ...

foreach (var condition in conditions)
{
 if (condition()) return true;
}

The last two statements of your original implementation can be combined into a single while using the conditional operator:

return experience != null ?
 !cinema.Settings.ChangeBookingExperienceFilter.Any(x => x.VistaAttributeCode == experience.ShortName)
 : false;

• Matthew Pigram: I like this one!

Answer 2

It's readable and straight forward. You can also make things simpler by implementing the methods inside the class itself or using method extensions to gather all common conditions to have a more readable, reusable, and easy to access code.

So for instance, gathering VistaBooking flags inside the VistaBooking class or using extension methods would give us this result :

public bool ShouldChangeBookingBeDisabled(bool isLoggedIn, bool hasRefundedBookingsAtCinema, VistaBooking booking, SitecoreCinema cinema, ODataAttribute experience)
{
 return 
 (hasRefundedBookingsAtCinema || !isLoggedIn) 
 && !booking.HasValidPaymentType()
 && !booking.HasVouchers() 
 && !booking.HasLoyaltyPoints()
 && !booking.HasValidTicket()
 && (experience != null && !cinema.Settings.ChangeBookingExperienceFilter.Any(x => x.VistaAttributeCode == experience.ShortName));
}

Notice that the conditions where inverted, to give a clear readability, as it might be reused to validate some parts of code in other areas.

You can also simplify your work further by using a class service or manager that would be scoped to certain purpose. For instance, if you have some automated services that would control feature settings, you can implement a service class and gather all required objects, then implement the required logic to handle each feature, so in the end you have a simple access with minimum arguments.

The main target should be always to minimize the required arguments (either in methods or constructors), and let the code handle the work itself. This would improve the code maintainability and stability, and reduce bugs, and overall errors that comes from unhandled dependency objects.

Answer 3

It is simple, which is good. No crazy giant blocks of logic, which is good. I will say I'm not really a fan of multiple exit points in a method though.

The most important thing is to write functioning code that can be easily understood by other developers; that is ideal.

• Matthew Pigram: I am not a fan of that either, the main reason I am doing it this way is to not process any more conditions if it is already identified as an invalid booking. I have tried to order it test the most likely cases first.

Answer 4

I'm not a c# programmer, but Java (which I do program in) is probably similar enough, and much of what I say shouldn't be language-specific.

• It's a matter of personal taste, but I'm quite happy to have multiple returns in this sort of context.
• And this is one of the few occasions where I'd not bother with braces round the statements controlled by if.
• I'm not found of compound conditions when they're avoidable, so I'd split if (hasRefundedBookingsAtCinema || !isLoggedIn) return true; into two separate tests.
• You have multiple reasons for disabling the booking, many of which are based on the VistaBooking object. I tend to regard methods which make lots of calls to methods in some other object as a bit of an "anti-pattern", probably indicating that that object should be taking that responsibility.
• Similarly, the final test a) uses an unnecessary boolean variable and b) seems to dive too deep into the SitecoreCinema object. Again, I'd want the SitecodeCinema object to deal with that responsibility.

So I'd probably prefer the shape of the code to be more like this:

public bool ShouldChangeBookingBeDisabled(bool isLoggedIn, bool hasRefundedBookingsAtCinema, VistaBooking booking, SitecoreCinema cinema, ODataAttribute experience)
 {
 if (hasRefundedBookingsAtCinema) return true;
 
 if (!isLoggedIn) return true;
 if (booking.disableChanges()) return true;
 
 if (experience == null) return false;
 return cinema.disableChanges(experience);
 }