Is an abundance of void, parameterless functions a code smell?

Question

So I'm looking over some code I've written and noticed that most of the function calls are parameterless and return void. They tend to use public properties instead of arguments and return values. This was an outgrowth of the architectural pattern I was using and I just wound up extending it. The canonical example of MVP passive view has the view exposing properties about itself and delegating all real work to the presenter through void parameterless function calls.

When I noticed I was using this in my domain and database layers I started to feel I was somehow ignoring the notion of encapsulation.

Have I misused a good idea where it was never intended to be used?

[Clarification]

All called objects hold a reference to an interface, which the calling object implements. This interface exposes any data needed by the called object to its job. In most cases, the called object only modifies a single property of the calling object. This mirrors how the presenter and view communicate in a Passive View pattern. Up to this point all exposed properties are readonly unless need dictates otherwise.

The more I think about it, the more uneasy I become with the whole approach. Its fine for view-presenter interaction because the view simply passes any changes directly to the UI. Domain and Database objects are much more complex and the potential for unintentional changes is much greater.

Answer 1

I don't believe the existence of several void returning parameterless instance methods is necessarily code smell. The only thing it's an indication of really is a method that mutates vs a pure non-mutating method. This is fairly common in object oriented programming.

It may make your code clearer in some cases to refactor these out to methods which take arguments and return values. But it would need to be evaluated on a case by case basis.

I would be worried if the methods were static / global. That would indicate the mutation of global state which is much harder to reason about than local state.

Answer 2

If your domain and database layers are accessing only interfaces then you could suppose that you are overapplying the idea of a presenter. Whether that's bad or good depends on the number of properties accessed this way and how they are accessed. It's probably not a good thing, though. Presenters definitely aren't meant to replace facades or tiers, but if you're successfully using ideas from presenter to reduce complexity and coupling, then more power to you.

If you're accessing them directly without an interface, you have massively coupled the layers which is very much against the fundamental point of having a tiered architecture. This would unquestionably be bad.

Answer 3

I definitely agree that it smells. A method like that have to do something, and since it doesnt return anything, it most probably changes the state of the object it works on. Changes that are somewhat hard to track, since you need to know which of the methods are being called, and at what point. If it goes like this:

1. MethodA
2. MethodB

-- and you change it to:

1. MethodA

2. MethodC

3. MethodB

.. you might have changed the state in some subtle way, that you know nothing about except for the post-conditions - And what's worse, you mightve broken MethodB because of a subtle pre-condition that MethodC ruins.

A few of these methods will be fine, but an "abundance" is too many :)

Answer 4

I'd say that if a typical user has to immediately follow a call to your void-returning method with a second call get the result out of your class, then yes, you should be returning that value yourself.

The guidepost should be what it makes client code look like, and how much (if any) unnessecary work it forces on them.

Answer 5

A class with several properties and a method called something like DoIt or Execute would be a code smell, because it's essentially the same thing as a single method with a bunch of parameters.

Answer 6

In the example that you give (if I understand you correctly,essentially parameter/return passing via member variables - regardless of naming them "properties" - if it was C++ they would be member variables) I do indeed think it suspect as you have immediately made the method non-thread-safe as those properties now are not local to the invokation and are now serially reusable resources.

I would say is an example of a scope error. Passing this information through Properties is making that data available at too wide a scope. Why make it a (public!) Property.

In the general case, I would say it is definitely NOT suspect, especially if they are private or protected.

For example, in much of my code I will factor relentlessly. If two methods of a class need the same 3 line idiom I factor(and sometimes 2 LOC or even 1(!) LOC if it is a VERY complicated expression, say involving arrays of objects and arrays of pointers to members.) These small helper functions usually do a very well defined something and their names are usually very natural. The compiler generally inlines them but at least I have captured the idiom and frequently I find that, sure enough, I need it again later.

Such methods are frequently of void (void) signature.

In a similar vein, breaking a class method down so that its sub-parts fit onto a screen can yield protected or private methods that are of void (void) signature when the "parameters" of interest really are member variables. (example - turning a global set of parameterized functions into a namespaced set of functions with namespace common data or a class)

But again this every different than the original situation that you describe.

Answer 7

They tend to use public properties instead of arguments and return values

I think that is a code smell, as it makes the objects more stateful and harder to analyze. To address the smell, try to factor out what methods that only rely on function parameters and return values- these methods should be static. Whats left will be wiring together of parameters to function calls.

I run into this in a lot of ASP.NET code. The pattern for builtin user controls is to expose properties where the control behavior can be customized. Often though its more appropriate to add a function to the custom control that takes those values as inputs, rather than increase the number of properties on the object.