Introduction
No, there's nothing wrong with the atof function. What's wrong is my code that uses it. And I thought this little story would be amusing and interesting to some of you, so I decided to up my article count by one more article and write this little exposition, exposing myself to you, the dear reader, to your ridicule and guffaws.
Now, there are a lot of excellent articles on CP, to which I of course have contributed several, but it seems that there are few (if any--I certainly didn't find any, but then again, I didn't look very hard) articles on our day-to-day trials and tribulations and the stupid things we do. So I am yet again setting a precedent at CP by writing about something everyone else is either too ashamed, scared or smart to write about. And in between the lines, you may discover something that touches your soul (or takes you for a spin on the porcelain bus).
Some bugs aren't actually that stupid and illustrate that even with the best of unit testing, we can't test for every contingency. This is a story of how I screwed up a piece of code that worked fine for a month, until one day we got an invoice from a vendor that was, well, too large an amount for my software to handle.
Setting The Stage
Some of you know that I do some contract work for two boatyards in the "tax me" state, otherwise known as Connecticut. Everything in Connecticut is taxed, and there is fees for everything--even the beaches. In fact, Governor Rowland, who just got re-elected, is closing half of the state's Department of Motor Vehicles offices because of a project $1,500,000,000 deficit for next year. Now, none of this has to do with my bug or this article, except for a particular feature of that very large amount. But I get ahead of myself.
One of the accounting functions requires verifying a vendor's invoice against the purchase order and entering the freight charges into the system. At that point, the PO can be closed and the customer(s) are billed for the parts and any freight charges incurred by the boatyard. So, how do freight charges get distributed, you may ask? Well, the freight charges are distributed based on the percent of an item's quantity*cost as part of the entire invoice amount. There's no other way of doing it because we don't know the weight of things, which is really how freight is charged. So, it means that a guy buying a $5,000 radar is going to pay for most of the freight when there's another guy buying a 500lb block of lead ballast for 5 bucks on the same invoice. Nobody said life is fair. And nobody said that my examples have any relationship to reality.
Now, note in this screen shot, which is inspecting the problem after it happened, that, while the vendor cost is $162.88 for this part (which by the way is a 12V pump), my software has calculated that, factoring in the 37.56 of freight charges, the part actually costs a whopping $1,304.86!
Oh my. This is not good. The boatyard maintains an "adjusted cost" for every inventory item that is a moving average of the actual price of the part plus freight charges. The moving average comes into play each time the part is purchased. The adjusted cost is recalculated to 4/5th's of the current cost plus 1/5th of the new cost. This way, we can slowly modify our inventory cost. Now, for this $162 part, I've added over $1300 in inventory value.
Needless to say, our accountant is not happy. With me.
The GUI and Data Acquisition
So, let's trace into how this is happening.
First off, the GUI that is displaying the information on the screen looks like this (this is a piece of it):
GUI:ReceiveCheckTab
dlg nosize true
dlg caption "Process Vendor Invoice"
font ("MS Sans Serif", 8)
end.
STATIC s1 at (0, 20) size (100, 15) font
("MS Sans Serif", 8, B) caption "Select Part:" end.
LISTCONTROL lcCheckItems at (0, 35) size (465, 120)
storage rcvCheckIdx
list storage rcvCheckList
header ("OK?":40C,
ID:0,
partID:0,
"WO/Dept":70,
"Vend. Part #":80,
"Qty":50,
"Cost $":65R,
"Total $":70R,
AdjustedCost:0,
MSRPCost:0,
MarkupOption:0,
Markup:0,
...)
with options (grid editable)
on selection ScriptMgr.RunMacro(dp_macroName, SelectRcvCheckItem)
on double click ScriptMgr.RunMacro(dp_macroName, RcvShowPartInfo2) end.
...
BUTTON btnClosePO at (480, 220) size (100, 20) caption "Close PO"
on selection ScriptMgr.RunMacro(dp_macroName, ClosePO) end.
...
GUIEND
Doesn't look like any GUI you've ever seen, does it? Well, that's because it's part of my Application Automation Layer for C++/MFC that I use for all my C++ projects. I'm sure the intelligent reader can correlate the specification for the LISTCONTROL to the GUI presented in the screenshot.
Now, here's the database query that loads the rcvCheckList matrix:
DBMgr.QueryMultiRow(dp_DB, MyDB, "select
a.CHECKED,
a.ID,
a.PARTNUM_ID,
g.WO_NUMBER,
f.VENDOR_PARTNUM,
a.QTY,
a.VENDOR_COST,
VAL(a.QTY)*VAL(a.VENDOR_COST),
b.ADJUSTED_COST,
b.MSRP_COST,
b.MARKUP_OPTION,
b.MARKUP,
a.PROBLEM_FLAG,
f.ID,
g.WO_NUMBER,
a.COMMENT,
0,
'per '+a.BASE_QTY+' '+h.NAME,
'Stocking Unit is per '+i.NAME,
h.ID,
i.ID,
0,
0,
0
from
PO_ITEM a,
INVENTORY b,
VENDOR c,
PURCHASE_ORDER d,
VENDOR_PART f,
WORKORDER g,
UNIT h,
UNIT i
where
b.ID=a.PARTNUM_ID and d.ID=a.PO_ID and c.ID=d.VENDOR_ID and d.ID={rcvID} and
g.ID=a.WO_ID and f.PARTNUM_ID=a.PARTNUM_ID
and VAL(f.BASE_QTY)=VAL(a.BASE_QTY) and
f.VENDOR_ID=d.VENDOR_ID and a.UNIT_ID=f.UNIT_ID and
h.ID=f.UNIT_ID and i.ID=b.STOCKING_UNIT_ID
order
by a.ID", rcvCheckList)
followed by some adjustments to the data and the list control:
...
DataMatrix.Iterate(dp_iterateMatrix, rcvCheckList, i, AdjustCheckList, 0)
Number.CommaFormat(dp_formatNumber, rcvCheckTotal, {rcvCheckTotal}, %.2lf)
WinMgr.SetListControlEditStyle(dp_EditStyle,
ReceiveCheckTab, lcCheckItems, 0, YESNO)
WinMgr.UpdateAllControls(dp_viewName, ReceivePartsTab)
...
MACRO:AdjustCheckList
Math.RPN(dp_RPN, "{rcvCheckTotal} {rcvCheckList(7, {i})} + rcvCheckTotal STO")
Number.Format(dp_formatNumber, rcvCheckList(6, {i}), {rcvCheckList(6, {i})}, %.4lf)
Number.Format(dp_formatNumber, rcvCheckList(7, {i}), {rcvCheckList(7, {i})}, %.2lf)
Number.Format(dp_formatNumber, rcvCheckList(8, {i}), {rcvCheckList(8, {i})}, %.4lf)
Number.Format(dp_formatNumber, rcvCheckList(9, {i}), {rcvCheckList(9, {i})}, %.4lf)
Number.Format(dp_formatNumber,
rcvCheckList(10, {i}), {rcvCheckList(10, {i})}, %.4lf)
Number.Format(dp_formatNumber,
rcvCheckList(11, {i}), {rcvCheckList(11, {i})}, %.4lf)
end.
MACROEND
For those of you not familiar with my AAL script language (and that's ALL of you), things in curly braces {} return their value. For example, {rcvCheckList(6, {i})} returns the value in the 6th column of the i'th row of the rcvCheckList matrix. "RPN" means "Reverse Polish Notation", which is a stack based processor, made famous by Hewlett-Packard calculators.
Now, have you, the astute reader, already realized my stupid mistake and are laughing at me??? If not, read on.
Closing The PO
When the "Close PO" button is pressed, the program begins executing the following script:
MACRO:ClosePO
ScriptMgr.VerifySelection(dp_verifySelection,
ClosePO, {rcvID}, -1, "Please select a PO.")
DBMgr.QuerySingleRow(dp_DB, MyDB,
"select FREIGHT <totalFreight> from PURCHASE_ORDER where ID={rcvID}")
DataMatrix.Iterate(dp_iterateMatrix,
rcvCheckList, i, GetLineItemFreight, 0)
...
which verifies that there's a PO on which to operate. It then gets the current purchase order freight charges, and then it iterates through the items on the PO, skipping items already processed (which handles backorders), and then calculates the freight charges that should be distributed for the specific line item:
MACRO:GetLineItemFreight
; skip items already billed (handles backorders)
DBMgr.QuerySingleRow(dp_DB, MyDB,
"select PROCESSED <billed> from
PO_RECEIVE where PO_ITEM_ID={rcvCheckList(1, {i})}
order by RECEIVE_DATE desc, RECEIVE_TIME desc")
ScriptMgr.VerifySelection(dp_verifySelection, GetLineItemFreight, {billed}, 1, "")
; Total = SUM(qty*cost), items = {rcvCheckTotal}
; for each line item: ItemFreight = TotalFreight * (qty*cost)/TotalCost
Math.RPN(dp_RPN, "{totalFreight}
{rcvCheckList(7, {i})} {rcvCheckTotal} / * itemFreight STO")
WinMgr.Message(dp_message,
"Info",
"totalFreight={totalFreight},
7,i={rcvCheckList(7, {i})},
rcvCheckTotal={rcvCheckTotal},
itemFreight={itemFreight}")
For the first item in our list:
totalFreight=37.56
qty*cost=162.88
purchase order total (rcvCheckTotal) = 1,987.03
According to the calculation, this should assign a freight charge of $3.08 to the item. Instead, we're getting a freight charge of $6117.77, as illustrated by our little debugging message! It's as if the PO total = $1, and we're just multiplying freight and item cost! (clue!) Next stop, the RPN function.
The RPN Function
The RPN function is really trivial and doesn't deserve commenting. All operators are derived from a base class, and the method that actually performs the operation, Go is a virtual method.
void RPN::Go(void)
{
while (rpn[0])
{
AutoString token=GetToken(rpn);
RPNOperator* oper=FindOperator(token);
if (oper)
{
AutoString result;
switch(oper->GetNumParams())
{
case 1:
{
AutoString v=valueStack.top();
valueStack.pop();
result=oper->Go(v);
break;
}
case 2:
{
AutoString v1=valueStack.top();
valueStack.pop();
AutoString v2=valueStack.top();
valueStack.pop();
result=oper->Go(v1, v2);
break;
}
}
if (result != "")
{
valueStack.push(result);
}
}
else
{
valueStack.push(token);
}
}
}
Now let's take one particular function, in this case the division function:
AutoString RPNOperatorDivide::Go(const AutoString& v1, const AutoString& v2)
{
return AutoString(atof(v2) / atof(v1));
}
Wow. What could be simpler? Tracing into this, we discover that:
v1="1,987.03"
v2="162.88"
But wait!!!
The BUG!!!
The atof function is too simple! Given "1,987.03", it returns 1.0000!!!
Why didn't we see this before? Well, for one, this code was tested on invoices less than $1,000. Secondly, I later on modified the script to comma format the number. Thirdly, the boatyard doesn't get too many invoices over $1,000 and therefore we went along for quite a while before this problem occurred.
The Fix
Believe it or not, I've had exactly this same bug before, but instead of fixing it right, I chose a different solution specific to the problem at hand and thus got bit by the same bug again. The general solution is to fix it in the RPN code by stripping out the commas. Of course, this necessitated changing the signature of the methods as well, because they were const references before.
AutoString RPNOperatorDivide::Go(AutoString v1, AutoString v2)
{
v1.Replace(",", "");
v2.Replace(",", "");
return AutoString(atof(v2) / atof(v1));
}
Conclusion
The beauty of this, and the AAL technology, is that now this problem is forever fixed in all my applications written with the AAL. And because the RPN functions are in the mathAtmtn.dll, I only need to update the DLL instead of recompiling every application I've written with this technology.
Now, there IS another bug in this code. Care to guess what it is?
Postscript
My girlfriend is giving me a real hard time. I told her I didn't spell check this article, and she said "Marc Clifton! how dare you! After you get on everyone else's case about spell checking!". Of course, then she immediately found a spelling error! "You won't get a 5", she said.
