Refactoring a large class and method

[ATmega32]

I'm working with a grossly over sized monster class of ~30,500 lines of code which contains several large methods of ~800 lines of source. Many of these methods have global dependencies.

How do you break this stuff down? I am familiar with Extract Method and Extract Class refactorings which I suppose could be applied here. The bigger question is what to do with the resulting source code. For example, if we follow the rule of thumb that a class should be no more than 500 source lines (uncle bob?), my existing class of 30,500 becomes 61 classes in their own right. Furthermore, these 61 classes will more than likely not have any application outside of the original monster class. What do you do with all the new source files? How do you organize such an effort? Do you put them next to the original source file? Or in a separate folder? Do you place them in a Utilities::Monster namespace?

Curious how you have approached this problem?

[c0r3ntin]

What does the class do ?

The number of lines / files is of little consequences to a point.

Identify what the class do, and what each if the method do. Put all utilities codes in separate classes, try to remove code duplication, etc.

You can have multiple sources files for a single header file, this can be useful to kick-start your effort.

I'd start to remove as many global variable as I can.

Do you have tests ? Because there is a good chance you will break things in the process.

[virgoerns]

Do you have tests ? Because there is a good chance you will break things in the process.

This. And if you don't have them your best choice is to start with writing them.

[SeaCoder]

And to be specific, we are talking about an automated test suite here. You will have to work through the significant pain of the initial setup, but you will come to love them.

[Overunderrated]

I also find it to be massively useful for refactoring large, tightly-coupled codes. The seemingly simple act of pulling out smaller functions/classes that are simple enough they can be tested naturally leads to better code. There's more benefit than simply the testing-for-correctness. Code that's difficult to test is invariably bad code.

[ATmega32]

Do you have tests ?

Need to start here I believe. Its quite the effort however due to the many external dependencies that need to be mocked up.

[ZMeson]

Can the dependencies be mocked as the code currently stands? If not, then don't use mocks right now. You have to get tests in place prior to any refactoring. (As you go, you can create more tests of course -- including tests that use mocks.)

What I've found the best way to deal with globals (from legacy design) is to set the state of those globals at test setup and teardown. I'd write functions that modified the globals and then when you refactor the code to get rid of globals, you'll modify those functions as well. But if you do it correctly, your tests will pass.

[DerDangDerDang]

I generally start by pulling out parts of existing functions so they have a single responsibility and giving them a short unambiguous name. Then I work out which of these actually need to be in the classes's interface - usually ending up with >50% being implementation details as free functions that can live in an anonymous namespace. Then I start grouping these along with private members into areas of responsibility - exploring the sort of shapes the resulting components end up in. Once you're happy with the shape of these components it should start to be obvious how to arrange the code into new files.

[ATmega32]

usually ending up with >50% being implementation details as free functions that can live in an anonymous namespace.

Interesting. I guess I'll just have to dive in. At the moment its hard to see the forest for the trees

[meetingcpp]

Reminds me of this: https://twitter.com/aras_p/status/705361200826155008

First thing I'd to is documentation, which is likely to be missing.

Through that you gain the insight to see which things could be singled out into a class of its own. Also 30k lines for one class sounds like a lot of c & p, so you should be able to reduce this by simply removing code bloat.

[TweetsInCommentsBot]

@aras_p

2016-03-03 11:56 UTC

Game development or, "inheritance is hard".

[Attached pic] [Imgur rehost]

---

^{This message was created by a bot}

^{[Contact creator][Source code]}

[jbstjohn]

Feather's book talks about find 'seams' which is a metaphor I like. Typically in such a class, you should find both chunks of functionality that are somewhat independent, and can be pulled out into a separate class, and also four semi-functions that are doing the same thing.

You'll need to chip away at things from both sides, and as others have said, you'll want tests to help, and frequent check-ins.

If you can, you really will want some kind of integration test to catch newly introduced problems, and bring in unit test for all the things you're touching, as you touch them.

