Sometimes when maintaining legacy software, you come across a piece of code and you just wonder;

But….why?

If you’re lucky the reasoning might end up being sound (i.e. this code is structured this way because of the great {insert-weird-issue-here} of 1979), but its probably more likely that it was just a plain old lack of knowledge or experience.

Its always a journey of discovery though, so enjoy the following post, sponsored by some legacy VB6 code that interacts with a database.

Building On Shaky Foundations

As is typically the precursor to finding weird stuff, it all started with a bug.

During the execution of a user workflow, the presence of a ‘ character in a name caused an error, which in turn caused the workflow to fail, which in turn led to some nasty recovery steps. I’m not going to get into the recovery steps in this post, but suffice to say, this workflow was not correctly wrapped in a transaction scope and we’ll just leave it at that.

Now, the keen-eyed among you might already be able to guess what the issue was. Maybe because you’ve already fought this battle before.

Deep in the bowels of that particular piece of code, an SQL statement was being manually constructed using string concatenation.

Private Function Owner_LetPrepFees_CreateTransaction(ByVal psTransAudit As String, ByVal plTransLink As Long, ByVal plTransPropertyID As Long, _
                                                     ByVal plTransOwnerID As Long, ByVal psTransFromRef As String, ByVal psTransFromText As String, _
                                                     ByVal plTransCreditorID As Long, ByVal psTransToRef As String, ByVal psTransToText As String, _
                                                     ByVal psTransReference As String, ByVal psTransType As String, ByVal psTransDetails As String, _
                                                     ByVal pbGSTActive As Boolean, ByVal pnTransAmount As Currency, ByVal psTransLedgerType As String, _
                                                     ByVal pbLetFee As Boolean, ByVal pbPrepFee As Boolean, ByVal plCount As Long) As Long
    Dim sInsertFields           As String
    Dim sInsertValues           As String
    Dim lCursorLocation         As Long

    ' replaces single "'" with "''" to avoid the "'" issue in insert statement
    psTransFromRef = SQL_ValidateString(psTransFromRef)
    psTransFromText = SQL_ValidateString(psTransFromText)
    psTransToRef = SQL_ValidateString(psTransToRef)
    psTransToText = SQL_ValidateString(psTransToText)
    psTransReference = SQL_ValidateString(psTransReference)
    psTransDetails = SQL_ValidateString(psTransDetails)

    sInsertFields = "AccountID,Created,UserId,Audit,Date"
    sInsertValues = "" & GetAccountID() & ",GetDate()," & User.ID & ",'" & psTransAudit & "','" & Format(UsingDate, "yyyyMMdd") & "'"
    If plTransLink <> 0& Then
        sInsertFields = sInsertFields & ",Link"
        sInsertValues = sInsertValues & "," & plTransLink
    End If
    If plTransPropertyID <> 0 Then
        sInsertFields = sInsertFields & ",PropertyId"
        sInsertValues = sInsertValues & "," & plTransPropertyID
    End If
    If plCount = 1& Then
        sInsertFields = sInsertFields & ",OwnerID,FromRef,FromText"
        sInsertValues = sInsertValues & "," & plTransOwnerID & ",'" & Left$(psTransFromRef, 8&) & "','" & Left$(psTransFromText, 50&) & "'"
    Else
        sInsertFields = sInsertFields & ",CreditorId,ToRef,ToText"
        sInsertValues = sInsertValues & "," & plTransCreditorID & ",'" & Left$(psTransToRef, 8&) & "','" & Left$(psTransToText, 50&) & "'"
    End If
    sInsertFields = sInsertFields & ",Reference,TransType,Details,Amount,LedgerType"
    sInsertValues = sInsertValues & ",'" & psTransReference & "','" & psTransType & "','" & IIf(pbGSTActive, Left$(Trim$(psTransDetails), 50&), Left$(psTransDetails, 50&)) & "'," & pnTransAmount & ",'" & psTransLedgerType & "'"
    If pbLetFee And psTransType = "CR" And psTransLedgerType = "JC" Then
        sInsertFields = sInsertFields & ",DisburseType"
        sInsertValues = sInsertValues & "," & dtLetFeePayment
    ElseIf pbPrepFee And psTransType = "CR" And psTransLedgerType = "JC" Then
        sInsertFields = sInsertFields & ",DisburseType"
        sInsertValues = sInsertValues & "," & dtPrepFeePayment
    End If
    If pbGSTActive Then
        sInsertFields = sInsertFields & ",GST"
        sInsertValues = sInsertValues & "," & 1&
    End If
    
    lCursorLocation = g_cnUser.CursorLocation
    g_cnUser.CursorLocation = adUseClient
    Owner_LetPrepFees_CreateTransaction = RSQueryFN(g_cnUser, "SET NOCOUNT ON; INSERT INTO Transactions (" & sInsertFields & ") VALUES (" & sInsertValues & "); SET NOCOUNT OFF; SELECT SCOPE_IDENTITY() ID")
    g_cnUser.CursorLocation = lCursorLocation

