6
\$\begingroup\$

What does this script tell you about how I need to improve as a programmer? I'm somewhat new to both Python and programming, so feel free to minimize your assumptions about my knowledge.

The purpose of this script is to read a .csv of names and emails addresses that are improperly organized -- sometimes there is only one name, sometimes three names (first, middle, last) are in the same cell, and sometimes the person has a title (Mr., Mrs.) with their name.

enter image description here

Right now, the code does "work". It will execute, but there's a few small issues with some of these functions -- for example, the names aren't properly distributed to columns before the file is written. I'm less interested in these smaller things right now, and would rather have a bigger picture review.

I'll happily accept whatever advice you'll offer, but I'd most appreciate feedback on how I've accepted command line arguments, and the layout of the "engine" that controls the flow of operation and calls functions, and specific changes to how I've written the internals of each function -- for example, how could they be faster?

I've also posted general questions I've had along the way. If you're interested, please weigh in on those (see below code). I'm also less interested in unanticipated cases, but welcome them if you're inspired.

import csv
import sys

# This script will expext three arguments:
# 1. File with data to be scrubbed (CleanCsv.in_file)
# 2. File name to output clean data (CleanCsv.clean_out_file)
# 3. File name to output data that's needs review (CleanCsv.dirty_out_file)

class CleanCsv(object):
    def __init__(self, args):
        self.in_file = args[1]  # Storing original (necc?)
        self.clean_out_file = args[2]
        self.dirty_out_file = args[3]
        self.flags = [x for x in args[4:] if sys.argv != ""]
        # Unnecessary?  sys.argv will never be blank---> ^^^
        self.manual_repair = []
        self.rows = []
        self.functions = [
            'strip_blank_fields',
            'strip_whitespace',
            'capitalize',
            'strip_blank_lists',
            'split_on_blanks',
            'remove_duplicate_names',
            'remove_bad_rows',
            'columnize',
            ]

    def do_scrub(self):
        """Runs all functions in self.functions that weren't negated by command-
        line arguments.  Writes a clean file and, optionally, a file of rows
        that couldn't be properly scrubbed
        """
        self.fxns = [x for x in self.functions if x not in self.flags]
        self.grab_file_data(self.in_file)
        for fx in self.fxns:
            next_operation = getattr(self, fx)
            next_operation()
        if self.manual_repair:
            self.write_csv(self.manual_repair, self.dirty_out_file)
        self.write_csv(self.rows, self.clean_out_file)

    def grab_file_data(self, filename):
        """Opens the file passed as filename and writes rows to a list of lists.
        """
        with open(filename, 'rt') as opened_file:
            read_file = csv.reader(opened_file)
            for row in read_file: #  [q1]
                self.rows.append(row)
            opened_file.close

    def strip_blank_fields(self):
        """If there are any blank fields in a row, take them output.
        """
        for row in self.rows:
            while "" in row:
                row.remove("")

    def strip_whitespace(self):
        """If there is whitespace in a field, take it out."""
        for row in self.rows:
            for num, field in enumerate(row):
                row[num] = field.strip()

    def capitalize(self):
        """Make all non email fields capitalized (string.title()).
        """
        for row in self.rows:
            for num, field in enumerate(row):
                if "@" not in field and not field.istitle():
                    row[num] = field.title()

    def strip_blank_lists(self):
        """Remove any rows that are blank.
        """
        while [] in self.rows:
            self.rows.remove([])

    def split_on_blanks(self):
        """For fields that have a space between two words, split them out into
        seperate fields
        """
        for row in self.rows:
            for num, field in enumerate(row):
                if ' ' in field:
                    x = field.split(' ')
                    row.pop(num)
                    for i in x:
                        row.append(i)

        # Move all the emails addresses to the last column.
        for row in self.rows:
            for num, field in enumerate(row):
                if "@" in field:
                    x = row.pop(num)
                    row.append(x)

    def remove_duplicate_names(self):
        """If two columns have the same name in them, remove one of the names.
        """
        for num, row in (enumerate(self.rows)):
            for x in xrange(len(row)):
                if row.count(row[x]) > 1:
                    row.pop(x)
                    break

    def remove_bad_rows(self):
        """Remove all rows that don't have at least three fields filled.
        Assumes that rows with less than three fields means either missing
        first, last, or email.
        """
        for num, row in enumerate(self.rows):
            if len(row) < 3:
                x = self.rows.pop(num)
                self.manual_repair.append(x)

        # Remove all rows that don't have an email address. [q2]
        for num, row in enumerate(self.rows): 
            bad = True
            for field in row:
                if '@' in field:
                    bad = False
                    break
            if bad == True:
                x = self.rows.pop(num)
                self.manual_repair.append(x)

    def columnize(self):
        """If there's no title (Mr, Mrs, etc), put a space in the first column.
        """
        titles = [
        'Mr', 'Mrs', 'Mr.', 'Mrs.', 'mr',
        'mrs', 'mr.', 'mrs.', 'Miss', 'miss'
        ]

        for row in self.rows:
            if set(row).isdisjoint(set(titles)):
                row.insert(0, '')

    def write_csv(self, rows, name_to_write):
        """Writes a csv based on a list of lists as data for the rows, and a
        name of the file to write (string).
        """
        f = open(name_to_write, 'wt')
        try:
            writer = csv.writer(f)
            writer.writerow(('Title', 'First', 'Middle', 'Last', 'Email'))
            for row in rows:
                writer.writerow(row)
        finally:
            f.close()

