8
\$\begingroup\$

You have a file that describes historic data of the London Underground showing dates when stations have been unavailable. The file contains these fields, separated by a pipe character:

  • Date: Format of this is mm-dd-yyyy.
  • Line: Each line has a unique name. For example, “Northern”.
  • Station: Each station has a unique name. For example, “Angel”.
  • Notes: Describes the outage

Here is an example of one entry in the file, which shows that the Northern Line was not operating at Bank on 23 August 2019 due to a function at London Bridge.

08-23-2019|Northern|Bank |No service on Bank branch due to birthday booking at London Bridge station.

Write a script that solves the following problems:

  1. Display each station that was affected by an outage on 10 July 2019.

  2. Between 10 July 2017 and 14 November 2017, which lines had outages and what was the number of stations affected?

I was able to complete both problems but the review of the work said it was done poorly.

# I created a test input file in the same folder as a proxy input.

import datetime  # import built in datetime module for problem 2
with open('input.txt') as file:  # read the input file into a list of lines
    file_input = file.read().splitlines()

# split the file input on the pipe
LonUndOutages = []
for var in file_input:
    LonUndOutages.append(var.split("|"))

# creating empty lists for the answers for Problem 2
Prob1Output = ""
LonUndStations = []
LonUndLines = []

# solving problem 1 and prep for the answers for Problem 2
print("Problem 1:")
for line in LonUndOutages:
    if line[0] == "07-10-2019":
        Prob1Output = Prob1Output + line[2] + "was affected by an outage on July 10, 2019\n"
    line[0] = datetime.datetime.strptime(line[0], '%m-%d-%Y')  # convert the date in line[0] to a datetime object
    if datetime.datetime(2017,7,10) < line[0] < datetime.datetime(2017,11,14):
        # the above line checks if line[0] is in the time range july 10 2017 to nov 14, 2017
        if line[1] not in LonUndLines:  # ensures LonUndLines doesnt have duplicates
            LonUndLines.append(line[1])  # adds each line that had an outage to a list
        if line[2] not in LonUndStations:  # ensures LonUndStations doesnt have duplicates
            LonUndStations.append(line[2])  # adds each station that had an outage to a list


if not Prob1Output:
    print("There were no stations affected by an outage on July 10, 2019")
else:
    print(Prob1Output)
print()
print("Problem 2:")
print("The following lines had outages:")
print(*LonUndLines, sep=', ')
print("During the period between July 10, 2017 and November 15, 2017 there were " + str(len(LonUndStations))
      + " station(s) affected")
\$\endgroup\$
  • \$\begingroup\$ Just curious - where are you getting this problem? (And others, e.g. "Problem 2") \$\endgroup\$ – BruceWayne Sep 27 '19 at 14:53
  • \$\begingroup\$ If the review was done by a human supervisor and it just says "it was done poorly", that's horrible teaching. He should tell you what you did wrong and how to improve it, otherwise how are you going to learn. \$\endgroup\$ – iuvbio Sep 27 '19 at 16:27
  • \$\begingroup\$ This was from the third stage of job interview process for a network engineer. My candidacy was rejected based on this test and the recruiter was able to get this as feedback. \$\endgroup\$ – mrhinton101 Sep 28 '19 at 1:21
5
\$\begingroup\$

That's a fun problem.

Don't use more memory than you have to

with open('input.txt') as file:  # read the input file into a list of lines
    file_input = file.read().splitlines()
# ...
for var in file_input:
    LonUndOutages.append(var.split("|"))

shouldn't be necessary. Rather than calling file.read(), iterate on the file object itself. It will yield lines that you can split on directly.

Naming conventions

LonUndOutages

becomes

lon_und_outages

and similarly for your other variables.

Unpack your line

Assign variables to the results of your split, like so:

date, line, station, notes = outage

Note that there's a nomenclature collision, because your spec describes one column as line, so call each file line a record or an outage.

Separate output from logic

        Prob1Output = Prob1Output + line[2] + "was affected by an outage on July 10, 2019\n"

You're storing output early for two purposes: to eventually show that output, and to know whether there were stations affected. This dual-purpose code is generally not advisable. Store the confirmed outages in a separate list, or yield them from a generator function, to be consumed and formatted by an output function that has no logic at all.

General

Run a linter that will tell you PEP8 hints, or use an IDE that will do the same. Make functions to house your logic.

Example

from collections import namedtuple
from datetime import date, datetime
from typing import Iterable, Set

Record = namedtuple('Record', ('date', 'line', 'station', 'notes'))


def read_file(fn: str) -> Iterable[Record]:
    with open(fn) as file:
        for record in file:
            date_str, *others = record.split('|')
            record_date = datetime.strptime(date_str, '%m-%d-%Y').date()
            yield Record(record_date, *others)


def stations_on_date(records: Iterable[Record], record_date: date) -> Set[str]:
    return set(r.station for r in records if r.date == record_date)


def lines_and_stations_between_dates(
    records: Iterable[Record],
    start: date,
    end: date,
) -> (Set[str], Set[str]):
    matching = [r for r in records if start <= r.date <= end]
    lines = set(r.line for r in matching)
    stations = set(r.station for r in matching)
    return lines, stations


