Writing an effective method that reflects good OO design and fixes this broke method

I'm working on my personal project and have recently had a bit of trouble with a method I wrote during the week.

This method compares user input from the same JComboBox. A before and after, if you will. It then makes an appropriate adjustment to a 2nd ComboBox depending on the user's input into the first box. It took me a bit of effort to figure this out. When I finally did, I was pretty happy with myself. Until I realized that the method uses a class variable and ruins any concept of good object-oriented design.

/**
** missileBalancer compares the users input before and after selection and adjusts the contents of the OH missile count box as necessary.
** @param JComboBox combobox is the combobox concerning the OP counts, JComboBox combobox1 is the combobox concerning the OH counts, prevOH is the masterOH count.
** @return
** @throws
**/
public void missileBalancer(JComboBox combobox, JComboBox combobox1, int prevOP){
    int difference = 0;
    int newOH = 0;
    int newOP = (int) combobox.getSelectedItem(); //Current OP count
    int oldOH = (int) combobox1.getSelectedItem();//Current OH count --Adjusting this by the appropriate amount is the goal of this method
    int oldOP = prevOP;
    //oldOP is the OP that we are testing against it needs to be set by the program into a var that we can use in our test.
    if(newOP  oldOP){
    difference = newOP - oldOP; 
    newOH = oldOH - difference;
    combobox1.setSelectedItem(newOH);
    if(newOH < 0){//You cannot set selected item to a jcombobox with a negative number so in all cases where newOH is less than zero we simply set the selected item to 0.
        combobox1.setSelectedItem(0);
    }
    oldPac3 = newOP;    
    }

}//End Missile Balancer

The problem is with the user of the oldPac3 variable. The method cannot always use this variable. I have variables for oldGemT and oldGemC, that this method needs to modify as appropriate. I can think of a couple ways to make this work, but I don't t

TL;DR

• Name your variables more descriptively.

• Never copy-and-paste logic; if you find yourself doing so, make a new method or analyze why you're repeating it.

• Don't pass around components if you don't have to (like JComboBoxes). In general, Swing components are global variables.

• Follow appropriate code formatting. (Proper tabs, indentation, spaces between brackets, etc.)

• Don't create unnecessary variables (i.e., int oldOP = prevOP)

• Perform validations and error handling while doing dangerous things like downcasting.

I rewrote your entire method as follows just by following some decent object-oriented design practices.

public void balance() {
    if(getOperational() > count) {
        int adjustment = getAdjustment() - (getOperational() - count);
        adjustmentComboBox.setSelectedItem(adjustment < 0 ? 0 : adjustment);
    }
    count = getAdjustment();
}

What I'm going to do is walk through your code step-by-step, explain why I made certain changes, and then present the finished code. I'll also point out any assumptions I've made in constructing my version. I had to make a number of them due to your inexplicable variable names, but it shouldn't affect the final outcome drastically even if I made a mistaken guess as to what the variables meant. You should just have to rename them to something appropriate.

So here are a few assumptions of mine right off the bat:

• prevOP means the previous count of a given missile;

• anywhere you use the infix Op or OP, it means something like "operational"; and,

• anywhere you use the infix Oh or OH, it means something like "adjustment".

So let's get started!

public void missileBalancer(JComboBox combobox, JComboBox combobox1, int prevOP){

I was skeptical of this method immediately because it's very, very rare to have to pass around GUI elements like this when using Swing. Ideally the combo boxes should all be declared as global fields in the class (i.e., private JComboBox myComboBox) so that all of your code can reference them as necessary. Also, this really breaks Java naming conventions. In general, a method name should be some kind of verb, never a noun like this. If I was skimming code and I saw something called missileBalancer, I would assume it was an object, like of the MissileBalancer class. Anything that's a noun in your code should be an actual object of some kind.

int newOP = (int) combobox.getSelectedItem();
int oldOH = (int) combobox1.getSelectedItem();

I'm not sure how you got this to compile. I received compiler errors saying "Unable to cast Object to int." In any case, that's easily solved by changing the downcast to Integer like so:

int newOP = (Integer) combobox.getSelectedItem();

This works thanks to autoboxing, and it's a handy trick to remember as you move forward with Java. Even though this bit of code now works, it's still really bad practice since it's a completely unchecked (and therefore unsafe) downcast. In other words, you're just assuming that getSelectedItem() will return something that can be cast to an Integer. We'll return to this later to add some sanity checking and error handling.

int newOH = 0;
int newOP = (int) combobox.getSelectedItem();
int oldOH = (int) combobox1.getSelectedItem();
int oldOP = prevOP;

if(newOP < oldOP){
    if(combobox.equals(pac_3OpCount)) {

I just picked this bit out because it contains a lot of your variable declarations. Your code was very, very difficult to decipher because of the names you chose for your variables. If you intend to ever work on a large scale project, you will likely be working with other people. And if you're working with other people, code readability is much, much more important than anything else. There's a common adage that goes something like "A programmer's job is 10% development, 90% maintenance," and that's incredibly true. You'll find that even if you're the only one working on a project, you'll come back to your code and look at this and ask yourself "...What in the world was I thinking here?" Believe me, it happens more often than you think.

Bottom line: pick good variable names to try to make your code self-documenting. Note that in the final version that I post below, you can pretty much know exactly what every line of code is trying to do just because I've chosen my variable and method names intuitively.

Also, if you want to be really Java-esque, follow the naming conventions. No underscores in variable names (unless they're public static final String CONSTANTS_LIKE_THIS).

if(combobox.equals(pac_3OpCount)){
    oldPac3 = newOP;//We need to keep track of our count outside of the method.
}
else if(combobox.equals(gemCOpCount)){
    oldGemC = newOP;
}
else if(combobox.equals(gemtOpCount)){
    oldGemT = newOP;
}

Do you see that this bit of code appears twice in your method? It looks like you copied and pasted it. A good rule of thumb is that

StackExchange Code Review Q#31633, answer score: 2