[Tutor] xlwt & xlrd: can I refactor this code better?

[Alan Gauld]

"Albert-Jan Roskam" <fomcl at yahoo.com> wrote

> Most functions have many arguments - isn't that called 'tight coupling'?

No. In fact too few parameters (arguments are what you pass in,
parameters are what you specify in the definition) often wind up
relying on global variables to share data - now THAT is tight coupling.

> The first function is the main() function. Shouldn't a programmer strive
> for information hiding in such a function?

Yes but only in terms of the algorithm and local variables. If you
genuinely need to pass in values then parameters are the correct
mechaniasm. You can sometimes reduce the param count by
building more complex data structures but that doesn't look to
be the case here. But 6 or so parameters is not excessive.
You can increase usability by providing default arguments,
but again you have already done that...

> it should almost read like regular english, right?
> Or is that too textbook-ish? ;-)

Its a good objective but don't become a slave to it.
You can do a lot with naming, but again you seem to have
tried there too.

> def merge_xls(in_dir="d:/temp/", out_file="d:/temp/merged_output.xls"):
>    """ Main function: merge xls sheets """

The doc string should say more about how to use the function.
What are each of the inputs and what is the output?
What errors are raised under what circumstances?

>    xls_files = glob.glob(in_dir + "*.xls")
>    xls_files.sort()
>    merged_book = xlwt.Workbook()
>    osheet_names = [os.path.basename(xls_file)[:-4] for xls_file in 
> xls_files]
>    for xls_no, xls_file in enumerate(xls_files):
>        print "---> Processing file %s" % (xls_file)

I'm not a fan of print statements inside functions, it reduces their
reusability in Web/GUI applications.

> def check_xls(book, merged_book, isheet_names, osheet_names, xls_no, 
> xls_files):
>
> def write_sheets(book, merged_book, isheet_names, osheet_names, xls_no, 
> xls_files):
>
>    else:
>        raise Exception ("ERROR *** File %s has %s sheets (maximum is 99)" 
> % (xls_file, book.nsheets))

Don't do this!
Create a new exception class specific to your functions.

This way the client has no alternative but to use

try:....
except:....

which will catch all exceptions. Then they must examine the exception data
to see what kind they have caught, that's a lot of work for the client just
to save you doing

class XLSFileError(Exception): pass

and

raise XLSFileError(....)

HTH,


-- 
Alan Gauld
Author of the Learn to Program web site
http://www.alan-g.me.uk/