End Function

Hilariously enough, I don’t even have a VB6 Syntax Highlighter available on this blog, so enjoy the glory of formatter:none.

Clearly we had run into the same problem at some point in the past, because the code was attempting to sanitize the input (looking for special characters and whatnot) before concatenating the strings together to make the SQL statement. Unfortunately, input sanitization can be a hairy beast at the best of times, and this particular sanitization function was not quite doing the job.

Leave It Up To The Professionals

Now, most sane people don’t go about constructing SQL Statements by concatenating strings, especially when it comes to values that are coming from other parts of the system.

Ignoring SQL injection attacks for a second (its an on-premises application, you would already have to be inside the system to do any damage in that way), you are still taking on a bunch of unnecessary complexity in handling all of the user input sanitization yourself. You might not even get it right, as you can see in the example above.

Even in VB6 there exists libraries that do exactly that sort of thing for you, allowing you to use parameterized queries.

No muss, no fuss, just construct the text of the query (which is still built, but is built from controlled strings representing column names and parameter placeholders), jam some parameter values in and then execute. It doesn’t matter what’s in the parameters at that point, the library takes care of the rest.

Private Function Owner_LetPrepFees_CreateTransaction(ByVal psTransAudit As String, _
            ByVal plTransLink As Long, ByVal plTransPropertyID As Long, _
            ByVal plTransOwnerID As Long, ByVal psTransFromRef As String, ByVal psTransFromText As String, _
            ByVal plTransCreditorID As Long, ByVal psTransToRef As String, ByVal psTransToText As String, _
            ByVal plTransReference As Long, ByVal psTransType As String, ByVal psTransDetails As String, _
            ByVal pbGSTActive As Boolean, ByVal pnTransAmount As Currency, ByVal psTransLedgerType As String, _
            ByVal pbLetFee As Boolean, ByVal pbPrepFee As Boolean, ByVal plCount As Long) As Long            


    'Trims the parameters to fit inside the sql tables columns'
    psTransFromRef = Left$(Trim$(psTransFromRef), 8&)
    psTransFromText = Left$(Trim$(psTransFromText), 50&)
    psTransToRef = Left$(Trim$(psTransToRef), 8&)
    psTransToText = Left$(Trim$(psTransToText), 50&)
    psTransDetails = Left$(Trim$(psTransDetails), 50&)

    Dim sCommand    As String
    Dim sFieldList  As String
    Dim sValueList  As String
    Dim oCommand    As ADODB.Command
    Dim oParameter  As ADODB.Parameter

    Set oCommand = New Command
    oCommand.ActiveConnection = g_cnUser
    oCommand.CommandType = adCmdText
    oCommand.CommandTimeout = Val(GetRegistry("Settings", "SQL Command Timeout Override", "3600"))

    sFieldList = "AccountID, Created, UserId, Audit, Date"
    sValueList = "?" & ", GetDate()" & ", ?" & ", ?" & ", ?"
    oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , GetAccountID())
    oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , User.ID)
    oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 10, psTransAudit)
    oCommand.Parameters.Append oCommand.CreateParameter(, adDate, adParamInput, , UsingDate)

    If plTransLink <> 0& Then
        sFieldList = sFieldList & ", Link"
        sValueList = sValueList & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , plTransLink)
    End If

    If plTransPropertyID <> 0 Then
        sFieldList = sFieldList & ", PropertyId"
        sValueList = sValueList & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , plTransPropertyID)
    End If

    If plCount = 1& Then
        sFieldList = sFieldList & ", OwnerID" & ", FromRef" & ", FromText"
        sValueList = sValueList & ", ?" & ", ?" & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , plTransOwnerID)
        oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 8, psTransFromRef)
        oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 50, psTransFromText)
    Else
        sFieldList = sFieldList & ", CreditorId" & ", ToRef" & ", ToText"
        sValueList = sValueList & ", ?" & ", ?" & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , plTransCreditorID)
        oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 8, psTransToRef)
        oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 50, psTransToText)
    End If

    sFieldList = sFieldList & ", Reference" & ", TransType" & ", Details" & ", Amount" & ", LedgerType"
    sValueList = sValueList & ", ?" & ", ?" & ", ?" & ", ?" & ", ?"
    oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, 8, plTransReference)
    oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 2, psTransType)
    oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 100, psTransDetails)
    oCommand.Parameters.Append oCommand.CreateParameter(, adCurrency, adParamInput, , pnTransAmount)
    oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 2, psTransLedgerType)

    If pbLetFee And psTransType = "CR" And psTransLedgerType = "JC" Then
        sFieldList = sFieldList & ", DisburseType"
        sValueList = sValueList & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , dtLetFeePayment)
    ElseIf pbPrepFee And psTransType = "CR" And psTransLedgerType = "JC" Then
        sFieldList = sFieldList & ", DisburseType"
        sValueList = sValueList & ",?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , dtPrepFeePayment)
    End If

    If pbGSTActive Then
        sFieldList = sFieldList & ", GST"
        sValueList = sValueList & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adBoolean, adParamInput, , pbGSTActive)
    End If

    Set oParameter = oCommand.CreateParameter(, adInteger, adParamReturnValue)
    oCommand.Parameters.Append oParameter
    sCommand = "INSERT INTO Transactions (" & sFieldList & ") VALUES (" & sValueList & "); SELECT ?=SCOPE_IDENTITY()"
    oCommand.CommandText = sCommand
    oCommand.Execute , , adExecuteNoRecords
    Exit Function    
    
    Owner_LetPrepFees_CreateTransaction = oParameter.Value
    Set oCommand = Nothing
    