def print_iter(to_print: Iterable[str]):
    msg = ', '.join(to_print)
    print(msg or 'none')


def main():
    records = tuple(read_file('input.txt'))

    date1 = date(2019, 7, 10)
    print(f'Stations affected by an outage on {date1}:')
    stations1 = stations_on_date(records, date1)
    print_iter(stations1)

    start2 = date(2017, 7, 10)
    end2 = date(2017, 11, 14)
    range_desc = f'between {start2} and {end2}'
    lines2, stations2 = lines_and_stations_between_dates(records, start2, end2)

    print(f'Lines with outages {range_desc}:')
    print_iter(lines2)

    print(f'Number of stations affected {range_desc}:')
    print(len(stations2))


if __name__ == '__main__':
    main()

Note:

  • Output and logic are separated
  • Use a namedtuple for better structure
  • Hold onto a date, not a datetime
  • Use set to avoid duplicates in the output
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Can't print_iter be reduced to one line, print(', '.join(to_print) if to_print else 'none') ? \$\endgroup\$ – Linny Sep 27 '19 at 4:06
  • 1
    \$\begingroup\$ if print_iter is fed an empty iterator, the if to_print will be True. another option would be to first assemble the message, and then print it if there is one: msg = ", ".join(to_print); print(msg or "none") \$\endgroup\$ – Maarten Fabré Sep 27 '19 at 9:33
  • \$\begingroup\$ And you've done it but not said it explicitly: make functions that e.g. take the date as a parameters, to promote reusability. All the arbitrary stuff, like the particular dates in question is in the reduced main program. Also by changing datetime to date I think you've fixed a open/closed interval bug in the original, which raises the issue of testing. \$\endgroup\$ – richardb Sep 27 '19 at 16:57
  • \$\begingroup\$ re. print_iter: it's much ado about nothing, but yes, it's a good change. \$\endgroup\$ – Reinderien Sep 28 '19 at 1:45
  • \$\begingroup\$ So should for loops always be stored in functions? Im pretty new to python and I thought the shorter the code the better where it looks like here ur saying to break each action into its own function. Is there a style guide I should look at for these types of things? \$\endgroup\$ – mrhinton101 Sep 28 '19 at 3:12
3
\$\begingroup\$

Memory Usage

When reading from the file, you can assign create the list there instead of reading lines, splitting them, and appending to the list later in the program. It can be as easy as this

with open('input.txt') as file:
    outages = [line.split("|") for line in file]

Variable Naming

PEP-8 guidelines say that variables should be in snake_case. The only time you should use PascalCase is when dealing with classes. Since you're dealing with specific outages in London, you can simply remove London from the variable names, making them shorter.

LonUndOutages -> outages
LonUndStations -> stations
LonUndLines -> lines

Creating Lists with Loops

Like shown above, when you have

my_list = []
for item in other_list:
    my_list.append(item)

This can be shortened to one line

my_list = [item for item in other_list]

Simplifying if/else

This

if not Prob1Output:
    print("There were no stations affected by an outage on July 10, 2019")
else:
    print(Prob1Output)

can become this

print("There were no stations affected by an outage on July 10, 2019" if not Prob1Output else Prob1Output)

Looks neater and less chunky.

Formatting Strings

You can directly include variables in your strings without having to type cast them, using f"". From this

print("During the period between July 10, 2017 and November 15, 2017 there were " + str(len(LonUndStations))
      + " station(s) affected")

to this

print(f"During the period between July 10, 2017 and November 15, 2017 there were {len(stations)} station(s) affected.")

Too Many Comments

Comments should mainly be used to describe code that may be hard to comprehend, like an algorithm, or why you chose to do something. Comments like this

import datetime  # import built in datetime module for problem 2
with open('input.txt') as file:  # read the input file into a list of lines
    file_input = file.read().splitlines()

# split the file input on the pipe
LonUndOutages = []
for var in file_input:
    LonUndOutages.append(var.split("|"))

Anyone reading the code and see what it does, without the comments. Comments like these can decrease the readability of your code, as readers see more bulk when reading your code.

| improve this answer | |
\$\endgroup\$
  • 5
    \$\begingroup\$ I would add that excessive commenting is a sign that the code isn't expressive enough, and line-by-line commenting is a sign that the author isn't fluent in the programming language. \$\endgroup\$ – 200_success Sep 27 '19 at 4:19
  • 1
    \$\begingroup\$ print("There were no stations affected by an outage on July 10, 2019" if not Prob1Output else Prob1Output) can be even clearer. fallback_msg = "There were no stations affected by an outage on July 10, 2019"; print(Prob1Output or fallback_msg) \$\endgroup\$ – Maarten Fabré Sep 27 '19 at 9:29
  • \$\begingroup\$ I don't see how reducing if/else etc down to one line without removing the if/else improves code readability. In fact, in many cases it will reduce it as you aren't able to see, at a glance, where the decision points in the code are, as they now "hidden" within blocks of text. \$\endgroup\$ – illustro Sep 27 '19 at 16:24

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.