How to properly structure functions?

let's say we have a class with some methods in it, of which at least one is of rather complex nature:

class Example {
    public function Example()
    {
    }

    private function complexFunction():void
    {
        //Do this
        //And do that
        //And of course do that
        //and do not forget that
    }

    private function otherImportantFunction():void
    {
        //Do something
    }

    //[...]
}

Now "complexFunction()" has grown pretty long and also a little complicated. So a nice thing to do to increase readability would be to split up "complexFunction()" in smaller sub-functions. I usually do it like this:

class Example {
    public function Example()
    {
    }

    private function complexFunction():void
    {
        doThis();
        doThat();
        andOfCourseDoThat();
        andDoNotForgetThat();
    }

    private function doThis():void
    {
        //Do This
    }

    private function doThat():void
    {
        //Do That
    }

    private function andOfCourseDoThat():void
    {
        //And of course do that
    }

    private function andDoNotForgetThat():void
    {
        //And do not forget that
    }

    private function otherImportantFunction():void
    {
        //Do something
    }

    //[...]
}

But by now the class is already drowning in minor functions whose sole purpose is to get called once inside "complexFunction()". Do this "splitting-up" a little more often and it becomes hard to spot the important methods between all those helper-functions.

At least this is what happens to me all the time and clarity really suffers for it. That makes me wonder if you know a way to solve that dilemma? Surely there is way or 'best practice' to handle this? I'm dreaming of a way to group functions together, or to subordinate the minor ones to the superior ones, without creating a whole new class for that purpose. Or is that how it's done?

Answers


You are right to split up the one large function into multiple functions. Provided it isn't something like AddOne() instead of value++. Especially functions you'll probably repeat more often can be of use.

When your class is getting filled with multiple functions (or long functions), it might be a good idea to reconsider what your class does. Try to keep your class destined for one subject. For example, a good idea for a class is to make it User-related. Things like creating or removing users can be done in there. Even matching users to, for example, cars they own can be done in there. But don't include functions in the User-class which save or remove Cars. Save that for a different class.

In that case, your Example-class would have instances of the User-class and the Car-class. If it looks like this, you're programming efficiently:

class Example {
    function Example()
    {
    }

    function complexFunction():void
    {
        Car newCar = new Car("KK-E8", Color.Red, true);
        carManager.Add(newCar);
        User newUser = new User("Moritz", "Krohn", Country.Germany, true);
        userManager.Add(newUser);
        newUser.addCar(newCar);
        ...
    }

It happens that some classes tend to get large, even when you try to keep things organised. That is never a bad thing, neither are long functions. Just try to keep things seperated, try not to repeat code too much (create functions for that) and try to keep things related to eachother in seperate classes.


I personally find that it increases readability of complex code a lot if it is split into simple functions. What I don't really get in the code presented is the way those simpler functions are combined into one function, this seems only to work for functions that don't return anything at all (they just seem to have some effects). A more natural way imho to build up a complex function from simpler ones, would be by way of function composition, case distinction or something alike...


Just because a function is "long" does not make it more complex, Some things just take many lines of code. Take for example:

var loader:Loader = new Loader();
loader.contentLoaderInfo.addEventListener(Event.COMPLETE, completeHandler);
loader.contentLoaderInfo.addEventListener(HTTPStatusEvent.HTTP_STATUS, httpStatusHandler);
loader.contentLoaderInfo.addEventListener(Event.INIT, initHandler);
loader.contentLoaderInfo.addEventListener(IOErrorEvent.IO_ERROR, ioErrorHandler);
loader.contentLoaderInfo.addEventListener(Event.OPEN, openHandler);
loader.contentLoaderInfo.addEventListener(ProgressEvent.PROGRESS, progressHandler);
loader.contentLoaderInfo.addEventListener(Event.UNLOAD, unLoadHandler);
loader.contentLoaderInfo.addEventListener(MouseEvent.CLICK, clickHandler);
var request:URLRequest = new URLRequest(url);
loader.load(request);

Is it "long"? yes Does it have a high Cyclomatic Complexity? no You can plainly understand what is going on here and it very easy to follow. And you will find functions/methods with low CC values will always be easier to read. On the other hand methods with high(+20) CC value should be putting up a red flag. Splitting a method into separate helper methods does not remove the issue at hand and is actually making things worse for the compiler since now it has to allocate resource devoted to those methods that will only ever be called once. The rule of thumb is to avoid going above a CC value of 20. When you get above 20 it is time to start rethinking your class design as it is becoming tightly coupled.

Perhaps the question you should be asking is .

 How can I reduce the Cyclomatic Complexity of this?

Need Your Help

Rails, devise, current_user get posts.name

ruby-on-rails devise

I have Post model which is associated with devise's User. I want to know names of the posts user has when logged in.

Trigger to disable and enable button

wpf button triggers styles

I have a list of items in WPF datagrid. As source I use ObservableCollection. One of the column is also checkbox binding to the bool property of collection. In the same window, out of the grid, I h...