5
\$\begingroup\$

I was learning my way on designing real world OOP problems and I was trying to solve this problem in the OOP way. The problem statement is as follows:

  • The conference has multiple tracks each of which has a morning and afternoon session.
  • Each session contains multiple talks.
  • Morning sessions begin at 9am and must finish by 12 noon, for lunch.
  • Afternoon sessions begin at 1pm and must finish in time for the networking event.
  • The networking event can start no earlier than 4:00 and no later than 5:00.
  • No talk title has numbers in it.
  • All talk lengths are either in minutes (not hours) or lightning (5 minutes).
  • Presenters will be very punctual; there needs to be no gap between sessions.

Test input

Writing Fast Tests Against Enterprise Rails 60min
Overdoing it in Python 45min
Lua for the Masses 30min
Ruby Errors from Mismatched Gem Versions 45min
Common Ruby Errors 45min
Rails for Python Developers lightning
Communicating Over Distance 60min
Accounting-Driven Development 45min
Woah 30min
Sit Down and Write 30min
Pair Programming vs Noise 45min
Rails Magic 60min
Ruby on Rails: Why We Should Move On 60min
Clojure Ate Scala (on my project) 45min
Programming in the Boondocks of Seattle 30min
Ruby vs. Clojure for Back-End Development 30min
Ruby on Rails Legacy App Maintenance 60min
A World Without HackerNews 30min
User Interface CSS in Rails Apps 30min

Test output

Track 1:
09:00AM Writing Fast Tests Against Enterprise Rails 60min
10:00AM Communicating Over Distance 60min
11:00AM Rails Magic 60min
12:00PM Lunch
01:00PM Ruby on Rails: Why We Should Move On 60min
02:00PM Common Ruby Errors 45min
02:45PM Accounting-Driven Development 45min
03:30PM Pair Programming vs Noise 45min
04:15PM User Interface CSS in Rails Apps 30min
04:45PM Rails for Python Developers lightning
04:50PM Networking Event

Track 2:
09:00AM Ruby on Rails Legacy App Maintenance 60min
10:00AM Overdoing it in Python 45min
10:45AM Ruby Errors from Mismatched Gem Versions 45min
11:30AM Lua for the Masses 30min
12:00PM Lunch
01:00PM Clojure Ate Scala (on my project) 45min
01:45PM Woah 30min
02:15PM Sit Down and Write 30min
02:45PM Programming in the Boondocks of Seattle 30min
03:15PM Ruby vs. Clojure for Back-End Development 30min
03:45PM A World Without HackerNews 30min
04:15PM Networking Event

I successfully finished designing the problem and it works perfectly but I would love to see the experts opinion on my code and provide suggestions.

timing.py

from datetime import timedelta, datetime


class Timing:
    def __init__(self):
        self.morning_start = (datetime.min+ timedelta(hours=9)).strftime('%I:%M %p')
        self.lunch = (datetime.min+ timedelta(hours=12)).strftime('%I:%M %p')
        self.afternoon_start = (datetime.min+ timedelta(hours=13)).strftime('%I:%M %p')
        self.day_end = (datetime.min+ timedelta(hours=17)).strftime('%I:%M %p')

if __name__ == '__main__':
    a = Timing()
    print(a.afternoon_start)

track.py

from datetime import timedelta, datetime

from .timing import Timing