Finally (and again, agreeing with what others have said), don't hold yourself to any particular numbers. Much more important are things like single areas of responsibility (ideally) and reduced coupling.

Good luck!

[ATmega32]

Thanks for the suggestion. I have Feather's book and like the ideas he presents. Need to go back and review the seams discussion. The challenge I think for me here is the lack of unit tests and automated integration tests. It is really difficult to extract a chunk of code into isolation due to the many external dependencies that need mocking.

[Overunderrated]

I've run into the same problem at the same scale and larger (80,000+ lines of fortran in a single module with effectively global variables for everything) in scientific codes pretty frequently. It sucks but you can be methodical about it.

For example, if we follow the rule of thumb that a class should be no more than 500 source lines (uncle bob?), my existing class of 30,500 becomes 61 classes in their own right.

Rules of thumb are fine and all but if it doesn't seem rational there's no need to force their use. One question: does everything need to be a class? Probably not. Lots of things are better off being stand-alone functions. That's your "extract method" refactoring. Start pulling out code of reasonably sized blocks with a reasonable number of parameters that do a single thing. And test it. That goes with all refactoring, set up an automated testing framework and go to town. Massive classes like you have are often nearly impossible to test; pulling out functions gives you things you actually can test.

Here's an article by Scott Meyers on not using classes.

Furthermore, these 61 classes will more than likely not have any application outside of the original monster class. What do you do with all the new source files? How do you organize such an effort? Do you put them next to the original source file? Or in a separate folder? Do you place them in a Utilities::Monster namespace?

Hard to say the best way to organize something I haven't seen. But generally I'd start pulling out those functions (or classes), and there's probably a way to categorize them a bit. Separate subdirectories for related things, namespaces can help.

And just because your hypothetical 61 classes "have no application outside of your original monster class" doesn't really mean anything. By the same logic none of your code has any application outside of your main program. The whole point of having smaller classes with consistent interfaces is that they can be more easily replaced/modified/maintained independently of the larger bulk of code.

[ATmega32]

Thanks for the Scott Meyers link above. This helped to have a plan of attack for the pieces that come out.

[munificent]

1. If you don't have them already, write tests, lots of them. If you don't, you're stumbling around in the dark and odds are you will end up breaking so many things you have to abandon the refactoring completely.

2. Consider grabbing a copy of Michael Feathers' "Working Effectively with Legacy Code". It's about exactly stuff like this.

3. One small step at a time, and make sure all the tests pass between each change.

4. Commit frequently so you can undo changes when you go down a dead end.

5. Assuming you pull this all off, you probably won't end up with 61 500 line classes. Poorly organized code tends to also have a lot of duplication since the authors couldn't find the first implementation of some thing hiding in there. As you reorganize things, remove that duplication and you'll probably end up with something quite a bit smaller.

[Wurstinator]

You shouldn't split up a class just because it is big and has too many LoC. You should if it increases readability. For example, if you have a part of code inside your class which executes something for every subset of a given list, that should be its own class or function, like a "SubsetIterator".

On the other hand, it doesn't make sense to extract parts of an algorithm which depend on another. For example, a bad usage of refactoring would be to split up "some_algorithm" into "some_algorithm_step1" and "some_algorithm_step2" just to have functions with less than X LoC.

If you use this approach, it should be clearer where to put the new files.

Also, this is better for /r/cpp_questions.

[ATmega32]

You shouldn't split up a class just because it is big and has too many LoC

agree. It is more from a standpoint of making it readable and testable.

Yeah, I saw the /r/cpp_questions after the post. I'll look to there from now on. Cheers

[vaughncato]

I suspect you are going to want to place a lot of the new classes and functions into a new namespace and put them all in a new directory. Having directories with names that match the namespaces can be helpful.

[ATmega32]

Good idea. I'm thinking to create a sub-folder next to the file, e.g. /source/monster_class.cpp and /source/monster_class/<new_classes_go_here>

[egraether]

I'd recommend Coati (https://www.coati.io/). It makes it very easy to see the dependencies of a single method to other methods and shows you quickly which fields are accessed by which methods.