if __name__ == "__main__":
    x = CleanCsv(sys.argv)
    x.do_scrub()

General questions:

  1. Is this well suited as an OOP? It makes it easier to organize, but what are the downsides of using OOP in this case? Should I separate the worker functions into a different class from the engine/control functions?
  2. Am I missing any PEP8 stuff?
  3. There's a bunch of other classes and functions in the csv module. Is there anything I could have used?
  4. Is there any reason to split this into two different files?
  5. What are the components of the Python language that I'm missing? What features of the standard library could I use to make this better?
  6. Is there too much nesting? My general sense is that less nesting is better for clarity, but I haven't seen a way to get around it here.

Specific in-line questions:

  1. Should I be able to access this without having to re-write it to lists?
  2. Should this loop be moved into another function for modularity?
\$\endgroup\$
7
\$\begingroup\$

Design

As far as I can tell, what you are actually trying to do is to clean up a CSV file so that it has five fields (title, first name, middle name, last name, and e-mail address), by applying a sequence of heuristics. You then allow the caller to specify flags to turns these heuristics off if they are not working.

There are several problems with this approach:

  1. You end up pushing the responsibility for working out which heuristics to apply onto the user of your program.

  2. In order for the user to be able to figure out which heuristics to turn on, the documentation is going to have to be complex, and most likely hard to understand.

  3. The heuristics interact with each other. For example, columnize adds blank fields, but strip_blank_fields removes blank fields: which wins? How will you explain this in the documentation?

  4. In any case, the heuristics don’t achieve what you are trying to do: for example you don’t move the title to the beginning of the row, or the e-mail address to the end.

So, although it’s probably not what you were hoping to hear, I think that your code would be more reliable, easier to use and easier to understand if you implemented your clean-up as a straightforward algorithm, rather than as a configurable collection of interacting heuristics. If you find that the algorithm doesn’t work on some of your data, then improve it until it does (instead of adding yet more optional heuristics).

I’ve put some revised code at the end of this answer showing how you might implement my suggested approach.

