2
\$\begingroup\$

I must create a program for information regarding schedules and topics of meetings at work. The data relating to the meetings are stored in an "M" matrix with 5 columns, in which the generic row [g, m, in, fin, arg] denotes the fact that a meeting has been scheduled for day "g" of month "m", from hours "in" to "fin" hours, on the topic "arg". Write a function "common_argument(M)" that returns the topic to which more time is dedicated. I wrote the function and correctly returns the element but is it ok or is it too long?

def common_argument(M):
    element = M[0][4]
    for i in range(len(M)):
        if M[i][4] != element:
            if period(M, element) < period(M, M[i][4]):
                element = M[i][4]
    return element



 def period(M,c):
        s = 0
        for i in range(len(M)):
            if c in M[i]:
                if M[i][2] != M[i][3]:
                     if M[i][2] > M[i][3]:
                         s += M[i][2] - M[i][3]
                     if M[i][3] > M[i][2]:
                         s += M[i][3] - M[i][2]
         return s


common_argument([[10, 1, 15, 17, 'Vendite'],
    [10, 1, 17, 18, 'Progetti'],
    [16, 2, 10, 12, 'Vendite'],
    [16, 2, 13, 15, 'Progetti'],
    [5,3,10, 13, 'Progetti']])
\$\endgroup\$
2
\$\begingroup\$

This can be radically simplified (and made much more legible) in a variety of ways.

First off, the you use 5 lines of code to compute the duration of a meeting, including checking in advance if that duration will be 0 (if M[i][2] != M[i][3]). You don't care if the duration is 0 (s += 0 leaves s unchanged), and the rest of that logic just ensures that you get a positive difference even if the order of those values is backwards. Technically, the definition of a "meeting" that you give doesn't allow those values to be backwards, so it should be safe to simply subtract the start from the end time; however, even if you want to ensure you end up with a positive value, you can just use abs (absolute value):

def period(M,c):
    s = 0
    for i in range(len(M)):
        if c in M[i]:
            s += abs(M[i][3] - M[i][2])
    return s

Next, don't iterate over the indices of a list when you can just iterate over the list. Why do you care about i? You don't, you only want the value M[i], which is much more cheaply and clearly obtained by simply looping on M:

def period(M,c):
    s = 0
    for meeting in M:
        if c in meeting:
            s += abs(meeting[3] - meeting[2])
    return s

Also, you check if c in m, but that's not really what you mean: you actually want to know if the topic c is the topic that meeting m is about. While we're at it, let's also unpack exactly what a meeting is expected to be, rather than use arbitrary integer indices into a list:

def period(M,c):
    s = 0
    for _, _, start, fin, topic in M:
        if c == topic:
            s += abs(fin - start)
    return s

That's become much more clear now that we've given every value a name (rather than just a number) and been explicit about what we're doing.

Alright, let's tackle the main function now. You iterate over every meeting, and compare it to every other meeting, but this isn't strictly necessary. It also risks doing repeated work (e.g., if you have one REALLY long meeting about 'A' and a thousand tiny ones about 'B', you'll compute the total meeting durations for 'B' a thousand times, and actually do the same with 'A'). Let's first figure out all unique topics, and that way we'll be sure to not re-do any work:

def common_argument(M):
    all_topics = {topic for _, _, _, _, topic in M}
    element = all_topics.pop()
    for other_topic in all_topics:
        if period(M, element) < period(M, other_topic):
            element = other_topic
    return element

Well, that still doesn't prevent us from not duplicating work... let's make sure that we only compute the period of each topic once:

def common_argument(M):
    topic_duration = {topic: period(M, topic) for _, _, _, _, topic in M}
    element, element_duration = ...  # What to put here?
    for other_topic, other_duration in topic_duration.items():
        if element_duration < other_duration:
            element = other_topic
    return element

Alright, we're now only compute each duration once. But the rest of that function is just asking "What is the max topic, sorted by duration?" And, in Python, there's a much easier way to ask that:

def common_argument(M):
    topic_duration = {topic: period(M, topic) for _, _, _, _, topic in M}
    return max(topic_duration, key=topic_duration.get)

The function topic_duration.get takes a topic and returns its duration, and iterating over topic_duration yields each unique topic once, so this will give us exactly what we want.

The one thing we could still improve is that we're iterating over M once for each topic, yielding a runtime of O(N*T) where N = len(M) and T is the number of unique topics. There's no reason for that, we could easily loop over the array just once. That would look more like the following:

from collections import defaultdict

def common_argument(M):
    topic_duration = defaultdict(int)
    for _, _, start, fin, topic in M:
        topic_duration[topic] += abs(fin - start)

    return max(topic_duration, key=topic_duration.get)

And voila, we've collapsed all of the same logic into effectively 4 lines of code, and reduced computational complexity significantly.

\$\endgroup\$
  • 1
    \$\begingroup\$ Consider using collections.Counter instead of defaultdict. That way, you can use .most_common() instead of max(). \$\endgroup\$ – 200_success Feb 17 '19 at 9:41
-2
\$\begingroup\$

You can keep the nesting small by using continue if the condition is not the right one.

def common_argument(M):
    element = M[0][4]
    for i in range(len(M)):
        if M[i][4] == element:
            continue
        if period(M, element) < period(M, M[i][4]):
            element = M[i][4]
    return element


 def period(M,c):
    s = 0
    for i in range(len(M)):
        if c not in M[i]:
            continue;

        if M[i][2] == M[i][3]:
            continue;

        if M[i][2] > M[i][3]:
            s += M[i][2] - M[i][3]
        if M[i][3] > M[i][2]:
            s += M[i][3] - M[i][2]
    return s

See the Zen of Python: Flat is better than nested.

\$\endgroup\$
  • 2
    \$\begingroup\$ I don't expect everyone else on CodeReview to fully re-write the OP's code, but really? You looked at that mass of code and thought "Hm, if only he made that one loop less intuitive"? \$\endgroup\$ – scnerd Feb 16 '19 at 16:13
  • 1
    \$\begingroup\$ For the record, I can respect "flat is better than nested," but that's not really the point in this case. The point is that his actual logic is redundant and simple, it's just his implementation that's verbose. "Explicit is better than implicit. Simple is better than complex." He should declare what he's doing clearly (e.g. using "max" and "key=duration") and rely on simpler logic rather than doing everything the hard way (e.g. using "abs") \$\endgroup\$ – scnerd Feb 18 '19 at 0:21

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.