This week's book giveaway is in the OO, Patterns, UML and Refactoring forum. We're giving away four copies of Refactoring for Software Design Smells: Managing Technical Debt and have Girish Suryanarayana, Ganesh Samarthyam & Tushar Sharma on-line! See this thread for details.
Hello everyone. I'm a first year Uni student discovering the, ummm, joys of Java Anyways I've written some code to calculate tax based on a progressive scale. It works but the method I created to calculate the tax payable seems a bit crude. I'd like to know if it can be improved.
Any suggestions appreciated
Wish I could think of something clever to say in my signature
To start with, you have many identical statements taxPayableField.setNumber(tax); Why not call this just once, after you've figured out what the tax will be? Also, I don't see the variable "tax" actually declared anywhere.
Originally posted by Ron Newman: To start with, you have many identical statements taxPayableField.setNumber(tax); Why not call this just once, after you've figured out what the tax will be? D'oh good point... *one quick edit later* Ahhh much better. Thanks Ron Also, I don't see the variable "tax" actually declared anywhere.
I just included the code for the dotax method but heres the whole thing :
The tax scale I have to work with is : <=$5000 : 0% 5001 - 10000 : 5% 10001 - 20000 : 10.5% 20001 - 30000 : 15% 30000+ : 25.5% The dotax loop is the best way I've managed to think of to cover this but I have a nagging feeling there is a more efficient way to calculate the figure on the fly as opposed to hard coding the amounts from each lower tax bracket then working out the amount for the current level (ie tax = 250 + (income - 10001) * 0.105 - the 250 is the tax on the 5001-10000 income).
OK. It seems as if you are happy with what the code does, just not with the way it does it. What you need to do to the code is known as "refactoring" - changing the internals without changing the overall behaviour. I can actually see a few potential problems with this code. One is the way you've coded the "dotax" method (you are aware of that). Another is the way you have mixed presentation code (all that swing stuff) and business logic (tax calculations). Although your primary concern is with the tax calculations, I'll start by factoring out the tax stuff from the presentation code, to make it easier to discuss later. To start with, we have the following:
Imagine we had a separate class which was solely responsible for tax calculations. Then the GUI class could just keep a reference to a tax calculator object, and ask it whenever it needed to know about tax. The use of it might look like:
This already looks neater, but before we move on to looking at the tax calculation itself, I want to make sure this class is a fine tuned as we can manage. I can't help noticing that those variables "income" and "tax" are now only used in "buttonClicked". There doesn't seem to be any reason for them to be member variables. They could just be local variables in "buttonClicked":
There's also a bit of code which hasn't made it over from the old "dotax" method, the bit which sets the precision of the output field. We could just put this in the "buttonClicked" method, equivalent to where it was before. But does it really need to be called every time a button is clicked? If not we should do it only once, when the output field is created. Ah, but the field is created when the variable is defined. Really, I think it's time for this class to have a constructor. A useful place to gather together stuff like this:
Looking at all those member variables for all the fields and labels, I can't help noticing that most of them are never used anywhere but in the declararions. Maybe we could make things simpler by creating these in the constructor, too. The appearance of the fields seems to be order dependent, so we will need to initialize all the fields in the constructor:
Let's just pause and see where we have got to so far:
That seems to express what it does fairly concisely. So we'll leave it there. Now lets look at the tax calculation. We have already written some code to create and call our class, so there is some stuff we can write straight away:
Now, let's paste in the code we already have, as a starting point:
First, we need to remove those field accesses. They have been left in the GUI class and are not needed here. Then we need to make sure we can return something; in this case we need to define and return the "tax" variable. So we get:
Fair enough. That seems to work. We could stop there. But the code is still "clumsy". I can't help feeling that each of those "if" clauses is very similar to the others. Let's try extracting one of them into a separate method and see what happens. As an aside: this code is going to look a lot worse before it gets better, but don't worry, that often happens when refactoring. The trick is to keep a good history of previous versions so if you do decide that something is a blind alley you can easily get back to a known good point. First, we need to get rid of those else ifs. If we are going to move to some other sort of control flow we first need to make those bits of code independent of each other. I note that the tax amount always starts at 0, and only the true "if" clauses set it to anything. We could replace that with evaluating all the "if"s and adding them together:
This may have made the method seem larger and more clumsy, but it has had two important results. One is that any of the "if" clauses can now be independently replaced by a method call. The other is a little more subtle, by eliminating the final "else" and replacing it with another "if" we have made all the clauses a little bit more similar. The more similar they are, the more likely it is that we can replace them all with calls to the same method. So let's try replacing one. For reasons which may become clearer later, I shall choose the 10000-20000 tax band:
OK, we can see that with a bit of work we could make another four such methods and replace all the "if"s. But that's not where I would like it to go. I want to replace them all with calls to the same method, remember. I'm worried by all those hard-coded numbers. For our new method to be at all re-usable, they really ought to be parameters passed in from the enclosing context. We know the lower and upper incomes for each tax band, and the tax rate which applies for that band from the specification, so how about something like:
Almost there. The problem is that "250". Where does it come from? If you've ever done this sort of calculation, you'll realize that it is actually the sum of the previous tax band "maximum" values. In this case (5000-0)*0 gives 0, and (10000-5000)*0.05 gives 250. If we are clever, we can avoid hard-coding this number and instead do the calculation as we run along. If our method were coded to do the following:
if income is less than the lower limit, return 0
if income is between the two limits, return the tax rate times the anount above the lower limit
if income is above the upper limit, return the "maximum"
we could still use the "start with 0, and add the result of each call" approach we have already started with. But we wouldn't need those wierd constants. Cool. Let's start with the simplest way to code it, the same as our informal description, above:
Now we can rewrite our calling method. Let's take a look at where this has got us:
That's looking a bit better, at last. The only trick I had to pull out of the hat was to use Integer.MAX_VALUE as the upper limit for the top band. If there is a real upper limit, it can, of course be substituted. I can still see some similarity between some lines in "caclulateTaxBand", though. Is there any way we can simplify it ? Notice that if the income is exactly on the upper limit, then both (income-lower)*rate and (upper-lower)*rate give the same value. What if we were to "crop" the income value to at most "upper", then we wouldn't need the final "else":
That's good. But look, that second "if" is now redundant, as by then the imcome will always be more than lower and less than upper. So we can actually reduce the method to:
If you want, you can actually make this even shorter using the "query-colon" or "tertiary" operator, but I think this code expresses clearly enough what it is doing, so I'll stop there and recap the whole "program" (now two classes):
Is this more along the lines of what you were looking for? There are still some issues, of course. In the "real world" I would be very wary of encoding someting like tax bands and rates into the code of a program. I'd probably want to at least read them from a file, or maybe direct from the IRS. If you want to go that route we can work toward that functionality in another message. I'm also still a little irritated by the duplication of passing in the upper limit of one band and the lower limit of the next, where they are obviously related. Refactoring to eliminate that duplication is actually a first step toward reading the tax information from an external source, so I'll defer deciding how to do that until a decision is made on any external storage format. I hope this helped.