Is my code too procedural?

Question

Someone recently took a look of my code and commented that it was too procedural. To be clear, it was not much of the code they saw - just a section which clearly outlines the logical steps taken in the application.

if(downloadFeeds(ftpServer, ftpUsername, ftpPassword, getFtpPathToLocalPathMap())) {
    loadDataSources();
    initEngine();
    loadLiveData();
    processX();
    copyIds();
    addX();
    processY();
    copyIds();
    addY();
    pauseY();
    resumeY();
    setParameters();
}

These various methods then create a whole bunch of different objects, and call various methods on these objects as required.

My question is - is a section of code which clearly drives your application, such as this, indicative of procedural programming, and if so what would be a more OO way to achieve the same outcome?

All comments are much appreciated!

Answer 1

Well, the class in which this piece of code resides has clearly too much responsibilities. I wouldn't go as far as hide all that stuff in a facade but instead of having all stuff related to some ftp engine, datasources and other entities located in a single god object (anti pattern), there should be a business process which HAS all these kinds of entities.

So the code could look more like that:

if(downloadFeeds(ftpServer, ftpUsername, ftpPassword, getFtpPathToLocalPathMap())) {
    datasource.load();
    engine.init();
    data.load();
    engine.processX(data);
    data.copyIds()
    foo.addX();
    engine.processY();
    // ...
}

Datasource, engine and all the other components may be Injected into your business process, so a) testing gets way easier, b) swapping implementations is simplified and c) code reuse is possible.

Please note that a procedural looking piece of code isn't always bad:

class Person {
    public void getMilk() 
    {
        go(kitchen);
        Glass glass = cupboard.getGlass(); 
        fridge.open(); 
        Milk milk = fridge.getMilk(); 
        use(glass, milk);
        drink(glass);
    } 
    // More person-ish stuff
}

While that code clearly looks procedural, it may be fine. It makes totally clear what happens here and does not need any documentation (clean code from martin encourages code like that). Just keep the single responsibility principle and all the other basic OOP rules in mind.