Converting events in an agenda to textual format

Question

Here is a serializer. My concern is to make the convert_to_internal function more pythonic, efficient and ideally more functional. Maybe I could also get rid of three nested loops.

class TimeSerializer(serializers.Field):
    DAYS = {
        0: 'monday',
        1: 'tuesday',
        2: 'wednesday',
        3: 'thursday',
        4: 'friday',
        5: 'saturday',
        6: 'sunday',
        7: 'holidays'
    }
    INVERSE_DAY = {v: k for k, v in DAYS.items()}

    def __day_comparator(day_1, day_2):
       """
       A helper to sort days, so they're sorted 
       as they appear in a week calendar.
       """
       inverse_dict = TimeSerializer.INVERSE_DAY
       return inverse_dict[day_1[0]] - inverse_dict[day_2[0]]

    def convert_to_internal(self, agenda):                                          
        x = []                                                                      
        t = OrderedDict(sorted(agenda['timconf'].items(),                           
                               key=TimeSerializer.__day_comparator))                
        for day, items in t.items():                                                
            day_num = TimeSerializer.INVERSE_DAY[day]                              
            for item in sorted(items, key=lambda k: k['begin']):                    
                for attr in ['begin', 'end']:                                       
                    if hasattr('item', attr):                                       
                        item[attr] = item[attr] + day_num*24*60*60                                   
                x.append(TimeItem(**item))                                          
        p = ';'.join(map(str, x))                                                   
        return "-t {} -n {} -p '{}'".format(agenda['timeZone'], agenda['locale'], p)

Answer 1

This is very hard to read. What's an internal, why are we converting to it? What's an agenda? This is clearly a class method so I'm missing some context but it's not a self explanatory function. You could improve this with proper naming. x and t are meaningless. For instance, if x was appointment_times it would tell me much more.

Likewise, your code is not broken up at all. It's one solid block, which makes it harder to see what each individual section does or even is. Adding whitespace will make it much clearer immediately:

def convert_to_internal(self, agenda):                                          
    x = []                                                                      
    t = OrderedDict(sorted(agenda['timconf'].items(),                           
                           key=TimeSerializer.__day_comparator))                

   for day, items in t.items():                                                
        day_num = TimeSerializer.DAYS_INVERSE[day]

        for item in sorted(items, key=lambda k: k['begin']):                    
            for attr in ['begin', 'end']:                                       
                if hasattr('item', attr):                                       
                    item[attr] = item[attr] + day_num*24*60*60                                   

            x.append(TimeItem(**item))                                          

    p = ';'.join(map(str, x))                                                   
    return "-t {} -n {} -p '{}'".format(agenda['timeZone'], agenda['locale'], p)

Answer 2

def convert_to_internal(self, agenda):                                          
    x = []                                                                      
    t = OrderedDict(sorted(agenda['timconf'].items(),                           
                           key=TimeSerializer.__day_comparator))    
    for day, items in t.items():    

First of all, from what I can see, you're only using t once: in the loop.

Instead, I'd suggest writing

    t = sorted(agenda['timconf'].items(), key=TimeSerializer.__day_comparator)
    for day, items in t:

That gets rid of the OrderedDict.

Let's look at the following piece of code:

            for attr in ['begin', 'end']:                                       
                if hasattr('item', attr):                                       
                    item[attr] = item[attr] + day_num*24*60*60   

What does this do? Of course! It shifts an item by a given number of days.

But it also does some funny things. It checks if item has an attribute (.attribute), but then modifies the value at a key ([key]). So item is some weird attribute dictionary-ish thing. Let's use all that knowledge together.

def shift_time_item(time_item, num_days):
    delta = num_days * 24 * 60 * 60
    try:
        time_item.begin += delta
    except AttributeError:
        pass

    try:
        time_item.end += delta
    except AttributeError:
        pass

Now, you might think it is not DRY, because I repeat a similar try/except, but it is. And it does not really matter, because the business logic is there only once: I specify how to move an item just once.

        day_num = TimeSerializer.INVERSE_DAY[day]                              
        for item in sorted(items, key=lambda k: k['begin']):
            shift_time_item(item, num_days)                                   
            x.append(TimeItem(**item))

Now, I'm left wondering what to do about the x. Why not

items = sorted(items, key=lambda k: k.begin)
for item in items:
    shift_time_item(item, num_days)

And then use items instead of x.

    p = ';'.join(map(str, items))                                                   
    return "-t {} -n {} -p '{}'".format(agenda['timeZone'], agenda['locale'], p)