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?
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.
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.
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.
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.
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.
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.
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.
