Is my code too procedural?

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();
    add开发者_高级运维Y();
    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!

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.

Wow ! It doesn't seem like OO-Style. How about like this:

    ConnectionData cData = new ConnectionData(ftpServer, ftpUsername, ftpPassword, getFtpPathToLocalPathMap());


    if(downloadFeeds(cData)) {
      MyJobFacade fc = new MyJobFacade(); 
      fc.doYourJob();
    }

MyJobFacade.java

public class MyJobFacade {

    public void doYourJob() {
          /* all your do operations maybe on different objects */
    }
}

by the way Facade Pattern http://en.wikipedia.org/wiki/Facade_pattern

My question is - is a section of code which clearly drives your application, such as this, indicative of procedural programming,

It is not possible to say if this code fragment is "too procedural".

• Those calls could all be to instance methods on the current object, operating on either separate objects, or on instance variables of the current instance. These would make the code OO, at least to some degree.

• If those methods are static, then the code is indeed procedural. Whether this is a bad thing depends on whether the methods are accessing and updating state stored in static fields. (Judging from the names, they probably would need to do that.)

and if so what would be a more OO way to achieve the same outcome?

It is hard to say without looking at the rest of the code, but there appear to be some implied objects in the picture; e.g.

• data sources and (maybe) a data source manager or registry
• an engine of some kind
• something to hold the live data, the X's and the Y's
• and so on.

Some of these probably need to be classes (if they are not already classes) ... but which ones depends on what they are doing, how complicated they are and so on. State that doesn't make sense as classes (for whatever reason) can be made "more OO" by turning static variables into instance variables.

Other Answers suggest specific refactorings, getting rid of all globals, use of dependency injection and so on. My take is that it there is insufficient information to judge whether these suggestions are going to help.

But is it worth it?

Just making an application "more OO" is not a useful or worthwhile goal. Your aim should be to make the code more readable, maintainable, testable, reusable, etc. Whether using OO will improve matters depends on the current state of your code, and of the quality of your new design and the refactoring work. Simply adopting OO practices will not correct a poor design, or turn "bad" code into "good" code.

I’m going to take a different approach to critiquing this than whether or not it’s ‘too procedural’ or not. I hope you find it somewhat useful.

First off I don't see any function parameters or return values. This mean you’re probably using all kinds of global data, which should be avoided for numerous good reasons that you can read about here if you like: Are global variables bad?

Second, I don't see any error checking logic. Let’s say resumeY fails with an exception, maybe the problem is in resumeY but it could also be higher up in pauseY or as far up as loadDataSources and the problem only manifests as an exception later on.

I don’t know if this is production code or not but it’s a good candidate for re-factoring in several stages. In the first stage you can go through and make each function return a Boolean success or not and in the body of each function check for known error cases. After you have some error checking in there start getting rid of your global data by passing in function args and returning result data; you can make your functions return null values in case of failure or convert over to exception handling, I suggest exceptions. After that think about making the individual pieces testable in isolation; e.g. so you can test downloadFeeds separate from the data processing function and vice versa.

If you go through and do some re-factoring you’ll start seeing the obvious places where you can modularize your code and improve it. IMO you should worry less about if you’re OOP-enough and more about whether or not you can 1. Debug it effectively, 2. Test it effectively and 3. Understand it after leaving it alone and coming back 6 months later to maintain it.

This reply ended up quite long, I hope you found parts of it useful. :-)

Yes. Your methods don't return any values so from the snippet it appears they're operating on global variables. It looks like textbook procedural programming.

In a more OO approach I'd expect to see something like this:

if(downloadFeeds(ftpServer, ftpUsername, ftpPassword, getFtpPathToLocalPathMap()))
{
  MyDatasources ds = loadDataSources();
  Engine eng = initEngine(ds);
  DataObj data = loadLiveData(eng);
  Id[] xIds = processX(data.getX());
  Id[] newXIds = xIds.clone();
  data.addX(newXIds);
  Id[] yIds = processY(data.getY());
  Id[] newYIds = yIds.clone();
  data.addY(newYIds);
  pauseY();
  resumeY();
  someObject.setParameters(data);
}

Paul

Here's how I've learned to differentiate:

A sign that your code is getting "procedural" on you is when a class has more than one responsibility (See this link on the Single Responsibility Principle). It looks like all of these methods are being called by one object, which means that one class is managing a bunch of responsibilities.

A better way would be to assign these responsibilities to the real-world object that can best handle them. Once these responsibilities have been properly delegated, implementing a software pattern that can efficiently drive these functions (such as the Facade pattern).

This is indeed procedural programming. If you're trying to make it more OO, I'd try a combination of these:

• Try to have classes that represent the data you're holding. For example, have a FeedReader class to handle feeds, a DataLoader class to load the data, etc.
• Try to separate the methods into classes with cohesive functionality. For example, group resume(), pause() into the previous FeedReader class.
• Group the process() methods into a ProcessManager class.

Basically, just try to think of your program as having a bunch of employees working for you, and you need to assign them responsibilities. (PM do this for me, then DM do this with the result of that). If you give all the responsibility to one employee, he or she is going to be burned out.

I agree that it looks too procedural. One obvious way to make it look less procedural is to have all of these methods be a part of a class. Erhan's post should give you a better idea of how to break it up :-)