Splitting big classes with Inner classes in Java

Question

I am working on an Android project. I have searched high and low but I can't figure out a good strategy to split and package my code.

My problem is that I have Inner Classes that use the main class variables, and I can't figure out how to decouple them.

I have tried to create helper classes, but then either I pass a lot of variables through constructors, or I expose my main class, and I don't want to do either.

I want to keep max lines of code per class to 150. Currently, it's 278. I am looking for ideas to decouple these, specifically, how to restructure classes to preserve abstraction (private variables). What are the Java best practices for this?

As an example, here's one of my main classes, MainActivity, ~300 lines.

Answer 1

Edit:

Following the addition of the actual code for MainActivivty, I'd suggest the following:

1. Follow MVC/MVP architectural pattern. You can find a link to a template that I wrote at the end, but there are many more templates - just choose one that you like. Once you understand how to get all UI related code outside of MainActivity, the method addButtons() will be gone, as well as CategoriesListener class.
2. There is really no need for AllPostsFetchAsyncTask to be an inner class. Implement it as a regular class outside of activity. In order to pass the data from this class back to MainActivity, just define a listener interface that your MainActivity will implement, and pass MainActivity.this to the constructor - when this task is finished, it will call a callback method on the MainActivity, which, in turn, will handle data binding to Adapter. In fact, you adopt here a very bad practice - by making AllPostsFetchAsyncTask aware of implementation details of MainActivity you create unnecessary coupling between the two, thus violating Encapsulation, Single Responsibility and Open Closed principles of OOP.

Just by implementing the two steps above you will make this particular MainActivity way shorter than 150 lines of code.

Said that, your intent of keeping the activities 150 lines long is too restrictive. This boils down to the fact that if your Activity or Fragment are not trivial, then once you implement onCreate(), onPause(), onResume(), onPrepareOptionsMenu(), onBackStackChanged() and other standard lifecycle methods, then you'll probably have more than 150 lines of code even before you add the logic of your custom controller.

Now, I totally hate inner classes and am trying to avoid them at all costs. The following checklist can serve as a guideline, but it is not complete by any means:

• Never manipulate UI elements in controllers/presenters (Activities, Fragments and Adapters) - encapsulate these manipulations in separate classes. These classes are MVC/MVP views (as opposed to Android View) and I put them in views or mvcviews packages. My Activities and Fragments tend to have zero findViewById() calls in their source code.
• Put all Adapters in a separate package (even if they 30 lines long). I call this package controllers.adapters or controllers.listadapters
• If you ever need to pass a set of related data in your app - define a POJO (also known as Value Object) and use it to encapsulate this data. I usually have a package named pojos, even if it contains just a single class.

• Define abstract classes AbstractActivity and AbstractFragment and put there any convenience logic which is used by your controllers. For example: I always have the following method (or similar) in my AbstractActivity and AbstractFragment:

  public void replaceFragment(Class <? extends Fragment> claz, boolean addToBackStack, Bundle args) { 
      // Code to replace the currently shown fragment with another one 
  }
  

• Check to see if there are any third party libraries which might be useful in the context of your app and use them.

My packaging usually follows this pattern:

I know you wrote that you've already seen some discussions on MVC, but I still encourage you to try the implementation I propose in this template/tutorial project: https://github.com/techyourchance/android_mvc_template

Hope this helps

Answer 2

First of all, based on the implementation of your Activity, you have missed a few important things regarding Activities.

1. Only use static inner classes or stand alone classes for AsyncTasks: see Background task, progress dialog, orientation change - is there any 100% working solution?

Important is this:

Step #2: Have the AsyncTask hold onto the Activity via a data member, set via the constructor and a setter.

Step #5: In onCreate(), if getLastNonConfigurationInstance() is not null, cast it to your AsyncTask class and call your setter to associate your new activity with the task.

You'll notice that you will have to register and deregister your components based on the life cycle methods of Android. This is important to know, always follow the Android life cycle!

Remembering this will always lead you to the right answers regarding decoupling the Android way.

2. Use data holding classes, when needed.

This here doesn't really belong inside an Activity:

// Stores the fetched dataMap
ArrayList<HashMap<String, String>> arrayList;

When your Activity gets destroyed, e.g. during a configuration change, all your data is gone and you need to load everything again.

Accessing and storing your data can be done in many different ways: http://developer.android.com/guide/faq/framework.html#3

In your case this could be applicable:

• A public static field/method

  An alternate way to make data accessible across Activities/Services is to use public static fields and/or methods. You can access these static fields from any other class in your application. To share an object, the activity which creates your object sets a static field to point to this object and any other activity that wants to use this object just accesses this static field.