Comments

  1. There’s no docstring for the CleanCsv class. Users will like to be able to run help(CleanCsv) to learn how to use the class.

  2. The CleanCsv class starts by processing the command-line arguments. This design decision means that you can only easily use this class from the command-line. If you want to use it from another program or from a test suite, or interactively from the Python interpreter, then you have to mock up an argv array. The class would be more widely useful if it took its arguments directly, leavng the command-line processing to be done in the command-line case.

  3. It seems perverse to me that you interpret the command-line argument capitalize to mean “don’t capitalize”. The argument should be something like --dont-capitalize or --no-capitalization to make it clear what it does.

  4. You don’t check the command-line arguments to see if they contain errors. This means that misspellings like capitalise won’t be detected.

  5. If you’re going to process command-line arguments, it would be a good idea to use the argparse module in the standard library. This gives you help text and error messages.

  6. You do your processing with the whole of the input and output in memory: you read the whole of the input CSV file into a list; then you apply each function to the whole list; then you write out the whole list to the output CSV file. This means that your memory usage is going to depend on the size of the CSV file, and means that your program will behave poorly (lots of swapping) if you need to process very large CSV files.

    You could change the code so that it works row-by-row instead of function-by-function: read a single line at a time from the input CSV file, apply each function to that line, and then write out the updated line to one of the output CSV files. That way the memory footprint doesn’t need to be much bigger than the biggest line in the input.

  7. All of your functions that operate on rows in the input have boilerplate of the form

    for row in self.rows:
    

    or

    for num, row in (enumerate(self.rows)):
    

    if you refactored the code so that it worked row-by-row then you would be able to avoid this (each function would operate on a single row).

  8. This line is bogus (as you say in your comment):

    self.flags = [x for x in args[4:] if sys.argv != ""]
    # Unnecessary?  sys.argv will never be blank---> ^^^
    

    You’re checking sys.argv each time around the loop to see if it’s equal to the empty string, which of course it isn’t (it’s an array, not a string). Probably you meant to write

    self.flags = [x for x in args[4:] if x != ""]
    

    which could be abbreviated to

    self.flags = [x for x in args[4:] if x]
    

    since only empty strings test false, but since what you are actually going to do is look up function names to see if they appear in this list, it would be more efficient to use a set than a list (sets can test membership in O(1) but lists take O(n)):

    self.flags = {x for x in args[4:] if x}
    

    but in practice you don’t care about whether this set contains the blank string or not (you’re never going to look it up), so you could just write

    self.flags = set(args[4:])
    

    but I still think using argparse would be even better.

  9. This operation is O(n2):

    while [] in self.rows:
        self.rows.remove([])
    

    (Consider a row whose first half is non-blank and whose second half is blank.) The natural way to remove blank entries from a list in Python is to filter the list:

    self.rows = [row for row in self.rows if rows]
    

    You have several other O(n2) operations in your code. In strip_blank_fields you have

    while "" in row:
        row.remove("")
    

    which could be turned into:

    row = [f for f in row if f]
    

    In split_on_blanks you have

    for num, field in enumerate(row):
        if ' ' in field:
            x = field.split(' ')
            row.pop(num)
            for i in x:
                row.append(i)
    

    which could be turned into a double comprehension:

    row = [f for field in row for f in field.split(' ')]
    

    (In all these cases you’d want to reorganize your code, because you are currently relying on being able to update your rows in-place. You could write

    row[:] = # ... filtering code here ...
    

    but it would be better to reorganize so you didn’t have to.)

  10. Splitting fields on single spaces is probably not what you want, because if there are multiple contiguous spaces you get useless empty strings:

    >>> 'First  Last'.split(' ')
    ['First', '', 'Last']
    

    You probably want to split on any contiguous sequence of whitespace:

    >>> 'First  Last'.split()
    ['First', 'Last']
    
  11. This code in remove_duplicate_names is not only O(n2) but only removes the first duplicate it finds:

    for x in xrange(len(row)):
        if row.count(row[x]) > 1:
            row.pop(x)
            break
    

    So it turns ['A','A','A'] into ['A','A'] which is probably not what you want. It isn’t clear to me what you actually want to do here, but if the specification is to keep only the first copy of each duplicated field, then the OrderedDict class in the collections module does what you need, like this:

    row = OrderedDict((f, None) for f in row).keys()
    

    (Morally speaking you want an ordered set here rather than an ordered dictionary, but there’s no ordered set in the standard library. You could use this recipe if you care about this issue.)

  12. This docstring doesn’t accurately describe what the function does:

    """If there is whitespace in a field, take it out."""
    

    Better would be something like

    """Strip whitespace from the beginning and end of each field."""
    
  13. This:

    titles = [
        'Mr', 'Mrs', 'Mr.', 'Mrs.', 'mr',
        'mrs', 'mr.', 'mrs.', 'Miss', 'miss'
        ]
    

    could be written as a set comprehension which expresses your intention more directly (that is, for each title you want capitalized and lower-case versions, both with and without a period):

    titles = {t for t in 'Miss Mr Mrs Ms'.split()
                for t in [t, t.lower()]
                for t in [t, t + '.']}
    

