6
\$\begingroup\$

I have 2 if-else statements that checks whether a booking(object mb) can be cancelled or not. If the current user is an admin or department admin, the booking can be cancelled. However, if the booking has been cancelled, of course, it absolutely cannot be cancelled by anyone.

What I am worried about if the if-else statements can be improved further. Any help would be useful.

foreach (var ub in usersBookings)
{
    MyBookingModel mb = new MyBookingModel();
    mb.bookId = ub.bookingId.ToString() + "+" + ub.startDateTime.ToString();
    mb.bookDate = ub.startDateTime.ToString("dd/MM/yyyy");
    mb.bookSlot = ub.startDateTime.ToString("HH:mm") + " - " + ub.endDateTime.ToString("HH:mm");
    mb.facilityName = ub.facilityName;
    mb.status = ub.bookingStatus.Trim();
    mb.startDateTime = ub.startDateTime.ToString("yyyy-MM-dd HH:mm:00");

    mb.canCancel = false;

    if (mb.status == "Cancelled")
    {  
        //absolute. if mb.status == "Cancelled", mb.canCancel is "false"
        mb.canCancel = false;
        goto add_object;
    }

    if (deptAdmin == true || isAdmin == true)
    {
        mb.canCancel = true;
    }

add_object:
    myBookings.Add(mb);
}
\$\endgroup\$
0

5 Answers 5

7
\$\begingroup\$

You can simply assign the if conditions to the canCancel property of the MyBookingModel like so

mb.canCancel = mb.status != "Cancelled" && ( deptAdmin || isAdmin );

While we are at the properties of the MyBookingModel: Properties should be named using PascalCase casing.

canCancel -> CanCancel and so on.

and while we are at naming things, why is it MyBookingModel and not just BookingModel ? What does the My buy you ?

\$\endgroup\$
1
  • \$\begingroup\$ The properties' format is a "bad habit" carried over from learning Java in school. I do agree yours look better. As for the model, this particular view just requires certain properties to be displayed and BookingModel has a tons of other properties that either do not have the correct data types or will not be used in this particular view. All in all, thank you for your suggestions! \$\endgroup\$
    – Adam Loh
    Jul 11, 2017 at 4:31
6
\$\begingroup\$
if (mb.status == "Cancelled")
{  
    //absolute. if mb.status == "Cancelled", mb.canCancel is "false"
    mb.canCancel = false;
    goto add_object;
}

if (deptAdmin == true || isAdmin == true)
{
    mb.canCancel = true;
}

If you are interested in reusability, testability and maintability then this solution is none of them. I guess this is not the only place in your code where you are checking if someone can do something. You should replace it with some kind of a role/authorization system and rules for each action etc. that you can apply whereever you need them instead of writing the same or similar conditions all over the place... just imagine what happens if there is a new role that can not only cancel bookings but also do other things. You'll have to adjust probably several methods. This is a maintenance-hell.

\$\endgroup\$
1
  • \$\begingroup\$ Appreciate the advice. Will keep this in mind! \$\endgroup\$
    – Adam Loh
    Jul 11, 2017 at 10:09
4
\$\begingroup\$
  • Those if/goto statements boil down to the following: a booking cannot be canceled, unless you've got admin rights and the booking has not been canceled already. In other words: can-cancel = (has-admin-rights AND status is not canceled). A boolean expression and assignment is sufficient here, but I'd probably still use an if/else statement to make it more obvious that there's an access check here and not just a simple value assignment.
  • I'd recommend using a status enum. That restricts it to only valid statuses, and gives you a single point that describes all valid booking statuses. It'll make your code-base easier to understand and maintain. Use the type system to your advantage.
  • The same goes for the other fields. They're much more difficult to work with when they're 'stringly-typed' rather than strongly typed.
  • If you're using string fields here for display purposes, then try moving that to a dedicated display/view layer. Sometimes a model can be displayed in multiple different ways, and often text and dates must be localized. You don't want those things to complicate your models.
  • It looks like MyBookingModel could use a constructor, assuming that fields like its ID and status should not be left empty. Think about your class invariants.
  • This has been mentioned already, but goto is a low-level construction that has almost no place in most modern-day code. if/else statements, loops with continue/break and function calls with return are safer and generally easier to follow.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Additionally, the constructor shouldn't have a long argument list that takes in a value for every property. Use the constructor for those values that absolutely have to be filled or don't have sensible default values such as the ID then use an object initialiser for the rest of the properties. \$\endgroup\$ Jul 11, 2017 at 11:58
2
\$\begingroup\$

Using goto is a very bad practice. The rule of thumb is that you never need to use it. And in this case it's just a matter of replacing the latter if with an else if clause.

Other than that, I'd follow advice from the other answers - @Heslacher's in the short term, @t3chb0t's in the long term.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ Using "goto" without any very good reason whatsoever is a very bad practice. \$\endgroup\$
    – gnasher729
    Jul 11, 2017 at 9:53
  • \$\begingroup\$ @gnasher729 obviously. I haven't yet encountered a case where I would need it, though, so I'm pretty sure a person who needs a review for several if clauses (no offense intended, of course, everyone has to start somewhere) has probably not hit an issue where they would actually need it either. That's why I said it's a 'rule of thumb'. And in OP's case the goto is clearly not needed. \$\endgroup\$
    – user622505
    Jul 11, 2017 at 23:41
0
\$\begingroup\$

In this case, I think Heslacher's one line code is the best solution. In more complicated cases, you can write code like

    if (mb.status == "Cancelled")
    {  
        //absolute. if mb.status == "Cancelled", mb.canCancel is "false"
        mb.canCancel = false;
    }
    else if (deptAdmin == true || isAdmin == true)
    {
        mb.canCancel = true;
    }
    else
    {
        mb.canCancel = false;
    }

In newer languages, it is preferred to have constants when possible, and constants can only be set once. Getting used to this pattern now will help you in the future. (Heslacher's code is equally fine in this respect).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.