In my workplace there's currently a discussion going on about regions in C#. I like regions (and collapsing functions, for that matter), because they help me regard pieces of code that I'm not dealing with as black boxes. I have a tendency to keep everything
collapsed, and only expand the parts that I'm currently working on.
A colleague of mine agrees, but he's taking it a step further. We're currently refactoring an application done by an intern, and he has a tendency to use as few functions as possible, which is commendable, but he then proceeds to make those functions 500 lines
long.
Now, my colleague feels that it's fine to identify everything he's doing in the function ("Initializing session variables" "adding controls", stuff like that), and wrapping it in regions. When collapsed, it becomes a nice 20-line affair, and you can expand
the parts you wish to see.
I, however, prefer refactoring all that stuff into separate functions, and having the main function just call them in succession, so that each function is roughly one 'screen' long. Possibly because this is how I've always done it, but it 'feels' better to
me if I do it this way. The criticism I get for this is that it results in a "crapload of small functions" and the overhead of function calls. I think the latter is trivial, but I have to admit that he has a point when a class ends up with 15 or more private
functions.
So, I'm wondering. What's your preferred method of tackling beastly Page_Load()'s that span several hundreds of lines?
-
Bas
It finds lightbulbs.
-
Dr Herbie
Horses for courses
Bas wrote:In my workplace there's currently a discussion going on about regions in C#. I like regions (and collapsing functions, for that matter), because they help me regard pieces of code that I'm not dealing with as black boxes. I have a tendency to keep everything collapsed, and only expand the parts that I'm currently working on.
A colleague of mine agrees, but he's taking it a step further. We're currently refactoring an application done by an intern, and he has a tendency to use as few functions as possible, which is commendable, but he then proceeds to make those functions 500 lines long.
Now, my colleague feels that it's fine to identify everything he's doing in the function ("Initializing session variables" "adding controls", stuff like that), and wrapping it in regions. When collapsed, it becomes a nice 20-line affair, and you can expand the parts you wish to see.
I, however, prefer refactoring all that stuff into separate functions, and having the main function just call them in succession, so that each function is roughly one 'screen' long. Possibly because this is how I've always done it, but it 'feels' better to me if I do it this way. The criticism I get for this is that it results in a "crapload of small functions" and the overhead of function calls. I think the latter is trivial, but I have to admit that he has a point when a class ends up with 15 or more private functions.
So, I'm wondering. What's your preferred method of tackling beastly Page_Load()'s that span several hundreds of lines?
My gut feeling is that using regions in this case is treating the symptom, not the cause. But then I don't know what sort of code you have in your load method.
If it's just a sequential list of setup stuff, then I'd be happy with regions, but if there's any real logic, I'd want it split out into methods.
However, I often find that the way I code up a screen I need to re-use initialisation code in other places; like where I have a 'refresh data' button to re-get from the database, or when I'm lazy loading data in response to combobox selections.
An argument for the 'lots of small methods' argument is that it's easier to pinpoint errors quickly and if you need to optimise, the perf statistics go by method (so if you have a couple of lines of code slowing everything up they're easier to find if everything's split smaller methods).
Herbie
-
Massif
aim stupidly high, expect to fail often.
What Herbie said (I'm not up to stringing much in the way of a sentence together this morning.)
I'm all for factoring out into smaller functions; but if the code's only being used once, then there's no real point. In that case I'd say using regions to tidy functions which have to be long is a reasonable approach. -
littleguru
<3 Seattle
Regions don't fix it. But they could help you to fix it. I mean if you find regions that are the same in different methods try to extract them into smaller methods. VS2005 offers the tools to do so.
I have always been a fan of small methods - not for everything, but if I'm going to reuse that piece of code, yes.
I don't know what people have with these long and long methods. If you split'em up in smaller methods the JITter can perfectly line them in... -
odujosh
Need Microsoft SUX now!
PageLoad that are several hundred lines points to poor design. (Nearly always)
Some things to trim the fat:
1)Do you make any ADO.NET calls in Page Load? Why not use a a DataSource or factor it into a Data Access Layer so all pages can use past work?
2)Does the code deal Business Logic. Do you catch yourself doing logic in terms of UI? Start with helper classes/managers and work towards business objects.
3)Validation embedded in page code?
4)Do you have to test the state of controls to do the logic. This leads to you discovering better events to run logic. Everything should not run in PageLoad event. In fact before you add something to PageLoad you should think is there somewhere else I can be doing this. Does this really deal with the page loading.
5)Have you looked into User Controls/Custom Controls/Templated controls. This makes a sprocket reusuable, also lets you isolate code to a particulair specilization, makes the code and more importabtly markup more discoverable. (its hard to follow if else mazes) -
Bas
It finds lightbulbs.
I'm just talking huge functions in general, not necessarily Page_Load, not even necessarily ASP.NET. I just used a gigantic Page_Load() as an example, because it's a commonly abused function for hundreds of lines of initialisation code.
-
Ion Todirel
buuu
usually methods should'n be larger than a screen
-
Dr Herbie
Horses for courses
Bas wrote:I'm just talking huge functions in general, not necessarily Page_Load, not even necessarily ASP.NET. I just used a gigantic Page_Load() as an example, because it's a commonly abused function for hundreds of lines of initialisation code.
Well, I guess my general comments about small methods still stand -- easier debugging and optimising, but wouldn't worry if it's generally sequential code with little logic flow. I'd use a cyclometric complexity of about 3 as a rule of thumb for splitting long methods, although the system I currently work on has methods with 700+ lines and cyclometric complexity > 100. Not allowed to refactor them as customer won't pay for it
Herbie
-
evildictaitor
strcpy(malloc(4), "Oh noes!");
It's a pity you can't instruct the .NET compilers to inline a function (C/C++ have the inline keyword on GCC, __inline and __forceinline in MSC) because someone might want to access your private functions via reflection later (grr).
If you could just create private methods and set them to __inline, the problem goes away because you always extract to little functions. -
W3bbo
Work hard; increase production; prevent accidents, and be happy.
evildictaitor wrote:It's a pity you can't instruct the .NET compilers to inline a function (C/C++ have the inline keyword on GCC, __inline and __forceinline in MSC) because someone might want to access your private functions via reflection later (grr).
If you could just create private methods and set them to __inline, the problem goes away because you always extract to little functions.
There's no point. The JIT compiler automatically inlines functions.
This is why trivial Get/Set properties are just as fast as direct fieldsets when running with "Release" code.
-
Dr Herbie wrote:but wouldn't worry if it's generally sequential code with little logic flow. I'd use a cyclometric complexity of about 3 as a rule of thumb for splitting long methods
Herbie
I agree. Long functions with generally sequential flow are acceptable, especially where each task is conceptually similar, e.g., setup code.
I run a Cyclomatic Complexity tool from Developer Express which I refer to periodically to help me to keep my functions manageable. With this tool they recommend cyclomatic complexity < 10 and something called maintenance complexity < 200.
Complexity Measures
-
blowdart
Peek-a-boo
Bas wrote:
Now, my colleague feels that it's fine to identify everything he's doing in the function ("Initializing session variables" "adding controls", stuff like that), and wrapping it in regions. When collapsed, it becomes a nice 20-line affair, and you can expand the parts you wish to see.
The obvious answer is to remove the regions. And put everything on one line, seperated by semi colons.
-
Dr Herbie
Horses for courses
blowdart wrote:
Bas wrote:
Now, my colleague feels that it's fine to identify everything he's doing in the function ("Initializing session variables" "adding controls", stuff like that), and wrapping it in regions. When collapsed, it becomes a nice 20-line affair, and you can expand the parts you wish to see.
The obvious answer is to remove the regions. And put everything on one line, seperated by semi colons.
<sarcasm>
Yes, 'cos that saves paper when you print it out to take home and read over the weekend.
</sarcasm>
Honestly, you non-technical guys ....
Herbie
-
evildictaitor
strcpy(malloc(4), "Oh noes!");
W3bbo wrote:
evildictaitor wrote:
It's a pity you can't instruct the .NET compilers to inline a function (C/C++ have the inline keyword on GCC, __inline and __forceinline in MSC) because someone might want to access your private functions via reflection later (grr).
If you could just create private methods and set them to __inline, the problem goes away because you always extract to little functions.
There's no point. The JIT compiler automatically inlines functions.
This is why trivial Get/Set properties are just as fast as direct fieldsets when running with "Release" code.
The JIT compiler never inlines functions that are over 32 bytes of IL, so in the example I gave (of call-once functions that are extracted from #regions) will never be inlined, thus creating overhead. It also never inlines methods containing loops or try..catch statements.
-
evildictaitor wrote:
The JIT compiler never inlines functions that are over 32 bytes of IL, so in the example I gave (of call-once functions that are extracted from #regions) will never be inlined, thus creating overhead. It also never inlines methods containing loops or try..catch statements.
That's because once you go over 32 bytes of IL you're more likely to degrade performance by inlining than you are to improve it. Branch prediction, speculative execution and caching can often negate the supposed benefits of inline functions. -
I tend to write things out full and if I notice that I'm repeating something, I'll promote it to a function/method. Other times, I'll just use functions for sequential compartmentalization. Anything I can give a quick descriptive name, I'll tend to compartmentalize. eg: InitializeVariables, OpenConnections, LayoutControls, etc.
-
Ion Todirel
buuu
Ya agree, cyclomatic complexity is a very good method to detect bad methods, i miss Developer Express add-inNewWorldMan wrote:I run a Cyclomatic Complexity tool from Developer Express which I refer to periodically to help me to keep my functions manageable. With this tool they recommend cyclomatic complexity < 10 and something called maintenance complexity < 200.
Complexity Measures
-
evildictaitor
strcpy(malloc(4), "Oh noes!");
AndyC wrote:
evildictaitor wrote:
The JIT compiler never inlines functions that are over 32 bytes of IL, so in the example I gave (of call-once functions that are extracted from #regions) will never be inlined, thus creating overhead. It also never inlines methods containing loops or try..catch statements.
That's because once you go over 32 bytes of IL you're more likely to degrade performance by inlining than you are to improve it. Branch prediction, speculative execution and caching can often negate the supposed benefits of inline functions.
Given a function of N statements it is possible to fully optimise it with O(N2), but because this can become expensive very quickly, you can produce a sub-optimal, but still good algorithm with O(N) compexity.
The problem of inlining a function is that it (by definition) increases the length of the function containing it, and the IL compiler then might "overstep" the boundary as to whether to use the optimal (but expensive) algorithm versus the sub-optimal (but cheap) compilation algorithm.
This is one of the differences between the Release and Debug options in the compiler - a Debug compilation has the priority of doing it fast, rather than doing it well.
Inlining a non-recursive function will always either improve the function call, or will leave it exactly the same. There is never a "downside" to inlining beyond the process of doing the inlining and subsequent optimising of the parent function (and the possibility of making your code go beyond memory-page boundaries). The reason the IL compiler chooses not to inline larger IL functions is that the overhead of call / function execution makes the inlining process less worthwhile, but also that it might "push" the parent function to the suboptimal optimiser, when it would otherwise have been fully optimised.
This being said, if I know that a function should be inlined, I object to the compiler preventing me from informing it that it should be. I can inline my functions manually in C / C++ (even with C++/CIL) but not on C#, and I find that irritating.
Thread Closed
This thread is kinda stale and has been closed but if you'd like to continue the conversation, please create a new thread in our Forums,
or Contact Us and let us know.