Answers to your general questions

  1. Once you’ve made the processing row-by-row (see comment #6), then you’ll see that the heuristic functions don’t refer to self any more, and so don’t need to be methods. This suggests that it’s unnecessary to organize the code as a class (and I chose not to, in my revision). But unnecessary is not the same as wrong: there’s room for personal taste here.

  2. Not that I noticed.

  3. No.

  4. This is really a usability quetsion, so I can’t answer this without knowing more about the context in which your program is going to be used. Try it both ways and see which is easier to work with.

  5. The argparse module (see comment #5) and the OrderedDict class (see comment #11).

  6. Nested loops are appropriate in some contexts, but in this case, I think the nesting (and especially the repeated boilerplate discussed in comment #7) was a sign that something was wrong with the organization of the code.

Answers to your specific questions

  1. Yes: see comment #6.

  2. No: I think the whole “modularity” approach has drawbacks, as discussed in the “Design” section above.

Revised code

import argparse
import collections
import csv
import sys

# Dispositions for a row.
CLEAN, DIRTY, DROP = range(3)

# Map from title to canonical version of that title.
TITLES = {t: title for title in 'Miss Mr Mrs Ms'.split()
          for t in [title, title.lower()]
          for t in [t, t + '.']}

def clean_row(row):
    """
    Return a pair `(disposition, row)` where `disposition` is:
    `CLEAN` if `row` was successfully cleaned up and should be
            output to the clean file;
    `DIRTY` if `row` could not be cleaned up and should be output to
            the dirty file; or
    `DROP`  if `row` was empty and should be dropped.
    """
    title, names, email = '', collections.OrderedDict(), ''
    for f in (f for field in row for f in field.split()):
        if f in TITLES:
            if title:
                return DIRTY, row
            title = TITLES[f]
        elif '@' in f:
            if email:
                return DIRTY, row
            email = f
        elif f:
            names[f.capitalize()] = None

    names = names.keys()
    if len(names) == 3 and email:
        return CLEAN, [title] + names + [email]
    elif len(names) == 2 and email:
        return CLEAN, [title, names[0], '', names[1], email]
    elif names or email:
        return DIRTY, row
    else:
        return DROP, row

def clean_csv(input, clean, dirty):
    """
    Read rows from the CSV file INPUT and attempt to clean them up
    to fit the format (title, first name, middle name, last name, and
    e-mail address). Rows that can be cleaned up are written to the
    output CSV file CLEAN, and rows that cannot be cleaned up are
    written to the output CSV file DIRTY.
    """
    clean_writer = csv.writer(clean)
    dirty_writer = csv.writer(dirty)
    headings = 'Title First Middle Last Email'.split()
    for w in clean_writer, dirty_writer:
        w.writerow(headings)
    for row in csv.reader(input):
        disposition, row = clean_row(row)
        if disposition == CLEAN:
            clean_writer.writerow(row)
        elif disposition == DIRTY:
            dirty_writer.writerow(row)
        else:
            assert(disposition == DROP)

if __name__ == '__main__':
    p = argparse.ArgumentParser(description = clean_csv.__doc__)
    p.add_argument('INPUT', type = argparse.FileType('r'),
                   help = 'Input CSV file.')
    p.add_argument('CLEAN', type = argparse.FileType('w'),
                   help = 'Output CSV file for rows that can be cleaned.')
    p.add_argument('DIRTY', type = argparse.FileType('w'),
                   help = 'Output CSV file for rows that need manual repair.')
    args = p.parse_args()
    clean_csv(args.INPUT, args.CLEAN, args.DIRTY)
\$\endgroup\$
  • \$\begingroup\$ Gareth: "So, although it’s probably not what you were hoping to hear, I think that your code..." No, I was hoping to hear something like this :-) This is helping me improve, so I'm more than glad to take polite and constructive criticism, which your certainly was. Such a humbling experience to have somebody spend so much effort on improving my code! Wow. Thanks a bunch. \$\endgroup\$ – Brian Dant Oct 7 '12 at 23:52

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.