Methods Vs Code Fragments

Complex code (especially UndocumentedTrickyCode) should be abstracted into methods/functions with MeaningfulNames.

See also ExtractMethod.

Rationale:

With a well-named function or method, you have a descriptive term, as opposed to just breaking the code into fragments. For fragments, you can use whitespace (WhitespaceIsGood) and blocks (BracesAreGood), but it's not the same. With a function or method, the caller function or method becomes simpler by having fewer statements.

You may also be able to reuse the method/function if you make it abstract enough. A bonus!(see NarrowTheInterface)

Arguments:

"Doesn't having too many functions/methods make the system more complex? I have to remember all these things!"

You don't have to remember them, especially if they're private methods or static functions or in an AnonymousNamespace. All you have to do is keep all the helper methods together underneath the main one (EmphasizeImportantInformation). That way they are all logically grouped together.

And it's not like each module is going to suddenly have 100 functions/methods. You're not likely to make every group of three or more statements another function. Just the ones that are hard to understand.

"How do you know how to break up a function?"

The same way you know how to break it into blocks (see BracesAreGood).

"It's slower to call all these functions."

It is unless they are inlined. If they aren't inlined, unless they are in inner loops, the performance hit is not likely to cause you grief. OptimizeLater!

"It takes fewer keystrokes to just leave the block where it is and comment it."

Has typing ever been the bottleneck when coding? Of course you don't want to distract yourself by trying to refactor while making a test pass. Simple solution: Get the test to pass, then ExtractMethod.

Exceptions:

Don't go overboard. If the function/method is clear already, there's no reason to touch it unless you can extract some useful abstract functionality from a piece of it.

Examples:

 double CEmployee::GetCostPerHour() const
 {
     // Calculate office supply usage
     double dOfficeSuppliesCost = 0;
     for( m_Supplies::Iterator i(m_Supplies); i != m_Supplies.end(); i++ )
          dOfficeSuppliesCost += i->GetCost();

     // Calculate hours employed.
     double dHours = 0;
     for( m_Timesheet::Iterator i(m_Timesheet); i != m_Timesheet.end(); i++ )
          dHours += i.GetHours();

     double dOfficeSuppliesCostPerHour = dOfficeSuppliesCost / dHours;

     ...

     return dOfficeSuppliesCostPerHour + dDrinksPerHour + dUtilitiesPerHour + GetWage?();
 }

It's best to change this to

 double CEmployee::GetCostPerHour() const
 {
     double dHours = CalculateHours();
     double dOfficeSuppliesCostPerHour = CalculateOfficeSuppliesCost() / dHours;

     ....

     return dOfficeSuppliesCostPerHour + dDrinksPerHour + dUtilitiesPerHour + GetWage();
 }

 double CEmployee::CalculateHours() const
 {
     double dHours = 0;
     for( m_Timesheet::Iterator i(m_Timesheet); i != m_Timesheet.end(); i++ )
          dHours += i.GetHours();

     return dHours;
 }

 double CEmployee::CalculateOfficeSuppliesCost() const
 {
     double dOfficeSuppliesCost = 0;
     for( m_Supplies::Iterator i(m_Supplies); i != m_Supplies.end(); i++ )
          dOfficeSuppliesCost += i->GetCost();

     return dOfficeSuppliesCost;
 }

possibly making the two helper methods private. [Although they are abstract enough that maybe protected or public is better; then this class and its subclasses or clients can reuse them. NarrowTheInterface.

Make them private. It's easy to make them protected or public later; in the mean time, you've got a public interface nobody is using. YAGNI

Make them public. Don't complicate your code (and your thinking!) with various visibility tags. DTSTTCPW.

Make them protected. I don't want to think about helper methods when I, the client, just want the object's interface and I don't want to prove that no one will ever need this method ever again.

Per the book LargeScaleCppSoftwareDesign by John Lakos:

Make them static functions at file-scope in the implementation file of the class.

This is a broken approach. Subclasses won't have access to them. You lose one of the great benefits of methods vs code fragments: componentization of the method. If a subclass wishes to override the original method, it can easily rebuild the method's behaviour. Also, a subclass can also override the components. This is almost flexibility ForFree. Module scoping is almost a throwback from C and should only be used when you really mean to semantically clamp down a symbol's scope to the file it's written in. -- SunirShah

Everyone says that about the parts of /Large Scale/ that bust compile time or assist porting. Think of it as a migration (a Refactor path) from in-line code, to a static HelperFunction, to a private method, to a virtual one. At each point you get a higher cost, but more benefits.

/Large Scale/ is after only one benefit - recompile time after the signature to a helper changes.

I would contend that LargeScaleEqualsFailure. If your project has reached the crossover point where higher compile time reduces quality significantly (compare to maintenance time, refactor time, comprehension time), your project is toast. It has too much cruft, it isn't modular enough, it is too big. The largest project I worked on was 350 000 lines of code and only got that large due to failure. Maybe I'll fill in that WikiPage when thoughts coalesce. (Or maybe somebody else?) -- SunirShah

Consider dynamic languages if compile times grow too large.