Refactor to fix architectural error

Question

In a sort-of-working application I see this monstrous code:

class SomeUglyClass extends Thread {
    ArrayList<SomeData> someDataStructure = new ArrayList<SomeData>();
    Handler mHandler = new Handler() {
        // a lot
        // writing to someDataStructure
    }
    public void run() {
        int some_count, ...;
        while(true) {
            // a lot
            // r/w access to someDataStructure

            try {
                Thread.sleep(1, 0);
            } catch (Exception e) {
                break;
            }               
        }
    } // end of run(), total 500 lines of code
} // end of SomeUglyClass, total 4K lines of code

Maybe you already see the problems with this code. If not, here they are:

1. mHandler is attached to the UI thread (because it is created by the thread that loads the class, which is the main thread)

2. there's no looper (which is fact is the bug)

3. the thread wastes CPU time and drains the battery

4. someDataStructure is not thread-safe, but synchronizing elementary access operations will not help; synchronizing large blocks of code in a endless loop will likely block the guarded resource and make it unavailable for other threads; finally, it is not only someDataStructure, the whole class is based on the assumption that only one thread can run its code.

5. I cannot just add the looper, because the endless loop in run() has to be run, while Looper.loop(); also is an infinite loop. One thread cannot run two infinite loops.

Despite this epic architectural fail, the code is really doing something, it cannot be re-written at once, it is 4K lines of code, and often I can only guess what the code really does.

I need to refactor it. It should be a sequence of small steps preserving the functionality.

How do I refactor this terrific code?

Answer 1

You should try separation of concerns: try first to divide the whole class into many smallest one, each one responsible for doing/dealing with exactly one thing.

You may have something for data Access (read/write data), service (isolated business logic), and the UI. You may use event bus to decouple between objects (consider otto) and may be dependency injection (consider Dagger).

This process of separation will help you understand what each piece of code is doing and also the dependencies between the different parts, thus making writing unit/integration tests much easier.

Answer 2

Add lots of tests, use version control and then work as slowly as you need to.

Answer 3

The 1st step has been to change:

    public void run() {
        int some_count, ...;
        while(true) {
            // a lot
            // r/w access to someDataStructure

            try {
                Thread.sleep(1, 0);
            } catch (Exception e) {
                break;
            }               
        }
    }

to:

    @Override
    public void run() {
        Looper.prepare();
        mHandler = new MyHandler();
        mHandler.post(run_step);
        Looper.loop();
    }

    Runnable run_step = new Runnable() {
        int some_count, ...;

        @Override
        public void run()
        {
            //while(true) {
                // a lot
                // r/w access to someDataStructure

                mIntoThreadHandler.postDelayed(this, 1);
            //}
        }
    }

This preserves the functionality but still wastes CPU time. The urgent bug has been fixed, and the issue has been closed; I could not sell "must refactor to kill monstrous code" to my management, but I could sell "this can work faster if I refactor," so a new separate issue has been opened. UGH!

PS no chance to sell "lots of tests".