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){        oldPac3 = newOP;//We need to keep track of our count outside of the method.    }else 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 BalancerThe 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 think any of them are very clean. I want the code for this method to be concise and elegant. Any help or some guidance in the right direction would be great.This is the rewritten method. It does what needs doing, but I think it is ugly. I'm posting it because I don't want to give the impression that I'm looking for other people to solve my problems for me. I really just want to write better code. /**** 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){        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;        }    }else 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);    }    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;        }       }       }//End Missile Balancer
Writing an effective method that reflects good OO design and fixes this broke method
java;object oriented
TL;DRName 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 if you ever hit CTRL+C and CTRL+V in succession, you are probably doing something the hard way.  If logic is repeated explicitly like this, it's a sign of bad design.  Either it should be refactored or the repeated logic should be encapsulated in its own, separate method.  I'll explain what I did with this later.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);}Here you are attempting to do some error handling, which is good.  In your comment you say You cannot set selected item [in] a [JComboBox] with a negative number.  And that's very true.  But notice that you already do so before you perform your error check.  First you call combobox1.setSelectedItem(newOH) and then you check to see if newOH is less than zero.  But at that point, your code has already broken.  Always check for valid data before doing anything else with it.Now I'll show you step-by-step how I refactored your code and present you with the final version at the end.private JComboBox PAC3OperationalBox = new JComboBox();private JComboBox GEMCOperationalBox = new JComboBox();private JComboBox GEMTOperationalBox = new JComboBox();private JComboBox PAC3AdjustmentBox = new JComboBox();private JComboBox GEMCAdjustmentBox = new JComboBox();private JComboBox GEMTAdjustmentBox = new JComboBox();Notice right away how my variable names tell you exactly what they represent.  It's very easy to know what I intend each one to hold.  If I had just named them comboBox1, comboBox2, etc., or something even more arcane like pacOPold, it would be much harder to understand.  Java is by nature a verbose language which encourages explicit names for things.public enum Missile {    PAC_3, GEM_C, GEM_T;    private int count = 0;    private JComboBox operationalComboBox = null;    private JComboBox adjustmentComboBox = null;If you don't know what an enum is, now's a great time to learn.  It's a very useful data structure which lets you define exactly what its potential values are.  Here, it seems like your missile types are PAC-3s, GEM-Cs, and GEM-Ts, so that's what we have above.This is why object-oriented programming is so powerful.  A missile is a thing in the world with its own properties, and so it is here.  This enum may have been more appropriately named MissileType, depending on your preference, since technically it will hold values for multiple missiles, like the count.  But having this data structure set up is what will let us completely rewrite your method in just five lines.    public void setComboBoxes(JComboBox operationalBox, JComboBox adjustmentBox) {        operationalComboBox = operationalBox;        adjustmentComboBox = adjustmentBox;    }This will let us set the JComboBox references for each missile type upon initialization.  With these references, each missile will be able to calculate its values on its own.    public int getOperational() {        return MissileUtils.getComboBoxValue(operationalComboBox);    }I created another class called MissileUtils which performs simple utility functions.  I said that I wanted to do some sanity checking on your (Integer) combobox.getSelectedItem() statements, and here's the method where I do that (in MissileUtils`, not in the enum):public static int getComboBoxValue(JComboBox comboBox) {    Object selectedItem = comboBox.getSelectedItem();    if(Integer.class.isInstance(selectedItem)) {        return Integer.class.cast(selectedItem);    }    throw new IllegalArgumentException(The combo box must have a valid integer selected.);}It's okay if you don't know what all this does.  Basically it's just checking to make sure that the Object returned by getSelectedItem() is actually an Integer before trying to downcast it.  If it's not, if will throw an IllegalArgumentException.I might've rewritten this method like the following, but in my production environment I really dislike using instanceof and this kind of downcasting.  (In general, it creates code smell.)if(selectedItem instanceof Integer) {    return (Integer) selectedItem;}... Now back in the Missile enum, we put all the pieces together to see your refactored method:public void balance() {    if(getOperational() > count) {        int adjustment = getAdjustment() - (getOperational() - count);        adjustmentComboBox.setSelectedItem(adjustment < 0 ? 0 : adjustment);    }    count = getAdjustment();}It performs your check to see if the selected value from the first combo box is greater than the current count.  If so, it adjusts the second combo box's selected item, incorporating your error handling (checking if the new value is less than 0) using the ternary operator.  Then we set the count to the second combo box's value, since we do so in all cases.  (That was where you were copying and pasting that big block of code around.)public MissileBalancer() {    Missile.PAC_3.setComboBoxes(PAC3OperationalBox, PAC3AdjustmentBox);    Missile.GEM_C.setComboBoxes(GEMCOperationalBox, GEMCAdjustmentBox);    Missile.GEM_T.setComboBoxes(GEMTOperationalBox, GEMTAdjustmentBox);}Here's simply where we give the Missile types references to the needed JComboBoxes.private void callTheNewBalanceMethod(Missile missileType) {    missileType.balance();}... And here's how you would call the new method.AND IN THE END, THERE WAS CODEpublic class MissileBalancer {    private JComboBox PAC3OperationalBox = new JComboBox();    private JComboBox GEMCOperationalBox = new JComboBox();    private JComboBox GEMTOperationalBox = new JComboBox();    private JComboBox PAC3AdjustmentBox = new JComboBox();    private JComboBox GEMCAdjustmentBox = new JComboBox();    private JComboBox GEMTAdjustmentBox = new JComboBox();    private static enum Missile {        PAC_3, GEM_C, GEM_T;        private int count = 0;        private JComboBox operationalComboBox = null;        private JComboBox adjustmentComboBox = null;        public int getOperational() {            return MissileUtils.getComboBoxValue(operationalComboBox);        }        public int getAdjustment() {            return MissileUtils.getComboBoxValue(adjustmentComboBox);        }        public void setComboBoxes(JComboBox operationalBox, JComboBox adjustmentBox) {            operationalComboBox = operationalBox;            adjustmentComboBox = adjustmentBox;        }        public void balance() {            if(getOperational() > count) {                int adjustment = getAdjustment() - (getOperational() - count);                adjustmentComboBox.setSelectedItem(adjustment < 0 ? 0 : adjustment);            }            count = getAdjustment();        }    }    public MissileBalancer() {        Missile.PAC_3.setComboBoxes(PAC3OperationalBox, PAC3AdjustmentBox);        Missile.GEM_C.setComboBoxes(GEMCOperationalBox, GEMCAdjustmentBox);        Missile.GEM_T.setComboBoxes(GEMTOperationalBox, GEMTAdjustmentBox);    }    public static int getComboBoxValue(JComboBox comboBox) {        Object selectedItem = comboBox.getSelectedItem();        if(Integer.class.isInstance(selectedItem)) {            return Integer.class.cast(selectedItem);        }        throw new IllegalArgumentException(The combo box must have a valid integer selected.);    }    private void callTheNewBalanceMethod(Missile missileType) {        missileType.balance();    }Note: setting the combo boxes into the enum this way will cause issues if you intend to use the class with concurrency, but the solution for that is left as an exercise for the reader. :)