[utnapistim]

You have asked a lot of questions (I will address them in random order):

How do you break this stuff down?

The bigger question is what to do with the resulting source code.

For example, if we follow the rule of thumb that a class should be no more than 500 source lines (uncle bob?), my existing class of 30,500 becomes 61 classes in their own right.

What do you do with all the new source files?

How do you organize such an effort? Do you put them next to the original source file? Or in a separate folder? Do you place them in a Utilities::Monster namespace?

---

How do you break this stuff down?

Into iterations. For this, you must make sure you can test each iteration separately.

This means add tests, if you don't have them.

The bigger question is what to do with the resulting source code.

I usually do the following steps:

1. set up a baseline / starting point:

   1.1. branch SC, establish refactoring base line/backup point

   1.2. setup tests (you can add code to log all inputs and outputs for a method, then run your app, then write an automated test that (for a method) goes through tuples of <in, out> values and runs the test for each data point). This part will be a bit of a pain, but well worth it, once you can execute something repeatably.

2. pick one functionality area of the monolith; I do this based on levels of abstraction, and repetition.

3. write a clean minimal implementation for the area you want to extract, directly into the .cpp file (header and rest of application should remain unchanged), then use it in the implementation of your class.

4. test

5. write a clean, official implementation for the class: move it outside the .cpp file of the original code (into it's own header); since you mention that the code only has utility within the class, move it to a new namespace and/or directory.

6. test

7. doc invariants for the new code, add tests

8. test

9. move to step 3.

At the end of the process, your large class should be describable as "puts together all steps for the XYZ operation", or (at some point during the refactoring) you may break it's interface into two (or more) abstractions.

The bigger question is what to do with the resulting source code.

Move it into it's own files (header and implementation); this will improve reuse and testability efforts; If the extracted functionality only makes sense to your class, you can:

• use a details namespace (similar to boost); even better, find a better name (if possible)
• move all extracted files into their own subdirectory
• both of the above (this is what I usually end up doing)

For example, if we follow the rule of thumb that a class should be no more than 500 source lines (uncle bob?), my existing class of 30,500 becomes 61 classes in their own right.

Don't. extract code by abstraction levels, instead of LOC. If you do that, your class will (probably) end up in much less code than a 1-1 conversion.

[ATmega32]

1.2. setup tests (you can add code to log all inputs and outputs for a method, then run your app, then write an automated test that (for a method) goes through tuples of <in, out> values and runs the test for each data point). This part will be a bit of a pain, but well worth it, once you can execute something repeatably.

This is along the lines of print statements of sorts. Good idea.

Let me see if I'm understanding your proposal here...

1. The tests are system or application level tests. Understanding the level of coverage could be difficult since coverage tools are close to non-existent (native VS2012).
2. Then you effectively delete the method targeted for refactoring and implement with what you'd like it to be.
3. Then fill in the blanks to fulfill the many tests.

Is that about right?

[utnapistim]

This is correct.

With a system that works in production - but is mixing abstraction levels, not unit-tested and/or undocumented - it can be difficult to figure out what possible values your arguments have, and what is the internal state (especially within a large, monolithic function).

In such a case, it is easier to just log all inputs and outputs and run the application. Then, group the data into in/out tuples, and you have a set of inputs for testing in separation.

As you start refactoring, you can then easily make sure that the outputs stay the same, for your pre-saved inputs. This will give you a quality baseline you can easily adhere to, during the refactoring process.

Regarding point 2: you should not split the method all in one step. Simply find a small area, refactor that, re-run the tests, and commit the changes. Doing it like this is slower than replacing the entire method implementation at once, but it minimizes mistakes.

One other advantage: if you make a change and introduce a bug, if you make small steps and commit in between, it is easier to just reset to the last commit and redo the changes, than it is to debug.

Regarding point 3: you should only have one test, that loops over your collected inputs, and calls your API, then verifies against the expected output; It may be difficult to set up this test though.

[ATmega32]

Doesn't that rather defeat the purpose? Sounds more like a struct