6

I've been using Sonar code quality management platform for some time, and for the most cases I find it very helpful in revealing hidden design flaws of my code base.

However, there's one rule that gives me more annoyance than help and that is its check for 'cyclic package reference' violations.

I guess I fully understand where such a dependency between packages is a bad thing. For example, in a typical 3-tier presentation/service/persistence layered design, it's almost always a bad idea to let the database handling code have a reference back to UI related classes. I have no problem with calling it a 'violation'.

But let's consider other cases, i.e. like designing an IDE-like application. Say, we have a main package which contains an Application interface, which defines List<View> Application.getViews() method to reference application's views.

However, when the View interface has an Application getApplication() method to refer back to its parent application, which I believe is a quite common design, it will introduce a cyclic reference, provided each of the interfaces are separated in com.myapp.ui, and com.myapp.ui.view respectively.

Of course, you can just put the View interface into the com.myapp.ui to break the cycle. But when you have various other view related APIs in com.myapp.ui.view, many of them another abstract APIs like AbstractView, ContentView, AbstractContentView, etc. I wonder if it's more advisable to keep them in separate packages for a management purpose.

And consider the said application has many other similar cases like com.myapp.ui.action, com.myapp.ui.perspective, etc. which would really make com.myapp.ui package crowded if we are to put them all in there.

So, what approach do you suggest to handle such a situation? Are really every cyclic package references a bad thing? Or if I have to live with them, how do you configure Sonar to check only real, problematic cycles?

Tomerikoo
  • 18,379
  • 16
  • 47
  • 61
mysticfall
  • 302
  • 2
  • 7

2 Answers2

6

Every absolute -- except this one ;) -- is going to be wrong some of the time. So, is every cyclic reference bad? No. You have to use your judgement.

But if you do introduce a cyclic dependency, it's worth asking if you really need it, and why. The tl;dr is that more often than not, you may find that breaking the cycle can improve your modularity, and in particular your ability to test components separately.

To use your example, does a view really need a getApplication(), which presumably returns a relatively "heavy" object (ie, one that itself needs a database, network, etc etc)? Maybe... but maybe not. If what you really need from that getApplication is something with a few callbacks (such as when a user initiates some action), then it could be useful to create an interface in some common package for that callback. So, rather than:

com.foo.app.Application
com.foo.view.View
    Application getApplication()

You'd have:

com.foo.common.Callback // maybe just a Callable, Runnable, etc?
com.foo.app.Application
    provides a Callback for some action foo
com.foo.view.View
    Callback getFooCallback()

The question you should be asking is: what does that give me? It could be that you have to stub out so much that it doesn't give you much -- though that may suggest you can break apart your classes some. But it could be that it actually makes it easier to test your view, because now your unit test can (1) test the view without spinning up a whole application, and (b) provide a "dummy" callback that does something like saving a string that describes the action, and then your unit test asserts that it saved the right string.

yshavit
  • 42,327
  • 7
  • 87
  • 124
  • Let me please try to think up a case where View.getApplication() could be justified. I suppose it could be a common requirement for a view to reference other resources normally associated with the application, like an i18n message bundle or a JDBC data source, etc. Maybe we can all sort them out in a DI manner, but I believe by definition, a view in that case is a part of a composite whole that cannot be thought in a separated context. So, JDBC datasource cannot be an arbitary one, but one that its parent application provides. wouldn't it be more clear to show such dependencies in design also? – mysticfall May 14 '13 at 05:38
0

And indeed there is an open JIRA ticket to prevent considering a cycle between father/child packages as a quality flaw : http://jira.codehaus.org/browse/SONAR-3452

Freddy - SonarSource Team
  • 2,850
  • 1
  • 15
  • 13