Click here to Skip to main content
Click here to Skip to main content

Diary Of A Coder--Marc's Boatyard Bug

, 14 Feb 2003
Rate this:
Please Sign up or sign in to vote.
Marc bares all in a discussion of a bug involving the atof function.

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.

License

This article has no explicit license attached to it but may contain usage terms in the article text or the download files themselves. If in doubt please contact the author via the discussion board below.

A list of licenses authors might use can be found here

Share

About the Author

Marc Clifton

United States United States
Marc is the creator of two open source projets, MyXaml, a declarative (XML) instantiation engine and the Advanced Unit Testing framework, and Interacx, a commercial n-tier RAD application suite.  Visit his website, www.marcclifton.com, where you will find many of his articles and his blog.
 
Marc lives in Philmont, NY.

Comments and Discussions

 
GeneralYou just got your 5 PinmemberAnders Molin15-Feb-03 14:32 
GeneralRe: You just got your 5 PinmemberMarc Clifton15-Feb-03 14:34 
GeneralRe: You just got your 5 PinmemberMikeBeard17-Feb-03 8:46 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.

| Advertise | Privacy | Mobile
Web04 | 2.8.140827.1 | Last Updated 15 Feb 2003
Article Copyright 2003 by Marc Clifton
Everything else Copyright © CodeProject, 1999-2014
Terms of Service
Layout: fixed | fluid