Refactoring methods in existing code base with huge number of parameters

Question

I have inherited an existing code base where the "features" are as follows:

• huge monolithic classes with (literally) 100's of member variables and methods that go one for pages (er. screens)
• public and private methods with a large number of arguments.

I am trying to clean up and refactor the code, to leave it a little better than how I found it. So my questions

• is worth it (or do you) refactor methods with 10 or so arguments so that they are more readable ?
• are there best practices on how long methods should be ? How long do you usually keep them?
• are monolithic classes bad ?

Answer 1

is worth it (or do you) refactor methods with 10 or so arguments so that they are more readable ?

Yes, it is worth it. It is typically more important to refactor methods that are not "reasonable" than ones that already are nice, short, and have a small argument list.

Typically, if you have many arguments, it's because a method does too much - most likely, it should be a class of it's own, not a method.

That being said, in those cases when many parameters are required, it's best to encapsulate the parameters into a single class (ie: SpecificAlgorithmOptions), and pass one instance of that class. This way, you can provide clean defaults, and its very obvious which methods are essential vs. optional (based on what is required to construct the options class).

are there best practices on how long methods should be ? How long do you usually keep them?

A method should be as short as possible. It should have one purpose, and be used for one task, whenver possible. If it's possible to split it into separate methods, where each as a real, qualitative "task", then do so when refactoring.

are monolithic classes bad ?

Yes.

Answer 2

if the code is working and there is no need to touch it, i wouldn't refactor. i only refactor very problematic cases if i anyway have to touch them (either for extending them for functionality or bug-fixing). I favor the pragmatic way: Only (in 95%) touch, what you change.

Some first thoughts on your specific problem (though in detail it is difficult without knowing the code):

• start to group instance variables, these groups will then be target to do 'extract class'
• when having grouped these variables you hopefully can group some methods, which also be moved when doing 'extract class'
• often there are many methods which aren't using any fields. make them static (they most likely are helper methods, which can be extracted to helper-classes.
• in case non-related instance fields are mixed in many methods, do loads of 'extract method'
• use automatic refactoring tools as much as possible, because you most likely have no tests in place and automation is more safe.

Regarding your other concrete questions.

is worth it (or do you) refactor methods with 10 or so arguments so that they are more readable?

definetely. 10 parameters are too many to grasp for us humans. most likely the method is doing too much.

are there best practices on how long methods should be ? How long do you usually keep them?

it depends... on preferences. i stated some things on this thread (though the question was PHP). still i would apply these numbers/metrics to any language.

are monolithic classes bad ?

it depends, what you mean with monolithic. if you mean many instance variables, endless methods, a lot of if/else complexity, yes.

also have a look at a real gem (to me a must have for every developer): working effectively with legacy code

Answer 3

Assuming the code is functioning I would suggest you think about these questions first:

• is the code well documented?
• do you understand the code?
• how often are new features being added?
• how often are bugs reported and fixed?
• how difficult is it to modify and fix the code?
• what is the expected life of the code?
• how many versions of the compiler are you behind (if at all)?
• is the OS it runs on expected to change during its lifetime?

If the system will be replaced in five years, is documented well, will undergo few changes, and bugs are easy to fix - leave it alone regardless of the size of the classes and the number of parameters. If you are determined to refactor make a list of your refactoring proposals in the order of maximum benefit with minimum changes and attack it incrementally.