Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm currently working on a set of reports that have many different sections (all requiring different formatting), and I'm trying to figure out the best way to structure my code. Similar reports we've done in the past end up having very large (200+ line) functions that do all of the data manipulation and formatting for the report, such that the workflow looks something like this:

DataTable reportTable = new DataTable();
void RunReport()
{
    reportTable = DataClass.getReportData();
    largeReportProcessingFunction();
    outputReportToUser();
}

I would like to be able to break these large functions up into smaller chunks, but I'm afraid that I'll just end up having dozens of non-reusable functions, and a similar "do everything here" function whose only job is to call all these smaller functions, like so:

void largeReportProcessingFunction()
{
    processSection1HeaderData();
    calculateSection1HeaderAverages();
    formatSection1HeaderDisplay();
    processSection1SummaryTableData();
    calculateSection1SummaryTableTotalRow();
    formatSection1SummaryTableDisplay();
    processSection1FooterData();
    getSection1FooterSummaryTotals();
    formatSection1FooterDisplay();

    processSection2HeaderData();
    calculateSection1HeaderAverages();
    formatSection1HeaderDisplay();
    calculateSection1HeaderAverages();
    ...
}

Or, if we go one step further:

void largeReportProcessingFunction()
{
    callAllSection1Functions();

    callAllSection2Functions();

    callAllSection3Functions();
    ...        
}

Is this really a better solution? From an organizational point of view I suppose it is (i.e. everything is much more organized than it might otherwise be), but as far as code readability I'm not sure (potentially large chains of functions that only call other functions).

Thoughts?

share|improve this question
    
At the end of the day...is it more readable? Yes, so it is a better solution. –  sylvanaar Mar 25 '12 at 0:05
add comment

8 Answers 8

up vote 15 down vote accepted

This is commonly referred to as functional decomposition and is generally a good thing to do, if done correctly.

Also, the implementation of a function should be within a single level of abstraction. If you take largeReportProcessingFunction, then its role is to define which different processing steps are to be taken in which order. The implementation of each of those steps is on an abstraction layer below and largeReportProcessingFunction shouldn't depend on it directly.

Please note that this however is a bad choice of naming:

void largeReportProcessingFunction() {
    callAllSection1Functions();
    callAllSection2Functions();
    callAllSection3Functions();
    ...        
}

You see callAllSection1Functions is a name that doesn't actually provide an abstraction, because it doesn't actually say what it does, but rather how it does it. It should be called processSection1 instead or whatever is actually meaningful in the context.

share|improve this answer
5  
I agree in principal with this answer, except I can't see how "processSection1" is a meaningful name. It's practically equivalent to "callAllSection1Functions". –  Eric King Mar 23 '12 at 21:28
2  
@EricKing: callAllSection1Functions leaks details about the implementation. From the outside perspective, it doesn't matter whether processSection1 defers its work to "all section1 functions" or whether it does it directly or whether it uses pixie-dust to summon a unicorn that will do the actual work. All that really matter is, that after a call to processSection1, section1 can be considered processed. I agree that processing is a generic name, but if you have high cohesion (which you should always strive for) then your context is usually narrow enough to disambiguate it. –  back2dos Mar 24 '12 at 11:55
1  
All I meant was, methods called "processXXX" are almost always terrible, useless names. All methods 'process' things, so naming it 'processXXX' is about as useful as naming it 'doSomethingWithXXX' or 'manipulateXXX' and similar. There's almost always a better name for methods named 'processXXX'. From a practical point of view, it's as useful (useless) as 'callAllSection1Functions'. –  Eric King Mar 24 '12 at 16:21
add comment

It depends. If all the functions you are creating are truly a single unit of work then it's fine, if they are just lines 1-15, 16-30, 31-45 of their calling function that is bad. Long functions are not evil, if they do one thing, if you have 20 filters for your report having a validate function that checks all twenty is going to be long. That's just fine, breaking it up by filter or groups of filters is just adding extra complexity for no real benefit.

Having a lot of private functions also makes automated unit testing more difficult, so some will try to avoid it for that reason, but this is rather poor reasoning in my opinion as it tends to create a larger more complex design to accommodate testing.

