The Problem with Embeds, Part 5: The Invoice App
This is the fifth article in an ongoing series on Clarion embed points and how almost all Clarion developers use them in the wrong way. If you haven't read the first article in the series, then you might find that statement offensive. You might be thinking, "What does Harms know anyway? I'm a professional developer. I know how to write embed code."
If that's you, stand down from red alert. First, I include myself among the offenders. I've written embed code for years, and I've almost always done it wrong, even when the code itself was pretty dang good. Please, just go back to Part 1, read it, and then tell me where I'm off base.
Once again, here's the idea in a nutshell:
Embed code should not contain business logic.
Put another way, your app contains code that gives it unique value to your users/customers. Don't bury that code inside embed points. You can call it from embed points (you'll almost certainly have to) but don't let it live there. Good code needs a home, a place where it can be conceived, nurtured, and yes, tested and disciplined.
So what does that home look like?
I think it almost always looks like a class library, and I've taken some heat for saying that. In this article I'll defend my point of view, and I'll do it by beginning a transformation of the Invoice application which ships with Clarion.
The Invoice app
The example Invoice app isn't something you'd want to use in real life, but I've chosen it because we all have it, and because many of us have, at one time or another, had to deal with invoicing.
Figure 1 shows the customer order window, which has a child browse showing line items for each invoice.
Figure 1. Browsing orders for a customer
Figure 2 shows the order detail form. Yeah, I know, none of this is pretty. But that's okay - it's old example code, and Clarion devs sometimes have to deal with old code.
Figure 2. Changing a detail record
Here's the Detail file structure:
Detail FILE,DRIVER('TOPSPEED'),PRE(DTL),CREATE,BINDABLE,THREAD
KeyDetails KEY(DTL:CustNumber,DTL:OrderNumber,DTL:LineNumber),NOCASE,OPT,PRIMARY
Record RECORD,PRE()
CustNumber LONG
OrderNumber LONG
LineNumber SHORT
ProductNumber LONG
QuantityOrdered DECIMAL(7,2)
BackOrdered BYTE
Price DECIMAL(7,2)
TaxRate DECIMAL(6,4)
TaxPaid DECIMAL(7,2)
DiscountRate DECIMAL(6,4)
Discount DECIMAL(7,2)
Savings DECIMAL(7,2)
TotalCost DECIMAL(10,2)
END
END There's a fair bit of embed code in the UpdateDetail form. The part I'll tackle first is the code that calculates totals and taxes:
CalcValues ROUTINE
IF DTL:TaxRate = 0 THEN
IF DTL:DiscountRate = 0 THEN
DTL:TotalCost = DTL:Price * DTL:QuantityOrdered
ELSE
LOC:RegTotalPrice = DTL:Price * DTL:QuantityOrdered
DTL:Discount = LOC:RegTotalPrice * DTL:DiscountRate / 100
DTL:TotalCost = LOC:RegTotalPrice - DTL:Discount
DTL:Savings = LOC:RegTotalPrice - DTL:TotalCost
END
ELSE
IF DTL:DiscountRate = 0 THEN
LOC:RegTotalPrice = DTL:Price * DTL:QuantityOrdered
DTL:TaxPaid = LOC:RegTotalPrice * DTL:TaxRate / 100
DTL:TotalCost = LOC:RegTotalPrice + DTL:TaxPaid
ELSE
LOC:RegTotalPrice = DTL:Price * DTL:QuantityOrdered
DTL:Discount = LOC:RegTotalPrice * DTL:DiscountRate / 100
LOC:DiscTotalPrice = LOC:RegTotalPrice - DTL:Discount
DTL:TaxPaid = LOC:DiscTotalPrice * DTL:TaxRate / 100
DTL:TotalCost = LOC:DiscTotalPrice + DTL:TaxPaid
DTL:Savings = LOC:RegTotalPrice - DTL:TotalCost
END
ENDCalcValues is called whenever the OK button is clicked, which means whenever the record is saved, on an insert or an update.
And I don't know about you, but I see one big bug staring me in the face already.
This routine updates values in a record, so that's hard data going into the database. If I retrieve a record from the database which previously had DTL:Discount and DTL:Savings values set, and I set DTL:TaxRate to zero and DTL:DiscountRate to zero, what will happen when I call CalcValues? This one line of code executes:
DTL:TotalCost = DTL:Price * DTL:QuantityOrdered
It appears there's no change at all to the discount and savings values.
But how can I test this? A few options come to mind. I could put in (shudder) Stop statements or Message statements. Or I could use OutputDebugString and DebugView. Or perhaps I could just add the rest of the fields to the screen so I can see what's actually happening to the data.
In Figure 3 I've added the calculated fields as display strings. I've also added a button to force a call to the CalcValues routine so I can see the results of several changes at one time.
Figure 3. Displaying the calculated fields
In Figure 3 I've added a new detail line, and I've selected an Aster, quantity 2. I've pressed the Recalculate Values button, and you can see that the values all appear to be correct.
In Figure 4 I've changed the tax rate to 5%.
Figure 4. Setting the tax rate
So far, so good.
In Figure 5 I've added a 10% discount.
Figure 5. Adding a discount
Whoops. The discount is calculated as $3.00, which is correct, at least for the total if not for the per-item price (the Invoice app has no formal notion of "extended" price). Why then is the savings only $1.65?
In Figure 6, I've set tax rate to 5% and the discount to 1%.
Figure 6. A 1% discount and a negative savings
According to Figure 6, offering a 1% discount to a customer results in a negative savings. Interesting.
In Figure 7, the wheels have well and truly come off.
Figure 7. Setting the tax rate to zero
I've set the discount rate and the tax rate to zero, but I'm still showing a tax paid of $1.50, a discount of 30 cents, and a savings of $-1.19.
Wonderful.
Of course you'd never write code that's this buggy, and I shouldn't be too hard on the author of the app either since it's nothing more than a demo.
We've all written code like this, with less-obvious bugs. Sometimes it's a long, long time before you even find out the bug is there. And what happens when you need to reuse the code somewhere else, say in a web version of your invoicing application (built with Capesoft's snazzy NetTalk 5, perhaps)? You make a copy, and guess what, you've just made a copy of an obscure bug. Let's hope you remember to fix the problem in both places when it finally comes to light.
But how did these bugs happen in the first place? Because Clarion's embed system, as powerful as it is, encourages untestable business logic that's too tightly bound to the user interface. It's easy to write code that's hard to test.
The solution is to take the code out of the embed and put it somewhere it can be both reused and tested.
The beautiful black box
The important thing about business logic is that you should be able to use it without having to know exactly how it works. That may sound a bit counter intuitive - after all, if it's your own code, shouldn't you know how it works? If you wrote it, yes, you should. But we all use black box code all the time. How many Clarion devs really know what's going on in Windows when a mouse is moved or a button clicked? How about the file driver system? Or even ABC? We don't need to know how these things all work; we just need to know how to use them.
You can take the same approach to your own software development. You don't need to remember how every bit of your application works (if you're like me, you can't remember all that stuff anyway). You just need to do a good job writing it in the first place, and you also need to write good tests for your code so you have confidence it's still doing what you originally wrote it to do.
Embed code is incredibly tedious to test, and almost impossible to test thoroughly, because you typically have to run the application, do a bunch of stuff on screen, and look at the results to see if the code works. That's the only way to test the CalcValues routine as long as it stays in the embed point.
But what's the first thing that happens if you take the routine out of an embed point and put it into a standalone source file? It won't compile. That's because it has references to app-specific data: the fields in the Details record. The code is entangled with the database definition, and that's not a good thing for either testing or re-use. Same goes for the user interface: if your code requires a certain window or control to be in existence, you're going to have a lot of difficulty with testing.
When it comes to black boxes containing business logic, you really don't want anything that's specific to one application, it's database, or its user interface. All you really want to do is feed some data into that box and get back some other data.
In the case of the CalcValues routine code, you want to feed in:
- Unit price
- Quantity
- Discount rate
- Tax rate
and you want to get back
- Total price
- Tax amount
- Discount amount
- Total cost
- Savings
How to design a black box
Once you know the inputs and outputs, it's easy to design your black box. You just need to decide whether to create your black box as a procedure or a class.
Here's how you might declare a black box procedure to replace the CalcValues routine:
CalculateInvoiceDetailValues procedure(*decimal price,|
long quantity,|
*decimal discountRate,|
*decimal taxRate,|
*decimal taxAmount,|
*decimal discountAmount,|
*decimal totalCost,|
*decimal savings)Note that the procedure doesn't have any dependency on the Detail record, or on anything else that's specific to the Invoice app.
To use the procedure in the Invoice app you'd simply call it like this:
CalculateInvoiceDetailValues(DTL:Price,|
DTL:QuantityOrdered,|
DTL:DiscountRate,|
DTL:TaxRate,|
DTL:TaxPaid,|
DTL:Discount,|
DTL:TotalCost,|
DTL:Savings)And here's how you could declare a similar black box class to replace the CalcValues routine:
InvoiceDetail Class
Init procedure(*decimal price,|
long quantity,|
*decimal discountRate, |
*decimal taxRate)
GetTotalCost procedure(*decimal totalCost)
GetTaxAmount procedure(*decimal taxAmount)
GetDiscountAmount procedure(*decimal discountAmount)
GetSavings procedure(*decimal savings)
End To use the class you'd write something like this:
InvoiceDetail.Init(DTL:Price,|
DTL:QuantityOrdered,|
DTL:DiscountRate,|
DTL:TaxRate)
InvoiceDetail.GetTotalCost(DTL:TotalCost)
InvoiceDetail.GetTaxAmount(DTL:TaxPaid)
InvoiceDetail.GetDiscountAmount(DTL:Discount)
InvoiceDetail.GetSavings(DTL:Savings)Notice that in both cases decimal values are passed by address, which is a Clarion requirement.
There isn't all that much difference between the two approaches so far. The procedural syntax takes a little less typing, but it's also slightly more difficult to understand. It's easier to make a mistake with eight parameters than it is with four. In the case of the class if you wanted to be very sure it was used correctly you could always create individual setter methods for each of the four initialization parameters (and report an error if not all were set before a value was requested).
The real problem with the procedural approach, however, is its inflexibility. In its current configuration you must pass all eight parameters, even if you only want one value back. You could make the parameters omittable, but then you have to make sure you omit the right parameters, which can also be tricky.
As well, the procedural code can really only be used as procedural code. That is, you're using the class pretty much as a set of procedures, but you can also use the class instance itself. You could, if you wish, create an InvoiceHeader object that contains a list of InvoiceDetail objects, such that you could model the entire invoice in memory. It's very difficult to do anything like that with procedural code.
It's also easier to extend the InvoiceDetail class without breaking existing functionality. As written, the CalcValues routine calculates total cost including taxes but not extended price excluding taxes. Adding a method to get the extended price would have zero effect on code already using the class, whereas with the procedural solution you'd have to make the additional parameter omittable.
In this very limited set of calculations and results, the procedural approach is already starting to show some strain. What happens when things get really interesting, as when you need to start tracking multiple taxes instead of a single tax?
In other words, the evolution of programming from procedural code to object-oriented code isn't without very good reason. OOP really does give you a greatly expanded toolbox for solving programming problems.
Testing
Whichever approach you take, procedural or class, your code needs to be testable. And I don't mean the kind of testing that involves someone running an app, entering values, and making sure the results are correct. Yes, you do still need to do that, but by the time your code gets to that point you should already have a high level of confidence in the underlying logic.
The kind of testing I'm talking about is called "unit testing". The "unit" refers to the smallest testable parts of the application. In the case of the procedural version of the above code, it would involve testing a single call to the CalculateInvoiceDetailValues procedure. In the case of the class version, you would have a test for each of the methods which return a value.
The idea behind unit testing is that if all of the individual objects (or procedures) that make up your application work properly you're much more likely to have an application that works properly as a whole.
As well, unit testing is a great way to detect the introduction of new bugs as you make modifications to existing code. If you have a large suite of unit tests, and you make a change to some core code that affects how other methods or classes (or procedures) work, any bugs introduced by those changes stand a good chance of showing up in the unit tests.
One of the keys to successful unit testing is to automate the process as much as possible. You don't want to have to manually launch each test; instead, you want a way to run a whole bunch of tests and see the results. It's also advantageous to run all the unit tests as part of every build, so that bugs can be found as quickly as possible.
Another key is to avoid outside dependencies in unit tests, such as a requirement to use an actual database or a user interface widget. You want your unit tests to run as quickly as possible, and with little to no chance of breakage due to something on the outside of the tested code not being set up correctly. Tests that take too long to run or are dependent on proper configuration are tests that tend not to get run.
In the case of the CalcValues routine code, isolation is pretty easy to achieve. Although the routine code operates directly on the fields in the Detail record, this isn't a requirement. The logic can all be made internal to the class; that way the class can be used anywhere at all without modification, and the only embed code needed is whatever it takes to assign the values to and from the class. Presto! You've successfully removed the business logic from the embed point!
What's next
There are a couple of ways to approach unit testing. One is to take existing code, refactor it if necessary to make it testable, and then write a bunch of tests to see whether it's working as you expect. The other is to start by writing tests, and then create the code to make those tests compile and then pass. This latter approach is called test-driven development (TDD) and can be enormously useful. Although I do have some existing code to work with, it's broken enough that I'm going to start fresh, using the routine code only as a reference.
In the next article I'll walk through the process of creating and running unit tests in Clarion, and then I'll flesh out the InvoiceDetail class and provide a refresher on some of the basics of object-oriented programming.
The downloadable source zip contains the modified Invoice app.