4
\$\begingroup\$

This was an interview question, I was supposed to refactor the EventPricingUpdate class to reduce the amount of nested ifs. Here are the basic rules:

  1. EventPricingUpdate() is responsible for updating the price of a list of events after each day.
  2. Each event's price will be reduced by 10 for each day.
  3. The price of each event must be in the range of 50 <= price <= 500.
  4. The field days_from_start represents how many days before the event begins. If days_from_start < 7, then the effect of pricing change will be doubled. For example, a regular event's price will be reduced by 10 * 2 each day in the last 6 days.
  5. There are special event_types that behavors differently.

    1. "music" events' price will always go up by 10 each day, and up by 10 * 2 each day in the last 6 days.
    2. "construction" events' price will never change.
    3. "sports" events' price will drop by double the regular amount(20 instead of 10).
  6. The class should support easily adding new conditions for a new event_type.

As you can see below, I had trouble refactoring the nested ifs in the update() method, what are some good approach to refactor it?

class Event(object):
    def __init__(self, price, days_from_start, event_type):
        self.price = price
        self.days_from_start = days_from_start
        self.event_type = event_type

class EventPricingUpdate(object):
    def __init__(self, events):
        self.events = events

    def update(self):
      for event in self.events:
        if event.event_type == 'construction':
          continue
        elif event.event_type == 'music':
          if event.days_from_start < 7 and event.price <= 480:
            event.price += 20
          elif event.price <= 490:
            event.price += 10
        elif event.event_type == 'sports':
          if event.days_from_start < 7 and event.price >= 90:
            event.price -= 40
          elif event.price >= 70:
            event.price -= 20
        elif event.days_from_start < 7 and event.price >= 70:
            event.price -= 20
          elif event.price >= 60:
            event.price -= 10
        event.days_from_start -= 1
      return self.events
\$\endgroup\$
  • \$\begingroup\$ You mean, if my event_type is sports (for eg.), according to the code above, event.price won't ever drop below 50? \$\endgroup\$ – hjpotter92 Mar 7 '18 at 8:23
1
\$\begingroup\$

After a small check, it seems the best solution is to add a condition per type of event, where you adjust accordingly.

Since all events have their effect 'doubled' on the last 7 days, you can use a single if for it

Lastly, you use a single if as well to check if you can update the event, regarding final price

Also I added a list comprehension to avoid looping through 'construction' events, whose value never changes

class EventPricingUpdate(object):
    ORIGINAL_REDUCTION = 10

    def __init__(self, events):
        self.events = events

    def events_to_update(self):
        return [event for event in self.events if event.event_type != 'construction']

    def update(self):
        for event in self.events_to_update():
            reduction = self.ORIGINAL_REDUCTION

            if event.event_type == 'music':
                reduction = reduction * -1

            if event.event_type == 'sports':
                reduction = reduction * 2

            if event.days_from_start < 7:
                reduction = reduction * 2

            # if you use python 2 this if needs a slight change
            if 500 >= event.price - reduction >= 50:
                event.price -= reduction

            event.days_from_start -= 1

        return self.events


# Fill some events
events = [
    Event(event_type='construction', days_from_start=10, price=100),
    Event(event_type='music', days_from_start=10, price=100),
    Event(event_type='sports', days_from_start=10, price=100)
]

EventPricingUpdate(events).update()

# Check update
assert events[0].price == 100
assert events[1].price == 110
assert events[2].price == 80

As you can see, is easy to plug new events, just a new condition on the loop

If you want to go further on the refactoring, you will add the reduction on the list comprehension method

\$\endgroup\$
  • 1
    \$\begingroup\$ I can't improve this answer.. ahh. can't it be [event for event in self.events if event.event_type != 'construction'] ? \$\endgroup\$ – Avinash Raj Mar 9 '18 at 6:16
  • \$\begingroup\$ Indeed, nicely spotted! \$\endgroup\$ – A. Romeu Mar 9 '18 at 8:19

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.