Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a main Class MyOne which has an inner helper Class MyTwo. I have created a private method loadValues which will save all the local class variable data into an object, which will be used in the main class for applying different logic.

The code provided below is only a rough sample.The actual code is pretty complex.The problem is that the section between /***START***/ and /***END***/ is really long and I want each of my methods to follow the rule of thumb.

Please provide ideas as to how I can handle the code within section /***START***/ and /***END***/. Should I be moving this section into another method? If so, how do I return all the modified variables myTwo.x,myTwo.z,myTwo.y back into the mainExecution method? I'm new to Java so any help would be very useful.

public class MyOne {

    public mainExecution() {

        MyTwo myTwo = loadValues();
        List dataList = getData();

        /****START*****/
        if (dataList.size() > 0){

            for (int j = 0; j <dataList.size(); j++){
                currData = dataList.getDataRecord(j);
                myTwo.b = currData.getStringVal();

                if (myTwo.b.contains("New Detail")){
                    myTwo.z = (myTwo.x + myTwo.y)/60;
                    myTwo.y = myTwo.x;
                }
                else if (myTwo.b.contains("Old Detail")){
                    myTwo.z = (myTwo.y - myTwo.x);
                }
                else{
                    //More logic
                }
            }
        }
        /****END*****/

        private MyTwo loadValues(){

            MyTwo myTwo = new MyTwo();
            myTwo.x = 12;
            myTwo.y = 20;
            myTwo.z = 55;
            myTwo.a = "My Test Class";
            myTwo.b = "";
            return myTwo
        }

        private class MyTwo{
            int x;
            int y;
            int z;
            String a;
            String b;
        }

    }
}
share|improve this question
 
Where is innner MyTwo Class ??? –  programmer_1 Feb 24 '12 at 6:25
 
@programmer_1 Scroll down :) –  user656523 Feb 24 '12 at 6:32
 
Oops.. I am sorry.. didn't see that –  programmer_1 Feb 24 '12 at 6:36
 
First, the 'if (dataList.size() > 0)' isn't needed, the loop won't execute when dataList.size() == 0. –  brettw Feb 24 '12 at 7:22
 
Second, and more to the point, just pass myTwo into another method ... process(myTwo) ... or process(dataList, myTwo). There is only one 'myTwo' from what I can see, and the reference held by mainExecution() will contain the updated values when process() returns. –  brettw Feb 24 '12 at 7:25
show 2 more comments

migrated from stackoverflow.com Feb 24 '12 at 13:49

This question came from our site for professional and enthusiast programmers.

3 Answers

up vote 1 down vote accepted

I think it'll be more readable if you have a method void updateMyTwo(MyTwo myTwo) which does the work starting from if (myTwo.b.contains("New Detail")) ... On top of that, you may want to remove the line if (dataList.size() > 0), which is handled by the for loop.

share|improve this answer
 
If my understanding is correct, I presume you suggested that I should create a method void updateMyTwo(MyTwo myTwo) where I apply all of my logic and then call this method within my mainExecution class....but If the updateMyTwo method returns void, my mainExecution method would still be using the initial loaded values itself? Should I be returning MyTwo updateMyTwo(MyTwo myTwo) instead? –  user656523 Feb 24 '12 at 6:47
 
This is the same as my comment on your original question above. You don't need to return a MyTwo ... the MyTwo instance modified by the update method is the same as the one held by mainExecution. When the update method returns, the myTwo object will be updated and visible in mainExecution. –  brettw Feb 24 '12 at 7:28
 
Thanks guys! This works! +1 :D –  user656523 Feb 24 '12 at 14:15
add comment

To avoid confusion, try the approach encapsulation (getter and setter). This is a good practice in java programming.

private class MyTwo{
    private int x;
    private int y;
    private int z;
    private String a;
    private  String b;
    //Constructor
    public MyTwo(int x, int y, int z, String a, String b){
        this.x = x;
        ...
        //equate all
        ...
    }

    public void setX(int x){
        this.x = x;
    }
    public int getX(){
        return this.x;
    }
    //add all getter and setter
}

Then in your mainExecution()

    MyTwo myTwo = new MyTwo(12, 20, 50, "My Test Class", "");

if you want to see the values, use myTwo.getValue

//example 
x = myTwo.getX();

if you want to change its values, use myTwo.setValue

//example
myTwo.setX(x);
share|improve this answer
 
:Thx, but this still doesnt solve my main concern of the mainExecution method being too long and wanting to optimize it. –  user656523 Feb 24 '12 at 6:49
 
Since the value that you manipulate is in the MyTwo class, why not add Methods for operations there. Like addXY() which adds the x and y and return the value. –  Marl Feb 24 '12 at 6:58
add comment

I think the best way to assign a value anywhere in the code and returning the same modified value can be done using a static keyword.You can also use setters and getters to accomplish this. static use can be like this while initisalizing you can do like this

private class MyTwo{
static int x,y,z;
static String a,b;
} 

and the getters and setters like

public setX(int x)
{
MyTwo.x = 12; 
}
public getX()
{
return MyTwo.x;
}

Now these getX()(any value like y/a/b) can be retrieved from anywhere in the code and the modified values are returned.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.