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@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
_______________________________________________ Tutor maillist - Tutor@python.org http://mail.python.org/mailman/listinfo/tutor
