[Tutor] Re: If elif not working in comparison

[Brian van den Broek]

gerardo arnaez said unto the world upon 2005-04-05 23:00:
> Hi all.
> 
> I would like some crituqe on this code.
> It is three separate files (all put on one web page)
> Each one is labeled in the comment that begins each section of code.
> 
> It is a little longer when I put it all on one page, so I have it up on a link.
> If any kind souls would critique it before I move on,
> to make it webbased (What would be a good interface php?)
> The link is at 
> http://mung.net/~dude/coumadinAll.html
> 
> Thanks all all for your time,
> G
> _______________________________________________
> Tutor maillist  -  Tutor at python.org
> http://mail.python.org/mailman/listinfo/tutor


Hi Gerardo,

Two factors conspire to make it difficult to follow your code: 1) I am
not the most experienced, either, and 2) there are, IMHO, some style 
difficulties with it.

It could well be that the first is the more influential here. But
given that its true, most of my comments are on style. And away we go:


> import input #input coumadin data and return it as workable date
> import inr #calulate percent increases in dose given an INR
> import math #to allow for rounding

Try that as:

import input     # input coumadin data and return it as workable date
import inr       # calculate percent increases in dose given an INR

etc. *Much* easier to take in at a glance.

Better still, move the explanatory information from the imports and
give the modules docstrings that tell this story. Oh, wait, you did
that. Then leave these out, I'd say. The docstrings speak for
themselves, and anyone (you even) who is curious later can find the
story out quite quickly.


> CurrentINR, UnitDose, TotalCurrentDose, CurrentDoseList, CurrentDoseRegimenWeek = input.AddItUp()

That is painfully long. It doesn't fit on my laptop screen at any
readable font size. Perhaps

# temp_container for unpacking the returned tuple
temp_container = input.AddItUp()
CurrentINR, UnitDose, TotalCurrentDose = temp_container[0:3]
CurrentDoseList, CurrentDoseRegimenWeek = temp_container[3:]

But I think I'd break it down one line per name. Again, I think much
easier to quickly parse, even with the overhead of the indirection.

> CurrentDoseUnit  = TotalCurrentDose/UnitDose

Easier to read as

CurrentDoseUnit  = TotalCurrentDose / UnitDose

Similar comments throughout on your use of operators. The extra space
is cheap, and oh so handy for ease of understanding.

The next line is
> print TotalCurrentDose/UnitDose, "is the Curent Coumadin Dose(%smg) in units" % TotalCurrentDose

so why not use the name you built? As in:

print CurrentDoseUnit, "is ..."

> print "The rounded dose, due to pill dose limitation is either %s" % math.floor(NewDoseUnit),\
>       "or %s" % math.ceil(NewDoseUnit), "units." 

I'd do:

print '''The rounded dose, due to pill dose limitation is either %s
or %s units.''' %(math.floor(NewDoseUnit), math.ceil(NewDoseUnit))

(With triple quotes you don't need the '\' -- I try to avoid them as
they are easy to miss.)

> WeekOrderedlist = 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday','Sunday'

But:

>>> type(WeekOrderedlist)
<type 'tuple'>

1) You name lies. That's bad :-)
2) I think clarity is enhanced by explicitly putting in the optional
'(' and ')' on tuples. People differ, but I like the instant (in
assignments) indication of tupleness.

>     x = decimal.Decimal(str((float(x)))) # make sure x is a number

I think (str(float(x))) would be fine.

A last general comment: I really like long_and_descriptive_names_too.
But, that said, some of yours seem too long to me. For instance,
CurrentDoseRegimenWeek could probably go to current_regimen without
loss of clarity. And, as you can see, I am a fan of underscore_names.
Religious wars have started for less, so no trying to convince you
here. :-)  but, I believe the convention that a leading cap indicates
a class name is pretty strong.

Hope these are of some help. Best to all,

Brian vdB