End Function

Of course, because VB6 is a positively ancient language, its not quite as easy as all that.

Normally, in .NET or something similar, if you are constructing a parameterized query yourself, you’d probably use a query builder of some sort, and you would name your parameters so that the resulting query was easy to read as it goes through profilers and logs and whatnot.

No such luck here, the only parameter placeholder that seems to be supported is the classic ?, which means the parameters are positional. This can be dangerous when the code is edited in the future, but if we’re lucky, maybe that won’t happen.

Its still better than jamming some strings together though.

Conclusion

I think of this as a textbook case for never just “fixing the bug” as its stated. Sure, we could have just edited the SQL_ValidateString function to be more robust in the face of a particular arrangement of special characters, but that’s just putting a bandaid on a bad solution.

Instead we spent the time to work with the language and the constructs available to put a solution in place that is a lot more robust in the face of unexpected input.

And lets be honest, at this point, the only input we get is the unexpected kind.

I expect accounting software to make some pretty convincing guarantees about the integrity of its data over time.

From my experience, such software generally restricts the users capability to change something once it has been entered. Somewhat unforgiving of innocent mistakes (move money to X, whoops, now you’ve got to move to back and there is a full audit trail of your mistake), but it makes for a secure system in the long run.

Our legacy software is very strict about maintaining the integrity of its transactional history, and it has been for a very long time.

Except when you introduce the concept of database restores, but that’s not a topic for this blog post.

Nothing is perfect though, and a long history of development by a variety of parties (some highly competent, some….not) has lead to a complicated system that doesn’t play by its own rules every now and then.

Its like a reverse Butterfly Effect, changing the present can unfortunately change the past.

Its All Wrong

A natural and correct assumption about any reports that come out of a piece of accounting software, especially one that focuses on transactions, is that it shouldn’t matter when you look at the report (today, tomorrow, six months from now), if you’re looking at data in the past, it shouldn’t be changing.

