14

I inherited a code base where there is a lot of code that goes something like this:

SomeDataAdapter sda = new SomeDataAdapter();
sda.UpdateData(DataTable updateData);

And then sda is never used again.

Is that a code smell that indicates that those methods should actually be static class methods instead?

  • Do these classes implement any interfaces? – Reactgular Mar 19 '14 at 19:18
  • 7
    The day after you refactor to static methods will be the day you want to run unit tests with a mocked version of your data adapter. – Mike Mar 19 '14 at 19:19
  • 22
    @Mike: Why would you ever need to mock a static method? I'm getting awfully tired of this fallacious argument that static methods are not testable. If your static methods are holding state or creating side effects, that is your fault, not the fault of your testing methodology. – Robert Harvey Mar 19 '14 at 19:21
  • 9
    In my opinion, it's a language smell showing the weakness of "everything must be a class" analysis. – Ben Mar 19 '14 at 20:51
  • 7
    @RobertHarvey: you may need to mock a static method for the same reason you mock any other method: it is too expensive to call during unit-testing. – kevin cline Mar 19 '14 at 21:53
1

I would consider it an architecture smell in that UpdateData probably should belong to a 'service' class.

Where the data is an Apple. Where AppleAdapter is service/business-intelligence class. Where AppleService is a Singleton reference to an AppleAdapter that exists outside of the current method.

private static volatile AppleAdapter _appleService = null;
private static object _appleServiceLock = new object();
private AppleAdapter AppleService
{
    get
    {
        if (_appleService == null)
        {
            lock (_appleServiceLock)
            {
                if (_appleService == null)
                    _appleService = new AppleAdapter();
            }
        }
        return _appleService;
    }
}

public SomeAppleRelatedMethod(Apple apple)
{
    AppleService.UpdateData(apple);
}

I don't think what you are doing is wrong necessarily but if SomeDataAdapter does indeed represent some kind of stateless business service, then a singleton would be the best practice for it. Hope that helps! The example provided is fancy way to ensure no contention of the _appleService if it happened to be both null and accessed at exactly the same time by two or more threads.

You know what? If SomeDataAdapter is an ADO IDbDataAdapter (which it almost certainly is), disregard this entire response!

:P

I don't have permission to add a comment to the original question, but if you could specify where this code exists.

If this code represents a custom implementation of an IDbDataAdapter, and UpdateData is creating an IDbConnection, IDbCommand, and wiring it all up behind the scenes, then no I wouldn't consider that a code smell because now we're talking about streams and other things that need to be disposed of when we're done using them.

  • 12
    Help! I'm drowning in software patterns. – Robert Harvey Mar 19 '14 at 19:44
  • 4
    ...or injected as a dependency. ;) – Rob Mar 19 '14 at 19:46
  • 12
    Messing with singletons and double-checked locking to get something equivalent to a simple function/procedure is a far bigger code smell to me! – Ben Mar 19 '14 at 22:02
3

I think this problem goes even deeper than that. Even if you do refactor it into a static method, then you get code where you call single static method all over the place. Which in my opinion is code smell in itself. It might indicate you are missing some important abstraction. Maybe you want to create a code that will do some pre and post-processing while allowing you to change what happens in between. That pre and post-processing will contain the common calls while the in between will depend on concrete implementation.

  • I agree there is a much deeper problem. I am trying to incrementally improve it without a full architectural redesign but it sounds like what I was thinking is not a good way to do it. – Shane Wealti Mar 19 '14 at 20:08
1

Kind of. It usually means you want to pass a function around and your language of choice won't let you. It's kind of clumsy and inelegant, but if you're stuck in a language without first-class functions, there's not a whole lot you can do.

If none of those objects are getting passed as arguments to other functions, you could turn them into static methods and nothing would change.

  • It could also mean that you want the static method to return a modified version (or a modified copy) of an object you are passing to it. – Robert Harvey Mar 19 '14 at 19:26
  • 2
    "if you're stuck in a language without first-class functions": There is no difference between an object with only one method and a first-class function (or first-class closure): They are just two different views of the same thing. – Giorgio Mar 19 '14 at 19:35
  • 4
    @Giorgio Theoretically, no. In practice, if your language doesn't at least have a syntax sugar for it, it's the difference between fn x => x + 1 and new Function<Integer, Integer>() { public Integer eval(Integer x) { return x + 1; }}; or limiting yourself to a few hard-coded and narrowly-useful functions. – Doval Mar 19 '14 at 19:42
  • Why couldn't fn x => x + 1 be syntactic sugar for new Function<Integer, Integer>() { public Integer eval(Integer x) { return x + 1; }}? Ok, maybe you mean if one is stuck in a language without this kind of syntactic sugar. – Giorgio Mar 19 '14 at 19:44
  • @Giorgio You'll have to ask Oracle. (Granted, that's finally changing with Java 8). Note that you'd need more syntax sugar than just that to be really useful - you'd also want to be able to pass pre-declared methods by name. Currying would also be handy. At some point you have to wonder if it's worth having all those hacks instead of treating functions as first citizens. But now I'm getting off-topic. – Doval Mar 19 '14 at 19:48
0

If holding state makes a given class more utilitarian, functional … re-usable then no. For some reason the command design pattern comes to mind; that reason certainly is not a desire to have Robert Harvey drown.

0

Define "frequently." How often do you instantiate the object? A lot - probably not?

There are likely bigger problems to worry about in your system. Going static will communicate more clearly to the programmer that internal state of the object isn't changing - great.

If state is being messed with though, and you have to start making other things static to make the first thing static, then you have to ask what parts need to be static and what parts don't.

Yes it's a code smell, just a small one.

0

Make it a static method and move on. There is nothing wrong with static methods. Once a pattern of use emerges then you can worry about abstracting the pattern.

0

The smell here is that this is procedural code wrapped (minimally) in objects.

There must be a reason why you want to perform that operation on the data table, but rather than modelling that operation with other related operations that share some semantics (i.e. have a common meaning), the author just spun up a new procedure inside a new class & called it.

In a functional language, that's how you'd do things ;) but in the OO paradigm, you'd want to model some abstraction that included that operation. Then you'd use OO techniques to flesh out that model and provide a set of related operations that preserve some semantics.

So the main smell here is that the author is using classes, probably because the compiler requires it, without organizing operations into a conceptual model.

BTW watch out anytime you see the program being modeled instead of the problem space. It's a sign that the design could benefit from more thought. In other words, watch out for classes all being "xAdaptor" and "xHandler" and "xUtility", and usually tied to an anemic domain model. It means someone is just bundling code up for convenience, as opposed to actually modelling the concepts they want to implement.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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