class Track(Timing):
    id = 0

    def __init__(self):
        super(Track, self).__init__()
        Track.id += 1
        self.talks = {}
        self.talk_list = Track.extract_input()

    @staticmethod
    def extract_input():
        __talks = {}
        lines = []
        try:
            lines = [line.strip() for line in open('test.txt')]
        except FileNotFoundError as e:
            print('File Not Found', e)
        for line in lines:
            title, minutes = line.rsplit(maxsplit=1)
            try:
                minutes = int(minutes[:-3])
            # negative indexing raises error, so it means it's lightning
            except ValueError:
                minutes = 5
            __talks[line] = minutes
        return __talks

    def get_talks(self, start_talk, end_talk):
        start = timedelta(hours=start_talk)
        for key, value in list(self.talk_list.items()):
            prev = start + timedelta(minutes=int(value))
            if prev <= timedelta(hours=end_talk):
                self.talks[(datetime.min + start).strftime('%I:%M %p')] = key
                self.talk_list.popitem()
                start += timedelta(minutes=int(value))
        return self.talks

    def show_output(self):
        while not len(self.talk_list) is 0:
            print('Track %s' % Track.id)
            self.__prepare_output(9, 12)
            print('%s - %s' % (self.lunch, 'Lunch'))
            self.__prepare_output(13, 17)
            print('%s - %s' % (self.day_end, 'Networking Event'))
            Track.id += 1

    def __prepare_output(self, start, end):
        for time, title in sorted(self.get_talks(start, end).items()):
            print(time, '-', title)
        # clear previous entries
        self.talks.clear()


if __name__ == '__main__':
    a = Track()
    a.show_output()
\$\endgroup\$
3
\$\begingroup\$

Here are some general comments:

  • Be more consistent about whitespace, particularly around operators. It will make your code easier to read.

  • I have two suggestions for the way you store instance variables in your Timing objects:

    • Each line is nearly identical. I think it would be better to have a function

      def time_after_start(hours):
          return datetime.min + timedelta(hours=hours)
      

      and call that for each function. It will make the code a little cleaner, and it’ll be easier to see the difference between the lines.

    • I’d keep them as datetime objects, and only cast to strings when you actually need to print them. Use strings for pretty-printing, not for storing structured data. If you need to modify those variables later, you’d have to parse them back from strings first.

      (I know you aren’t changing them here; it’s just a general habit I dislike.)

  • You open test.txt, but never close it again. And you’re creating a list with all the lines, which is inefficient for memory. It would be better to go through the lines one at a time, as follows:

    with open('test.txt') as f:
        for line in f:
            # do stuff with the line
    
    return __talks
    

    This is more memory efficient and idiomatic.

  • I’m not sure why you feel the need to put two underscores on the talks variable; I think it just looks weird and would get rid of it.

  • In get_talks(), you cast the result of self.talk_list.items() to a list, and I’m not sure why you’ve done that. It’s fine to iterate directly over .items() without turning it into a list first.

    Likewise, we know that the values are int’s, because we set them up as such in extract_input(). So casting to int() is just redundant.

  • Rather than iterating over key, value, I’d choose variable names that reflect what those elements in the dictionary mean. How about

    for talk, minutes in self.talk_list.items():
    

    That will make it easier to follow the intent of your code.

  • In show_output(), the if statement is a bit weird. I think you’re trying to say “if there are still elements in self.talk_list, carry on”, which is better expressed as

    if self.talk_list:
    

    I think that’s easier to read.

\$\endgroup\$
2
\$\begingroup\$

First off, negative indexing does work in Python, so the below except block will never execute. Also, when you try to access a list element that doesn't exist, it'll raise an IndexError, not a ValueError.

try:
    minutes = int(minutes[:-3])
# negative indexing raises error, so it means it's lightning
except ValueError:
    minutes = 5

Why is the variable __talks in the method extract_input prefixed with two leading underscores? The variable is local to the function, so there's no need to make it "private".

Finally, add some docstrings to describe your functions/classes. Docstrings are special comments that you use to describe your functions. An example docstring looks something like this:

def my_func( ... ):
    """
    Describe what your function does
    in addition to it's arguments.
    """
    ...
\$\endgroup\$
  • 1
    \$\begingroup\$ Negative indexing works; the test is supposed to catch lightning talks which don't have a time duration like 45minat the end of the line; Negative indexing doesn't raise valueerror, doing int("lightn") does. (Testing for line.endswith('lightning') for lighining talks would be clearer, and keeping exceptions as error handling for misformatted talks with durations would be neater). \$\endgroup\$ – TessellatingHeckler Jul 7 '15 at 19:05

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.