When it comes to the core transactional items (i.e. “Transferred $200 to Bob”) we’re good. Those sorts of things are immutable at the time when they occur and it doesn’t matter when you view the data, its always the same.

Being that this is a blog post, suspiciously titled “Protecting the Timeline”, I think you can probably guess that something is rotten in the state of Denmark.

While the core transactional information is unimpeachable, sometimes there is meta information attached to a transaction with less moral integrity. For example, if the transaction is an EFT payment exiting the system, it needs to record bank account details to be compliant with legislation (i.e. “Transferred $200 to Bob (EFT: 123-456, 12346785)”).

Looking at the system, its obvious that the requirement to capture this additional information came after the original implementation, and instead of capturing the entire payload when the operation is executed, the immutable transaction is dynamically linked to the entities involved and the bank account details (or equivalent) are loaded from the current state of the entity whenever a report is created.

So we know unequivocally that the transaction was an EFT transaction, but we don’t technically know which account the transfer targeted. If the current details change, then a re-printed report will technically lie.

Freeze Frame

The solution is obvious.

Capture all of the data at the time the operation is executed, not just some of it.

This isn’t overly difficult from a technical point of view, just hook into the appropriate places, capture a copy of the current data and store it somewhere safe.

Whenever the transactions are queried (i.e. in the report), simply load the the same captured data and present it to the user.

Of course, if the requirements change again in the future (and we need to show additional information like Bank Name or something), then we will have to capture that as well, and all previous reports will continue to show the data as it was before the new requirement. That’s the tradeoff, you can’t capture everything, and whatever you don’t capture is never there later.

But what about the literal mountain of data that already exists with no captured meta information?

Timecop!

There were two obvious options that we could see to deal with the existing data:

1. Augment the reporting/viewing logic such that it would use the captured data if if existed, but would revert back to the old approach if not.
2. Rewrite history using current information and “capture” the current data, then just use the captured data consistently (i.e. in reports and whatnot).

The benefit of option one is that we’re just extending the logic that has existed for years. When we have better data we can use that, but if not, we just fall back to old faithful. The problem here is one of complication, as any usages now need to do two things, with alternate code paths. We want to make the system simpler over time (and more reliable), not harder to grok. Also, doing two operations instead of one, combined with the terrible frameworks in use (a positively ancient version of Crystal Reports) led to all sorts of terrible performance problems.

Option two basically replicates the logic in option one, but executes it only once when we distribute the upgrade to our users, essentially capturing data at that point in time, which then becomes immutable. From that point forward everything is simple, just use the new approach, and all of the old data is the same as it would have been if the reports had of been printed out normally.

If you couldn’t guess, we went with option two.

Conclusion

What we were left with was a more reliable reporting system, specifically focused around the chronological security of data.

Also, I’m pretty sure I made up the term “chronological security”, but it sounds cool, so I’m pretty happy.

I honestly don’t know what led to the original decision to not capture key parts of the transaction in an immutable fashion, and with hindsight its easy for me to complain about it. I’m going to assume the group (or maybe even individual) that developed the feature simply did not think through the ramifications of the implementation over time. Making good software requires a certain level of care and I know for a fact that that level of care was not always present for our long-suffering legacy software.

We’re better now, but that’s still a small slice of the overall history pie, and sometimes we build on some very shaky foundations.

Building new features and functionality on top of legacy software is a special sort of challenge, one that I’ve written about from time to time.

To be honest though, the current legacy application that I’m working with is not actually that bad. The prior technical lead had the great idea to implement a relatively generic way to execute modern . NET functionality from the legacy VB6 code thanks to the magic of COM, so you can still work with a language that doesn’t make you sad on a day to day basis. Its a pretty basic eventing system (i.e. both sides can raise events that are handled by the other side), but its effective enough.

Everything gets a little bit tricksy when windowing and modal dialogs are involved though.

One Thing At A Time

The legacy application is basically a multiple document interface (MDI) experience, where the user is free to open a bunch of different entities and screens at the same time. Following this approach for new functionality adds a bunch of inherent complexity though, in that the user might edit an entity that is currently being displayed elsewhere (maybe in a list), requiring some sort of common, global channel for saying “hey, I’ve updated entity X, please react accordingly”.