Also think about storing your data insida a DB or by other means, so even after your App gets destroyed, your data isn't gone.

3. Communication with your Activity can be done like this: http://developer.android.com/guide/components/fragments.html#CommunicatingWithActivity

Use it for your Views and View Listeners in the same way. Have a component managing your Views (like a Fragment does), register it to your Activity, use it, deregister it when not needed or when the life cycle calls for it.

Like said in 1., the Android life cycle is the key to everything.

4. Dependency Injection is a very important topic and you can either use a framework for it (like Dagger 2 or RoboGuice) or do it your own way. Make sure your Injector knows the dependencies (like which Buttons need which ClickListeners and Information or which data your Adapter needs) and bind them together. When always considering the life cycle, you will see which interfaces and which methods you need and when to invoke them.

5. Don't worry about the amount of lines of code. If your design is consistent and makes sense, you won't be having readability problems even with 500 lines. Btw. when properly documenting your code, it gets easily above 150 lines of codes. So, again to worry about that.

If you have any specific questions about implementation details, ask a specific question or else you get a bloated answer.

Answer 3

This is an answer to part of problem. As stated in the question

I have tried to create helper classes, but then either I pass a lot of variables through constructors

This is something very similar to Telescoping constructor. So, to solve this problem i would personally use something similar to Builder Pattern.

class A {
  public class B {
     public B(int x, int y, int z, int m, int n, int o){

     }
  }
}

Above case can be modified like below.

class A {
   public class B{
     int a, int b, int c, int m, int n, int p = 0;
     public B(){
     }
     public B setA(int x){
       a = x;
       return this;
     }     
     public B setB(int x){
       b = x;
       return this;
     }
     ... and similar methods for other properties.     
   }
}

Above solution may make your class look lengthy when you have many properties and your class-client need to remember more methods. So for that i would like to make slight modification in above pattern. Assigning key to every property will make things simpler for class-client as well.

class A {
   public class B{
     int a, int b, int c, int m, int n, int p = 0; // key for int a == "a" and for b is "b" and so on... this is our assumption.
     public B(){
     }
     public B setProperty(String key, int value){
       if(key.equals("a")){
           a = value;
       }else if(key.equals("b")){
           b = value;
       } ... and so on for other properties.
       return this;
     }     

   }
}

Answer 4

If the inner classes are only accessing fields, then introduce a new Container class of all relevant fields of your MainActivity class (you can of course also make two or three tiny containers instead of a single big one).

Your example can then be modified into this:

/** new container class */
class FooBar {
    public Foo foo;
    public Bar bar;
}

/** nice, easy and SHORT! */
class MainActivity {

    private FooBar fooBar;

    public MainActivity() {
        new Ping(fooBar);
        new Pong(fooBar).someMethod();
    }
}

/** successfully converted from inner class to class */
class Ping {

    public Ping(FooBar fooBar) {
        fooBar.foo = new Foo(); // Ping modifies Foo
    }
}

/** successfully converted from inner class to class */
class Pong {

    private Bob bob;
    private FooBar fooBar;

    public Pong (FooBar fooBar) {
        this.fooBar = fooBar;
        fooBar.bar = new Bar(); // Pong modifies bar
        bob = new Bob();
    }

    public void someMethod () {
        fooBar.bar.setSomethingTo(Bob.getSomething()); // Pong modifies bar of Main class
        fooBar.foo = new Foo(fooBar.bar); // Pong assignes something to bar
    }
}

I used These class stubs to get the code compiling:

class Foo {
    public Foo() {}
    public Foo(Bar bar) {}
}
class Bar {
    public void setSomethingTo(String something) {}
}
class Bob {
    static String getSomething() {return "Something";}
}

If the inner classes are also accessing methods, then you could specifiy those in an Interface, that is implemented by MainActivity. Hand the instance of MainActivity to the other classes, by using only the Interface. This way you don't have to expose your complete MainActivity and you can avoid circular dependencies.

Answer 5

Take your inner classes out,pass the instance of MainActivity itself to them in their constructors.

MainActivity mainActivity;
DownloadJSON(MainActivity mainActivity) {
        super();
        mProgressDialog = new ProgressDialog(MainActivity.this);
        mProgressDialog.setCancelable(false);
        this.mainActivity=mainActivity;
    }

Make the variables in mainActivity public,and you can access to them like this:

          // Extract the metadata
         mainActivity.pageCount =Integer.parseInt(metaData.get("PAGE_COUNT"));