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. 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 csvimport 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: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?  Am I missing any PEP8 stuff?  There's a bunch of other classes and functions in the csv module.  Is there anything I could have used?  Is there any reason to split this into two different files?  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?  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:Should I be able to access this without having to re-write it to lists?  Should this loop be moved into another function for modularity?
Script that scrubs data from a .csv file
python;optimization;performance;csv
DesignAs 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:You end up pushing the responsibility for working out which heuristics to apply onto the user of your program.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.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?In any case, the heuristics dont achieve what you are trying to do: for example you dont move the title to the beginning of the row, or the e-mail address to the end.So, although its 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 doesnt work on some of your data, then improve it until it does (instead of adding yet more optional heuristics).Ive put some revised code at the end of this answer showing how you might implement my suggested approach.CommentsTheres no docstring for the CleanCsv class. Users will like to be able to run help(CleanCsv) to learn how to use the class.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.It seems perverse to me that you interpret the command-line argument capitalize to mean dont capitalize. The argument should be something like --dont-capitalize or --no-capitalization to make it clear what it does.You dont check the command-line arguments to see if they contain errors. This means that misspellings like capitalise wont be detected.If youre 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.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 doesnt need to be much bigger than the biggest line in the input.All of your functions that operate on rows in the input have boilerplate of the formfor row in self.rows:orfor 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).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---> ^^^Youre checking sys.argv each time around the loop to see if its equal to the empty string, which of course it isnt (its an array, not a string). Probably you meant to writeself.flags = [x for x in args[4:] if x != ]which could be abbreviated toself.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 dont care about whether this set contains the blank string or not (youre 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.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 havewhile  in row:    row.remove()which could be turned into:row = [f for f in row if f]In split_on_blanks you havefor 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 youd want to reorganize your code, because you are currently relying on being able to update your rows in-place. You could writerow[:] = # ... filtering code here ...but it would be better to reorganize so you didnt have to.)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']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)        breakSo it turns ['A','A','A'] into ['A','A'] which is probably not what you want. It isnt 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 theres no ordered set in the standard library. You could use this recipe if you care about this issue.)This docstring doesnt accurately describe what the function does:If there is whitespace in a field, take it out.Better would be something likeStrip whitespace from the beginning and end of each field.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 questionsOnce youve made the processing row-by-row (see comment #6), then youll see that the heuristic functions dont refer to self any more, and so dont need to be methods. This suggests that its unnecessary to organize the code as a class (and I chose not to, in my revision). But unnecessary is not the same as wrong: theres room for personal taste here.Not that I noticed.No.This is really a usability quetsion, so I cant 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.The argparse module (see comment #5) and the OrderedDict class (see comment #11).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 questionsYes: see comment #6.No: I think the whole modularity approach has drawbacks, as discussed in the Design section above.Revised codeimport argparseimport collectionsimport csvimport 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, rowdef 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)