This kind of works in the legacy code (VB6), because it just runs global refresh functions and changes form controls whenever it feels like it.

When the .NET code gets involved though, it gets very difficult to maintain both worlds in the same way, so we’ve to isolating all the new features from the legacy stuff, primarily through modal dialogs. That is, the user is unable to access the rest of the application when the .NET feature is running.

To be honest, I was pretty surprised that we could open up a modal form in WPF from an event handler started in VB6, but I think it worked because both VB6 and .NET shared a common UI thread, and the modality of a form is handled at a low level common to both technologies (i.e. win32 or something).

We paid a price from a user experience point of view of course, but we mostly worked around it by making sure that the user had all of the information they needed to make a decision on any screen in the .NET functionality, so they never needed to refer back to the legacy stuff.

Then we did a new thing and it all came crashing down.

Unforgettable Legacy

Up until fairly recently, the communication channel between VB6 and .NET was mostly one way. VB6 would raise X event, .NET would handle it by opening up a modal window or by executing some code that then returned a response. If there was any communication that needed to go back to VB6 from the .NET, it always happened after the modal window was already closed.

This approach worked fine until we needed to execute some legacy functionality as part of a workflow in .NET, while still having the .NET window be displayed in a modal fashion.

The idea was simple enough.

• Use the .NET functionality to identify the series of actions that needed to be completed
• In the background, iterate through that list of actions and raise an event to be handled by the VB6 to do the actual work
• This event would be synchronous, in that the .NET would wait for the VB6 to finish its work and respond before moving on to the next item
• Once all actions are completed, present a summary to the user in .NET

We’d actually used a similar approach for a different feature in the past, and while we had to refactor some of the VB6 to make the functionality available in a way that made sense, it worked okay.

This time the legacy functionality we were interested in was already available as a function on a static class, so easily callable. I mean, it was a poorly written function dependent on some static state, so it wasn’t a complete walk in the part, but we didn’t need to do any high-risk refactoring or anything.

Once we wrote the functionality though, two problems became immediately obvious:

1. The legacy functionality could popup dialogs, asking the user questions relevant to the operation. This was actually kind of good, as one of the main reasons we didn’t want to reimplement was because we couldn’t be sure we would capture all the edge cases so using the existing functionality guaranteed that we would, because it was already doing it (and had been for years). These cases were rare, so while they were a little disconcerting, they were acceptable.
2. Sometimes executing the legacy functionality would murder the modal-ness of the .NET window, which led to all sorts of crazy side effects. This seemed to happen mostly when the underlying VB6 context was changed in such a way by the operation that it would historically have required a refresh. When it happened, the .NET window would drop behind the main application window, and the main window would be fully interactable, including opening additional windows (which would explode if they too were modal). There did not seem to be a way to get the original .NET window back into focus either. I suspect that there were a number of Application.DoEvents calls secreted throughout the byzantine labyrinth of code that were forcing screen redraws, but we couldn’t easily prove it. It was definitely broken though.

The first problem wasn’t all that bad, even if it wasn’t great for a semi-automated process.

The second one was a deal-breaker.

Freedom! Horrible Terrifying Freedom!

We tried a few things to “fix” the whole modal window problem, including:

• Trying to restore the modal-ness of the window once it had broken. This didn’t work at all, because the window was still modal somehow, and technically we’d lost the thread context from the initial .ShowDialog call (which may or may not have still been blocked, it was hard to tell). In fact, other windows in the application that required modality would explode if you tried to use them, with an error similar to “cannot display a modal dialog when there is already one in effect”.
• Trying to locate and fix the root reason why the modal-ness was being removed. This was something of a fools errand as I mentioned above, as the code was ridiculously labyrinthian and it was impossible to tell what was actually causing the behaviour. Also, it would involve simultaneously debugging both VB6 and .NET, which is somewhat challenging.
• Forcing the .NET window to be “always on top” while the operation was happening, to at least prevent it from disappearing. This somewhat worked, but required us to use raw Win32 windowing calls, and the window was still completely broken after the operation was finished. Also, it would be confusing to make the window always on top all the time, while leaving the ability to click on the parts of the parent window that were visible.

