Extract Functions, One Step at a Time

With the safety net in place, you can change the script with confidence, as long as you change it in small steps. The rule for the whole lesson is:

Make one small change. Run the tests. Repeat.

If the tests fail, the cause is in the last change, and the last change was small. You never have to hunt through an hour of edits for the one that broke the output.

The move: Extract Function

Nearly all of today is one move, repeated:

1. Find a few lines that do one thing you can name.
2. Move them into a function with that name.
3. Whatever the lines read becomes a parameter. Whatever they produce becomes the return value.
4. Call the function where the lines used to be.
5. Run the tests.

Here’s the first one. Reading the file is three lines at the top of the script plus the blank-line check inside the loop. Together they do one nameable thing, “get the non-blank lines of a file”:

# before
f = open("sample.log")
data = f.read()
f.close()
...
for l in data.split("\n"):
    if l.strip() == "":
        continue
    ...

# after
def read_lines(path):
    """Return the non-blank lines of a log file."""
    with open(path) as file:
        return [line for line in file.read().splitlines() if line.strip()]

...
for l in read_lines("sample.log"):
    ...

Run the tests. They pass, so read_lines did exactly what the old lines did, for this file at least. It’s also the first place a reader can learn what this part does without reading how.

Name the magic numbers

w[2] and w[3][:-1] appear in the loop with no explanation. A function with a name is the explanation:

def level_of(line):
    """The level is the third word of a line."""
    return line.split()[2]


def source_of(line):
    """The source is the fourth word, minus its last character (the colon)."""
    return line.split()[3][:-1]

source_of still uses [:-1]. rstrip(":") would be nicer, but it’s a different behaviour: they disagree on a word with no colon. That’s a fix, and fixes come after the refactor, as their own step with their own test. For now, keep it identical and note it.

The counting loops

The loop is doing two jobs, counting levels and counting error sources. Give each its own function, with the whole list as the parameter and a dict as the result:

def count_levels(lines):
    counts = {}
    for line in lines:
        level = level_of(line)
        counts[level] = counts.get(level, 0) + 1
    return counts

Splitting one loop into two means reading the lines twice. For a log that fits in memory that costs nothing worth measuring, and in return each function can be tested alone. (Day 2 comes back to this trade-off for logs that don’t fit in memory.)

A main function, and the main guard

What’s left at the top level is the part that decides what happens: read this file, print these three lines. Put that in a main function too, behind the guard you met in the beginner course:

def main():
    lines = read_lines("sample.log")
    print("Lines read:", len(lines))
    print("Levels:", count_levels(lines))
    print("Errors by source:", errors_by_source(lines))


if __name__ == "__main__":
    main()

This is what makes the file testable piece by piece. A test does import report to call count_levels. Without the guard, that import runs the whole script, and it reads a file and prints a report in the middle of your test run. With it, importing only defines the functions. The safety net still works, because it runs the file with run_name="__main__", just like python report.py.

Bug hunt: the test that got the wrong answer

Here’s someone’s refactor, halfway done. The safety net passes. They’ve added two unit tests with a two-line input, and one of them fails:

Reproduce. Already reproduced: test_errors_by_source fails, and the input is just two lines.

Inspect. Two things in the output don’t add up. First, the failure itself: two lines went in, one of them an error from payments, and the answer mentions search, which isn’t in the input at all. It is in sample.log, though. Second, the report is printed at the top of the output, before any test ran. The tests never call the script, so what printed it?

Narrow. The second clue explains the first. from report import ... runs report.py from top to bottom, and there’s no main guard, so the script part runs too. That creates a module-level variable lines holding every line of sample.log. Now read errors_by_source carefully:

def errors_by_source(log_lines):
    counts = {}
    for line in lines:          # not log_lines

The parameter is log_lines, but the loop reads lines. There’s no local lines, so Python looks in the module, finds the one the script created, and quietly counts sample.log instead of its argument. The safety net couldn’t catch it, because in the full script the global and the argument happen to be the same list.

Pin it. test_errors_by_source already is the regression test. It passes a list that is not sample.log, which is exactly what catches a function reading the wrong data.

Fix. Two changes, and both matter:

1. Loop over the parameter: for line in log_lines, or rename the parameter to lines.
2. Move the script part into main() behind the main guard, so importing the module creates no lines variable for a function to find by accident.

After the first fix alone the test passes. The second makes this whole class of bug impossible: a function with no global to fall back on fails loudly with NameError instead of silently using the wrong data.

Your turn

"""Count what happened in a log: the same report, rebuilt from small functions."""


def read_lines(path):
    """Return the non-blank lines of a log file."""
    with open(path) as file:
        return [line for line in file.read().splitlines() if line.strip()]


def level_of(line):
    """The level is the third word of a line."""
    return line.split()[2]


def source_of(line):
    """The source is the fourth word, minus its last character (the colon)."""
    return line.split()[3][:-1]


def count_levels(lines):
    counts = {}
    for line in lines:
        level = level_of(line)
        counts[level] = counts.get(level, 0) + 1
    return counts


def errors_by_source(lines):
    counts = {}
    for line in lines:
        if level_of(line) == "ERROR":
            source = source_of(line)
            counts[source] = counts.get(source, 0) + 1
    return counts


def main():
    lines = read_lines("sample.log")
    print("Lines read:", len(lines))
    print("Levels:", count_levels(lines))
    print("Errors by source:", errors_by_source(lines))


if __name__ == "__main__":
    main()

What you can now do

• Extract a function: name it, pass in what it reads, return what it produces, and run the tests.
• Change code in steps small enough that a failing test points at one step.
• Use main() and the main guard so a module can be imported without side effects.
• Recognise a function reading a global instead of its parameter, and prevent it.

Next: splitting the script into modules, a trap in default arguments, and the Day 1 checkpoint.