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.

Omidyar

if you don't want to pass "tons of" variables through constructors (good design principle is to pass not more than 5 variables to methods/constructors), just encapsulate all those needed variables in an own class (see Value-Object or Transfer-Object design pattern). Then you don't have to expose any variables in your MainActivity and only have to pass one value object to the corresponding constructors or methods of your refactored inner classes. – Monometallism

Both are almost similar and I will probably pass 4 variables. However, the new class would also need to modify variables in the main class, and that requires exposure, or more code just to check this. Ideally, I am looking for a C++ type friend or something similar so I can only pass pass the class object MainActivity.this and be done. – Omidyar

You're thinking, "how can I structurally split this up?" Try thinking instead, "how can I conceptually split this up?" Huge value-type classes with tons of utility methods (e.g., String) are perfectly okay. Other huge classes probably have too many responsibilities. Each class should do one thing to one other thing. – Estivate

@slartidan Thanks. Actually it's new Bob().getSomething(). I was writing a simple example so that people don't have to read so many lines to get the gist. Fixed! – Omidyar

@KevinKrumwiede The problem is that conceptually, MainActivity is abstracting exactly what it's supposed to abstract, and as far as I can tell sticks to OOP. I believe that conceptually it wouldn't make sense to split these. I am simply looking for an implementation that allow me to implement this in a structure that maintains these relations but to keep code manageable. – Omidyar

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

Nonferrous

Agreed about lines of code. Java is notorious for requiring a lot of boilerplate, and Android is even worse in that regard. I'm happy when an activity comes in under 300 lines. (Of course, about half my lines are usually Javadoc comments.) – Estivate

@Nonferrous I added my actual MainActivity for reference. I am already keeping Adapters outside. But AsyncTasks, onClickListeners, each requires access to MainActivity, and hence I wrote them as inner classes. @slartidan That is one way, but it seems more like a hack. I can try that, but I am hoping for a more elegant solution. – Omidyar

@prakharsingh95, based on the code for MainActivity I added two specific approaches that you can use. See the edited answer – Nonferrous

Thanks! I have added my new MainActivity in case anyone would be interested. – Omidyar

IMHO it's often better to package by feature than layer. As stated here for example medium.com/mindorks/… – Rajkot

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.

Kappa

I can't really agree on #5: "Don't worry about the amount of lines of code". While 500 lines of code per class (including javadoc) is fine for Android in particular cases, one should always try to minimize the amount of code. In some cases, if I have a choice between using a single line API call vs. implementing it myself in 20 lines (more efficiently), I choose the shorter option for readability of code. – Nonferrous

In addition: no one starts programming with knowledge of all the concepts, therefore there should be some simple metrics that indicate "you're probably doing it wrong, man". Number of lines is one of these metrics, and it is too easily discarded by experienced developers. I can tell this after reading some portions of AOSP - it could have been made much more readable in many cases... – Nonferrous

Well you certainly have a point, that with more experience comes better understanding of concepts and structure. But there really are huge differences depending on the platforms you're developing for; especially with limited resources like on mobile or embedded systems. Performance is a really important subject, e.g. in the context of Android you absolutelly need to make sure to complete each computation well under 16ms to maintain smooth 60 frames. If you don't, it will become choppier and the user uninstalls your app. That's why you sometimes can't write the most elegant code.... – Kappa

...You can't allocate too much memory for your app, can't instantiate many objects, have ugly static fields, etc. Nevertheless there are still many ways to decouple your componemts for more readability, but in the case of Android I wouldn't set the bar as low as 150, but I guess it depends on many factors like time, complexity, experience, etc. – Kappa

Well, the only performance critical apps I'm aware of are games. All other "lags" are usually caused by running a code which should be run in background on the UI thread. In any case, it is completely fine to write a lengthy code, as long as you are sure that there is no better option. However, this should be a calculated decision, based on understanding that lengthy files are evil ) – Nonferrous

Well there is a huge diversity of apps like graphically demanding stuff like games or visualization, simulation (CPU heavy), sensory dependent (data resource heavy), realtime network communication. And each of those need very different strategies. Some even need more monolithic approaches. – Kappa

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;
     }     

   }
}

Nonconformity

This is what I had approached initially as well. The issue is that the inner classes are coupled. I not only need to pass variables, I need to be able to return variables as well. If I move inner classes out, package level privilege is not sufficient for modifying private variables. – Omidyar

can you point to some instances in your code that may cause problem if i blindly move 'CategoriesListener' and 'AllPostsFetchAsyncTask' class outside 'MainActivity' class. Would moving both classes out will be solution for you? – Nonconformity

These two classes need access to MainActivity's context, current, pageCount, adapter, etc. The are accessed and modified by these classes. I would also like to keep Listeners, AsyncTasks in separate packages, so I need to work with a private privilege level. Thanks for going through the whole code, btw! – Omidyar

1. For resolving these dependencies you need to inject the context to the AsyncTask/Listener. Either pass it in constructor or plain method. 2. Other dependencies (pageCount, current) can be resolved by making a getters and setters for each property. 3. As far as adapter are concerned, if you have access to view (as in onClick), you can navigate in view heirarchy and reach to Listview and then call getAdapter() to get the attached adapter. Or, you can opt approach#2 as well. – Nonconformity

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.

Benison

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"));

Cow

he doesn't want to expose variables in MainActivity to other classes (which is done by declaring them public), so this is not the desired solution – Monometallism

This means I would need to declare pageCount as public or use a hacked friend implementation. It's very important to in android to secure Activities. – Omidyar

Oh,i see. If you don't want to declare them as public,I think what @Monometallism said in his comment is a good way. – Cow

I think you can introduce some getters and setetrs methods in your mainactivty and take your inner classed out and pass the instance of mainActivity to the inne class constrcutor and usng that instance you can call getters method. – Sainfoin

@Raj Sharma Getters and setters would not limit exposure, especially when working with objects. – Omidyar

sorry I was thinking he just want to use mainActivity varibales without make them public... – Sainfoin

This is not the desired solution. – Hakodate