In the end, we went with just making the .NET window non-modal and dealing with the ramifications. With the structures we’d put into place, we were able to refresh the content of the .NET window whenever it gained focus (to prevent it displaying incorrect data due to changes in the underlying application), and our refreshes were quick due to performance optimization, so that wasn’t a major problem anymore.

It was still challenging though, as sitting a WPF Dispatcher on top of the main VB6 UI thread (well, the only VB6 thread) and expecting them both to work at the same time was just asking too much. We had to create a brand new thread just for the WPF functionality, and inject a TaskScheduler initialized on the VB6 thread for scheduling the events that get pushed back into VB6.

Conclusion

Its challenging edge cases like this whole adventure that make working with legacy code time consuming in weird and unexpected ways. If we had of just stuck to pure .NET functionality, we wouldn’t have run into any of these problems, but we would have paid a different price in reimplementing functionality that already exists, both in terms of development time, and in terms of risk (in that we don’t full understand all of the things the current functionality does).

I think we made the right decision, in that the actual program functionality is the same as its always been (doing whatever it does), and we instead paid a technical price in order to get it to work well, as opposed to forcing the user to accept a sub-par feature.

Its still not immediately clear to me how the VB6 and .NET functionality actually works together at all (with the application windowing, threading and various message pumps and loops), but it does work, so at least we have that.

I do look forward to the day when we can lay this application to rest though, giving it the peace it deserves after many years of hard service.

Yes, I’ve personified the application in order to empathise with it.

You may have noticed the Crystal Reports tag attached to this post.

Yes.

Crystal Reports.

The much maligned, overly complex and generally poorly regarded report generation software that I’m sure all software developers have had to touch at some point or another.

Well, it was finally my turn to touch it. I’ve managed to dodge it for the last 10 years of my career, but it looks like my number finally came up and I needed to delve into the belly of the beast to try and figure out why it was generating incorrect output.

Unluckily for me it wasn’t even a newer version of Crystal Reports, it was an old version. For all I know the component might be amazing now, but I suspect it has its reputation as a result of the older versions anyway, so I at least knew what I was getting myself into.

Reporting Issues

I’m currently working on a legacy line-of-business application written primarily in VB6. We are in the process of (slowly) moving it into .NET (which is good), but being that this is a gradual process, we still need to maintain and support the older code, ensuring that it continues to work as it always has.

This particular application, like many line-of-business applications, has the ability to generate many different varieties of reports. Suffice to say though, they are important to the users of the software.

The reports are generated using an old ActiveX version of Crystal Reports, for use with VB6. I don’t actually know what version it is.

There was a bug logged that a particular section of one of the reports generated by the application was accumulating values between rows for a particular field.

That’s right, values were accumulating, meaning that a column in the first row would have some text in it like “Discombobulation Widget”, and the second row would have “Discombobulation Widget Frazwozzer Widget”, when it should have just said “Frazwozzer Widget”. The next row was worse, containing 3 different values jammed together. The weirdest part was, it was only happening for some of the rows, not all of them. Only rows representing data of a particular type would exhibit this behaviour, others would be fine.

This was a new thing, as apparently the report had worked correctly in the past.

Of course, the obvious cause would be that the dataset behind the report was wrong, and contained the wrong values for those fields. Something that could be dealt with inside SQL or maybe VB6 if you’re unlucky.

That was not the cause. The data was fine.

It was coming from inside the report…

Maw of Madness

The reports in the application are defined as .rpt files, which seem to be the Crystal Reports file format. The files are binary of course, so diff tools are useless for tracking changes,

Anyway, after some digging I figured out that Crystal Reports has the concept of formulas, which seem to define the calculations behind a field. Fields are what you use to show actual data in the report, and seem to be derived from the dataset supplied to the report. In the case of this report, the dataset was being supplied via an SQL query on a temporary table that the application had constructed prior to running the report.

The field that was accumulating values between rows was called “details”, which isn’t all that important.

It had a formula though, which looked like this:

stringVar sDetails;