share|improve this answer
2  
In my experience, long functions are evil. –  Bernard Mar 23 '12 at 20:16
    
If your example was in OO language, I would create a base filter class and each of the 20 filters would be in different deriving classes. Switch statement would be replaced by a creation call to get the right filter. Each function does one thing and all are small. –  DXM Mar 24 '12 at 15:34
add comment

Since you are using C#, try and think more in objects. It looks like you are still coding procedurally by having "global" class variables which subsequent functions depend on.

Breaking out your logic in separate functions is definitely better than having all the logic in a single function. I would bet you could go further by also separating the data the functions do work on by breaking this out in several objects.

Consider having a ReportBuilder class where you can pass in the report data. If you can break ReportBuilder into separate classes do this as well.

share|improve this answer
2  
Procedures serve a perfectly good function. –  DeadMG Mar 23 '12 at 20:53
add comment

You said you're using C#, so I wonder if you could build a structured class hierarchy taking advantage of the fact that you're using an object-oriented language? For example, if it makes sense to break up the report into sections, have a Section class and extend it for client specific requirements.

It may make sense to think about it as if you were creating a non-interactive GUI. Think about the objects you might create as if they were different GUI components. Ask yourself what a basic report would look like? How about a complex one? Where should the extension points be?

share|improve this answer
add comment

Yes.

If you have a code segments that needs to be used in multiple places own function. Otherwise if you have to make a change to the functionality you will need to make the change in multiple places. This not only increase work but increases the risk of bugs showing up as the code gets changed in one place but not another or where a but is introduced in one place but not the other.

It also helps make the method more readable if descriptive method names are used.

share|improve this answer
add comment

The fact that you use numbering to distinguish functions suggests that there are underlying problems with duplication or a class doing too much. Breaking up a large function into many smaller functions can be a useful step in refactoring out duplication, but it should not be the last one. For instance, you create the two functions:

processSection1HeaderData();
processSection2HeaderData();

Aren't there any similarities in the code used to process section headers? Can you extract a common function for both of them by parameterizing the differences?

share|improve this answer
    
This is not actually production code, so the function names are just examples (in production the function names are more descriptive). Generally, no, there are no similarities between different sections (though when there are similarities, I do try to reuse code as much as possible). –  Eric C. Mar 23 '12 at 21:13
add comment

I think this is mostly personal preference. If your functions were going to be reused (and are you sure you couldn't make them more generic and reusable?) I would definitely say split them up. I've also heard that it's a good principle that no function or method should have more than 2-3 screens of code in them. So if your big function just goes on and on, it might make your code more readable to split it up.

You don't mention what technology you're using to develop these reports. It looks like you might be using C#. Is that correct? If so, you might also consider the #region directive as an alternative if you don't want to split your function into smaller ones.

share|improve this answer
    
Yes, I'm working in C#. It is possible that I will be able to reuse some functions, but for many of these reports the different sections have vastly different data and layouts (customer requirement). –  Eric C. Mar 23 '12 at 20:10
1  
+1 except for suggesting regions. I wouldn't use regions. –  Bernard Mar 23 '12 at 20:15
    
@Bernard - Any particular reason? I'm assuming the asker would only consider using #regions if he already ruled out splitting the function. –  Joshua Carmody Mar 23 '12 at 20:17
2  
Regions tend to hide code and can be abused (i.e. nested regions). I like seeing all the code at once in a code file. If I need to visually separate code in a file I use a line of forward slashes. –  Bernard Mar 23 '12 at 20:23
2  
Regions are usually a sign the code needs refactoring –  coder Mar 23 '12 at 20:30
add comment

Breaking a function up into logical subfunctions is helpful, even if the subfunctions aren't reused -- it breaks it down into short, easily understood blocks. But it has to be done by logical units, not just n # of lines. How you should break it up will depend upon your code, common themes would be loops, conditions, operations on the same object; basically anything that you can describe as a discrete task.

So, yes, it's a good style -- this may sound strange, but given your describe limited reuse, I would recommend that you think carefully about ATTEMPTING reuse. Be sure that the usage is truly identical, and not just coincidentally the same. Attempting to handle all cases in the same block is frequently how you end up with the large ungainly function in the irst place.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.