if {@TableType} = 2 and ({Transactions.TransType} = "RE" or {Transactions.TransType} = "LE") then
(
    if IsNull({Transactions.FromText}) or {Transactions.FromText} = "" then
        if IsNull({Transactions.FromRef}) or {Transactions.FromRef} = "" then
            sDetails:={Transactions.Details} 
        else
            sDetails:={Transactions.Details} + " (" + {Transactions.FromRef}+")"
    else
        sDetails:={Transactions.Details} + " (" + {Transactions.FromText} + ")";
)
else if {@TableType} = 0 then
(
     //( //Bank Ledger
    if not isNull({Owners2.FileAs}) and {Owners2.FileAs} <> "" then
        sDetails:={Owners2.FileAs} + " - " + {Transactions.Details}
    else if isNull({Owners2.ID}) and not isNull({Creditors2.FileAs}) and {Creditors2.FileAs} <> "" then
        sDetails:={Creditors2.FileAs} + " - " + {Transactions.Details}        
    else                
        sDetails:={Transactions.Details}
)
else if {@TableType} = 4 then //Sale
(
   if {Transactions.LedgerType}="C" then
        if isnull({Transactions.FromText}) or {Transactions.FromText}="" then
            sDetails:={Transactions.Details}
        else
            sDetails:={Transactions.Details}+" ("+{Transactions.FromText}+")"
)
else if {@TableType} = 5 then //Tenant
(
    if not isNull({ID.ActionID}) then
        sDetails:={Actions.Subject}
    else if not isNull({ID.LogID}) then
        sDetails:={Log.Details}
    else
        sDetails:={Transactions.Details}
)
else
    sDetails:={Transactions.Details};


if {Transactions.LedgerType} ="D" then
    if isnull({Transactions.Paidby}) or {Transactions.Paidby}="" then
        sDetails:=sDetails
    else
        sDetails:=sDetails+"("+{Transactions.Paidby}+")";


if {ID.TextField2}="EFT" or {ID.TextField2}="JNL" then
    sDetails:=sDetails + " " + {ID.TextField3};

sDetails;

By the way, that’s not any normal language to my knowledge. Its some Crystal Reports specific script.

Regardless of what it is, it’s not good code, but it doesn’t look like it has any obvious bugs that would be causing it to carry over data for previous rows.

I’ll cut right to the chase, even though it took me quite a while to figure it out.

See that variable declaration at the top of the code block above?

stringVar sDetails;

That’s not locally scoped.

I know. How could that possibly not be locally scoped. Its right there. Its literally declared inside the script block.

As a result of it not being locally scoped, the values held within it can bleed over when the formula is evaluated for each row. That means there is a code path somewhere in those nested if statements that does not set the variable directly, and instead uses the value already there, which is the root cause of the accumulating values.

That piece of code is at the bottom, executed when the LedgerType is D. It takes the current value and just appends some additional information. When the TableTable is 4 (Sale) and the LedgerType is D, there is no code path that initialises the sDetails variable, meaning it keeps whatever value it had in it from the previous run.

The fix?

Initialise the variable to the empty string at the top and run away as quickly as possible.

Protect Yourself

Well, maybe not run away as quickly as possible.

I didn’t mention it earlier, but there were no tests for this report. I think you probably saw that coming though.

Nothing was being run that would automatically check that is contents were approximately what you would expect. Being that the report had worked in the past, this sort of test would have caught the error (which was introduced when some “improvements” were being made) very close to when the code was actually written, making it much easier to fix.

I would like to rectify the lack of test at some stage, but I know putting that sort of test in place, for the very first time, will not be easy, and will be extremely time consuming.

Testing the code in an isolated way seems to be impossible, as parts of it live in VB6 and other parts live in Crystal Reports, which seems to have a hard dependency on an actual database (actually a temporary table in the database that the application puts together to make the report work, but same idea). That means no easily stubbed out unit test.

The only way I can see to test it end-to-end would be to create a functional test that runs the application itself to create the entries needed for the report, generates the report and then checks it against a screenshot or something similarly simple and robust.

I have not yet done this, but I hope to soon.

Conclusion

Typically I like to close out my blog posts with some sort of lesson learned, or something that I have taken away from the situation that may help me (and maybe you) in the future.

I’m not entirely sure what I can take away from this experience.

Don’t trust third party components?

Sometimes variables that appear local aren’t?

Maybe the lesson is that there isn’t always a lesson, and sometimes software is just super depressing.