Sonar code analysis of VASSAL

You can view a code analysis of the VASSAL engine here:

strategicinitiative.org:9000 … SAL:VASSAL

[size=150]Cycles[/size]

The biggest thing that jumps out at me is the 23% package tangle index with 615 cycles. Most of you probably know what dependency cycles are, but in case the concept is new to some of you, I’ll describe what they are and how to fix them. A system that is properly layered has 0 cycles. Code as tangled as VASSAL is is much harder to write tests for. If I were to tackle cleaning up the VASSAL code, this is where I’d start: introduce interfaces and reorganize packages to get the dependency cycles down to 0. It’s not that hard; the main thing you need to think about is how you want your code to be layered. Lower layers should know nothing about layers above them that use them (i.e. no imports from higher layers). There’s a jdepend plugin for Eclipse that can help make this work easier.

[size=150]Duplication[/size]

The next one I’d probably look at is code duplication. I see two practically identical ConcurrentSoftHashMap classes–that just looks like an error. The code duplication in HexGrid and SquareGrid look like they just need refactoring. Similarly for a bunch of other code duplication Sonar found…

[size=150]Complexity[/size]

This one can be a bit religious. But the recommended complexity for a class is 4 or 5. An average of 20.2 is way too high–it suggests that there are many classes that have more than one responsibility. This problem also has a big impact on unit testing–it’s harder to test a big gangly class that does too many different things.

[size=150]Code coverage[/size]

The number to aim for is 75%. 1.4% is, err, a serious problem. Having no test coverage is particularly problematic in a project like VASSAL where it’s an engine that other people write code (modules) to run on it. How will you know if your code change will break any modules out there if you don’t have test coverage?

[size=150]Rule compliance[/size]

This one is lower down on the priority list for me compared to the others. It’s a good thing to keep an eye on as you go along. Code with no Major/Critical violations is WAY easier to maintain than code that has those violations. Some of the rules are debatable, and stylistic in nature. Some of the critical ones are genuinely scary like storing arrays, equals hash, suspicious equals–those are probably bugs. Visibility modifiers may look like it’s a style thing, but reducing visibility has some subtle but valuable benefits–e.g. it allows the compiler to detect whether anybody actually uses that method/field. Over time, unused methods/fields accumulate. Reducing their visibility makes the job of cleaning them up easier. But like I said, I’d put rules compliance way down the list compared to the bigger problem of layering, big classes, and test coverage.

Ken

P.S. Note that Sonar requires Maven–Sonar is one of the reasons I moved VASSAL into Maven.

Thus spake fil512:

Duplication

The next one I’d probably look at is code duplication. I see two
practically identical ConcurrentSoftHashMap classes–that just looks
like an error.

It’s not. One of them is deprecated. This is the result of moving the
class from one package to another.

This one is lower down on the priority list for me compared to the
others. It’s a good thing to keep an eye on as you go along. Code with
no Major/Critical violations is WAY easier to maintain than code that
has those violations. Some of the rules are debatable, and stylistic in
nature. Some of the critical ones are genuinely scary like storing
arrays, equals hash, suspicious equals–those are probably bugs.

Some of these I know are intentional, like when we catch Throwable,
and when we pass arrays as arguments without copying them in the graphics
subsystem. Others, maybe not. I wouldn’t be surprised by anything you
find in there, really.

Visibility modifiers may look like it’s a style thing, but reducing
visibility has some subtle but valuable benefits–e.g. it allows the
compiler to detect whether anybody actually uses that method/field.
Over time, unused methods/fields accumulate. Reducing their visibility
makes the job of cleaning them up easier. But like I said, I’d put
rules compliance way down the list compared to the bigger problem of
layering, big classes, and test coverage.

On the flip side, reducing visibility also makes it harder to write
custom code.


J.

Thus spake fil512:

The biggest thing that jumps out at me is the 23% package tangle index
with 615 cycles. Most of you probably know what dependency cycles are,
but in case the concept is new to some of you, I’ll describe what they
are and how to fix them. A system that is properly layered has 0
cycles. Code as tangled as VASSAL is is much harder to write tests for.
If I were to tackle cleaning up the VASSAL code, this is where I’d
start: introduce interfaces and reorganize packages to get the
dependency cycles down to 0. It’s not that hard; the main thing you
need to think about is how you want your code to be layered. Lower
layers should know nothing about layers above them that use them (i.e.
no imports from higher layers). There’s a jdepend plugin for Eclipse
that can help make this work easier.

I’m guessing that it will be completely impossible to do this without
breaking the custom code in every VASSAL module which has any. Can you
comment on that?


J.

Instead of duplicating the code, could you use an empty subclass? More code = more bugs.

Sometimes the opposite is true. There is functionality you want extension designers to customize, and there’s functionality that, if they changed, would get them into trouble. Visibility can help extension designers tell which is which.

Reducing visibility can also help reduce cycles and improve layering, as there will be less chances of something “leaking” out of a package into lower packages it depends on.

Are all extensions in the VASSAL subversion repository? How many are they? Do they all work now? Have VASSAL engine releases ever broken extensions in the past?

There is probably only a subset of VASSAL classes that custom extensions depend on. Can we find out what that subset is? Could it be formalized using interfaces and a dedicated jar for people writing extensions?

If we have all the extension code, then all we’d need to do is reorganize imports on them after moving the classes and release new versions of the extensions with the new VASSAL release.

-K

Thus spake fil512:

It’s not. One of them is deprecated. This is the result of moving the
class from one package to another.

Instead of duplicating the code, could you use an empty subclass? More
code = more bugs.

That’s what I’ve done in other cases of package relocation. I infer from
this that I must have had some reason not to in this case, though I can’t
recall what it was. Usually when I do things like this I note the reason
in the revision log or in a comment; you might find the reason there.

Sometimes the opposite is true. There is functionality you want
extension designers to customize, and there’s functionality that, if
they changed, would get them into trouble. Visibility can help
extension designers tell which is which.

I’m continually surprised by how badly visibility was set for some
fields and methods in Swing. It’s amazingly frustrating to be unable
to override a method you know is safe to override because you’ve read
the code for it. This experience makes me think that we will not do a
lot better in this regard if me make more methods or fields private.


J.

Thus spake fil512:

I’m guessing that it will be completely impossible to do this without
breaking the custom code in every VASSAL module which has any. Can you
comment on that?

Are all extensions in the VASSAL subversion repository?

VASL and VSQL are.

How many are they?

Dozens to hundreds. No idea, really. I was hoping to determine that when
someone does a compatibility survey of modules we host. (As for modules
we don’t host, I can’t even begin to guess how we’d find out.)

Do they all work now?

I would be willing to be any amount of money that you could find some
modules which have 3.1-incompatible custom code.

Have VASSAL engine releases ever broken extensions in the past?

3.0 to 3.1 broke a lot of custom code, but much of this was due to
intentoinal changes. Some 3.1.x releases have also; usually that was
because the custom code was relying on some buggy behavior which the
release corrected, though sometimes we’ve accidentally broken things
as well.

There is probably only a subset of VASSAL classes that custom extensions
depend on. Can we find out what that subset is?

You could determine that by looking at each module we host and checking
whether it contains any custom classes which extend core classes. Probably
the easiest way to do that would be with a Perl script, direclty on the
server.

Could it be formalized
using interfaces and a dedicated jar for people writing extensions?

Maybe. I can’t tell at this point.

If we have all the extension code, then all we’d need to do is
reorganize imports on them after moving the classes and release new
versions of the extensions with the new VASSAL release.

In some sense, they’re not really ours to release. (A lot of the custom
code doesn’t even have a license indicated in it.) Doing this would
involve potentially dozens of module maintainers, as well.


J.

These are the rules of thumb I usually follow for visibility:

Unless a field is final, it’s hard to make the case for a field being anything but private.

Methods on the other hand I think should mostly be protected, with little internal classes being private. Those rare methods that need to be called by higher layers should be public (i.e. are a part of the public API of the package).

-K

What is a compatibility survey and how would a person go about performing one?

Thus spake fil512:

“uckelman” wrote:

How many custom code extensions are there?

Dozens to hundreds. No idea, really. I was hoping to determine that
when
someone does a compatibility survey of modules we host. (As for
modules
we don’t host, I can’t even begin to guess how we’d find out.)

What is a compatibility survey and how would a person go about
performing one?

In the files table on each module page, there’s a column labeled
“compatability”. The intended value in that column is a list of
VASSAL versions which the module is compatible with. (E.g., “3.0,
3.1”.) Right now, this isn’t filled in for most modules. Filling
it in would involve loading each module in various versions of
VASSAL to see if it works.


J.

Would one need to know how to play all the games to make this determination, or is there some sort of “smoke test” that would indicate compatibility right away?

Thus spake conradz45:

“uckelman” wrote:

In the files table on each module page, there’s a column labeled
“compatability”. The intended value in that column is a list of
VASSAL versions which the module is compatible with. (E.g., “3.0,
3.1”.) Right now, this isn’t filled in for most modules. Filling
it in would involve loading each module in various versions of
VASSAL to see if it works.

Would one need to know how to play all the games to make this
determination, or is there some sort of “smoke test” that would indicate
compatibility right away?

You can determine incompatibility right away if the module won’t load,
or if you load the module and then can’t load some predefined setup.
I don’t know of any way that you could conclusively demonstrate
compatibility (it’s probably a variant of the Halting Problem).


J.

I just wanted to shout out here that visibility reduction sucks. You have know idea what a future developer wants to override until they’re faced with a nasty private declaration (or, God forbid, final class declaration modifier). In a program like VASSAL which permits custom classes in modules, visibility reduction is the equivalent of putting a closed sign on the only gas station for 800km.

  • M

On 2010-08-17, at 10:48 AM, fil512 ken.stevens@sympatico.ca wrote:

You can view a code analysis of the VASSAL engine here:

strategicinitiative.org:9000 … SAL:VASSAL[1]

Cycles

The biggest thing that jumps out at me is the 23% package tangle index
with 615 cycles. Most of you probably know what dependency cycles are,
but in case the concept is new to some of you, I’ll describe what they
are and how to fix them. A system that is properly layered has 0
cycles. Code as tangled as VASSAL is is much harder to write tests for.
If I were to tackle cleaning up the VASSAL code, this is where I’d
start: introduce interfaces and reorganize packages to get the
dependency cycles down to 0. It’s not that hard; the main thing you
need to think about is how you want your code to be layered. Lower
layers should know nothing about layers above them that use them (i.e.
no imports from higher layers). There’s a jdepend plugin for Eclipse
that can help make this work easier.

Duplication
The next one I’d probably look at is code duplication. I see two
practically identical ConcurrentSoftHashMap classes–that just looks
like an error. The code duplication in HexGrid and SquareGrid look like
they just need refactoring. Similarly for a bunch of other code
duplication Sonar found…

Complexity

This one can be a bit religious. But the recommended complexity for a
class is 4 or 5. An average of 20.2 is way too high–it suggests that
there are many classes that have more than one responsibility. This
problem also has a big impact on unit testing–it’s harder to test a big
gangly class that does too many different things.

Code coverage

The number to aim for is 75%. 1.4% is, err, a serious problem. Having
no test coverage is particularly problematic in a project like VASSAL
where it’s an engine that other people write code (modules) to run on
it. How will you know if your code change will break any modules out
there if you don’t have test coverage?

Rule compliance

This one is lower down on the priority list for me compared to the
others. It’s a good thing to keep an eye on as you go along. Code with
no Major/Critical violations is WAY easier to maintain than code that
has those violations. Some of the rules are debatable, and stylistic in
nature. Some of the critical ones are genuinely scary like storing
arrays, equals hash, suspicious equals–those are probably bugs. Visibility modifiers may look like it’s a style thing, but reducing
visibility has some subtle but valuable benefits–e.g. it allows the
compiler to detect whether anybody actually uses that method/field. Over time, unused methods/fields accumulate. Reducing their visibility
makes the job of cleaning them up easier. But like I said, I’d put
rules compliance way down the list compared to the bigger problem of
layering, big classes, and test coverage.

Ken

P.S. Note that Sonar requires Maven–Sonar is one of the reasons I
moved VASSAL into Maven.

[1] strategicinitiative.org:9000 … SAL:VASSAL


Read this topic online here:
Sonar code analysis of VASSAL


messages mailing list
messages@vassalengine.org
vassalengine.org/mailman/listinfo/messages

As I posted earlier, I have never advocated making methods private. I
advocate making fields private unless they’re final, and making methods
protected.

-----Original Message-----
From: messages-bounces@vassalengine.org
[mailto:messages-bounces@vassalengine.org] On Behalf Of Michael Kiefte
Sent: August 21, 2010 4:45 PM
To: messages@vassalengine.org
Subject: Re: [messages] [Developers] Sonar code analysis of VASSAL

I just wanted to shout out here that visibility reduction sucks. You have
know idea what a future developer wants to override until they’re faced with
a nasty private declaration (or, God forbid, final class declaration
modifier). In a program like VASSAL which permits custom classes in modules,
visibility reduction is the equivalent of putting a closed sign on the only
gas station for 800km.

  • M

On 2010-08-17, at 10:48 AM, fil512 ken.stevens@sympatico.ca wrote:

You can view a code analysis of the VASSAL engine here:

strategicinitiative.org:9000 … SAL:VASSAL[1]

Cycles

The biggest thing that jumps out at me is the 23% package tangle index
with 615 cycles. Most of you probably know what dependency cycles
are, but in case the concept is new to some of you, I’ll describe what
they are and how to fix them. A system that is properly layered has 0
cycles. Code as tangled as VASSAL is is much harder to write tests for.
If I were to tackle cleaning up the VASSAL code, this is where I’d
start: introduce interfaces and reorganize packages to get the
dependency cycles down to 0. It’s not that hard; the main thing you
need to think about is how you want your code to be layered. Lower
layers should know nothing about layers above them that use them (i.e.
no imports from higher layers). There’s a jdepend plugin for Eclipse
that can help make this work easier.

Duplication
The next one I’d probably look at is code duplication. I see two
practically identical ConcurrentSoftHashMap classes–that just looks
like an error. The code duplication in HexGrid and SquareGrid look
like they just need refactoring. Similarly for a bunch of other code
duplication Sonar found…

Complexity

This one can be a bit religious. But the recommended complexity for a
class is 4 or 5. An average of 20.2 is way too high–it suggests that
there are many classes that have more than one responsibility. This
problem also has a big impact on unit testing–it’s harder to test a
big gangly class that does too many different things.

Code coverage

The number to aim for is 75%. 1.4% is, err, a serious problem.
Having no test coverage is particularly problematic in a project like
VASSAL where it’s an engine that other people write code (modules) to
run on it. How will you know if your code change will break any
modules out there if you don’t have test coverage?

Rule compliance

This one is lower down on the priority list for me compared to the
others. It’s a good thing to keep an eye on as you go along. Code
with no Major/Critical violations is WAY easier to maintain than code
that has those violations. Some of the rules are debatable, and
stylistic in nature. Some of the critical ones are genuinely scary
like storing arrays, equals hash, suspicious equals–those are
probably bugs. Visibility modifiers may look like it’s a style thing,
but reducing visibility has some subtle but valuable benefits–e.g. it
allows the compiler to detect whether anybody actually uses that
method/field. Over time, unused methods/fields accumulate. Reducing
their visibility makes the job of cleaning them up easier. But like I
said, I’d put rules compliance way down the list compared to the bigger
problem of layering, big classes, and test coverage.

Ken

P.S. Note that Sonar requires Maven–Sonar is one of the reasons I
moved VASSAL into Maven.

[1]
strategicinitiative.org:9000 … SAL:VASSAL


Read this topic online here:
Sonar code analysis of VASSAL


messages mailing list
messages@vassalengine.org
vassalengine.org/mailman/listinfo/messages

Hi Ken,

That rant wasn’t directed at you specifically. I have, however, encountered a lot of private methods which didn’t need to be private. I’m perfectly happy with protected declarations.

  • M

On 2010-08-25, at 8:34 AM, “Ken Stevens” ken.stevens@sympatico.ca wrote:

As I posted earlier, I have never advocated making methods private. I
advocate making fields private unless they’re final, and making methods
protected.

-----Original Message-----
From: messages-bounces@vassalengine.org
[mailto:messages-bounces@vassalengine.org] On Behalf Of Michael Kiefte
Sent: August 21, 2010 4:45 PM
To: messages@vassalengine.org
Subject: Re: [messages] [Developers] Sonar code analysis of VASSAL

I just wanted to shout out here that visibility reduction sucks. You have
know idea what a future developer wants to override until they’re faced with
a nasty private declaration (or, God forbid, final class declaration
modifier). In a program like VASSAL which permits custom classes in modules,
visibility reduction is the equivalent of putting a closed sign on the only
gas station for 800km.

  • M

On 2010-08-17, at 10:48 AM, fil512 ken.stevens@sympatico.ca wrote:

You can view a code analysis of the VASSAL engine here:

strategicinitiative.org:9000 … SAL:VASSAL[1]

Cycles

The biggest thing that jumps out at me is the 23% package tangle index
with 615 cycles. Most of you probably know what dependency cycles
are, but in case the concept is new to some of you, I’ll describe what
they are and how to fix them. A system that is properly layered has 0
cycles. Code as tangled as VASSAL is is much harder to write tests for.
If I were to tackle cleaning up the VASSAL code, this is where I’d
start: introduce interfaces and reorganize packages to get the
dependency cycles down to 0. It’s not that hard; the main thing you
need to think about is how you want your code to be layered. Lower
layers should know nothing about layers above them that use them (i.e.
no imports from higher layers). There’s a jdepend plugin for Eclipse
that can help make this work easier.

Duplication
The next one I’d probably look at is code duplication. I see two
practically identical ConcurrentSoftHashMap classes–that just looks
like an error. The code duplication in HexGrid and SquareGrid look
like they just need refactoring. Similarly for a bunch of other code
duplication Sonar found…

Complexity

This one can be a bit religious. But the recommended complexity for a
class is 4 or 5. An average of 20.2 is way too high–it suggests that
there are many classes that have more than one responsibility. This
problem also has a big impact on unit testing–it’s harder to test a
big gangly class that does too many different things.

Code coverage

The number to aim for is 75%. 1.4% is, err, a serious problem.
Having no test coverage is particularly problematic in a project like
VASSAL where it’s an engine that other people write code (modules) to
run on it. How will you know if your code change will break any
modules out there if you don’t have test coverage?

Rule compliance

This one is lower down on the priority list for me compared to the
others. It’s a good thing to keep an eye on as you go along. Code
with no Major/Critical violations is WAY easier to maintain than code
that has those violations. Some of the rules are debatable, and
stylistic in nature. Some of the critical ones are genuinely scary
like storing arrays, equals hash, suspicious equals–those are
probably bugs. Visibility modifiers may look like it’s a style thing,
but reducing visibility has some subtle but valuable benefits–e.g. it
allows the compiler to detect whether anybody actually uses that
method/field. Over time, unused methods/fields accumulate. Reducing
their visibility makes the job of cleaning them up easier. But like I
said, I’d put rules compliance way down the list compared to the bigger
problem of layering, big classes, and test coverage.

Ken

P.S. Note that Sonar requires Maven–Sonar is one of the reasons I
moved VASSAL into Maven.

[1]
strategicinitiative.org:9000 … SAL:VASSAL


Read this topic online here:
Sonar code analysis of VASSAL


messages mailing list
messages@vassalengine.org
vassalengine.org/mailman/listinfo/messages


messages mailing list
messages@vassalengine.org
vassalengine.org/mailman/listinfo/messages


messages mailing list
messages@vassalengine.org
vassalengine.org/mailman/listinfo/messages

Did any of you click on the link I posted and have a look at the analysis?

I posted three primary concerns: Cycles, Duplication and Complexity, and I feel like the only thing we’ve been talking about on this thread is the stuff I explicitly said in my post was stylistic and unimportant.

What about a serious effort to fix the cycles, duplication and complexity?

Related to this, how about introducing a formalized API (all interfaces) that people who write VASSAL extensions (which I assume are custom traits?) code to? Inviting VASSAL extension writers to use anything and everything in the VASSAL source code will really limit our ability to improve the VASSAL engine down the road.

-K

Thus spake fil512:

Did any of you click on the link I posted and have a look at the
analysis?

I did, but the @!#$% web site is sucking up all of my dev time right
now. (Did I mention that I hate doing web stuff?)

I posted three primary concerns: Cycles, Duplication and Complexity, and
I feel like the only thing we’ve been talking about on this thread is
the stuff I explicitly said in my post was stylistic and unimportant.

What about a serious effort to fix the cycles, duplication and
complexity?

I did reply about the duplication: The most serious cases will be fixed as
side effects of (1) removing deprecated code, and (2) carying out the model-
view project.

There are a lot of methods which should be chopped into smaller pieces, I
agree.

I’m not sure at all that the cycles are avoidable or even matter. For
example, lots of clases depend on Map because they represent things which
can be on a map, and vice versa. Making all of these things interfaces
won’t change the fact these typess are interdependent.

Related to this, how about introducing a formalized API (all interfaces)
that people who write VASSAL extensions (which I assume are custom
traits?) code to? Inviting VASSAL extension writers to use anything and
everything in the VASSAL source code will really limit our ability to
improve the VASSAL engine down the road.

Yes, I think we need to make more use of interfaces. It’s not clear to
me what things should be part of an official API, however.


J.

Cycles are avoidable and they do matter. Code is more maintainable when it has no cycles. The less a given class knows about how other packages work, the less that can go wrong. Layering also gives you a lot of flexibility because you can improve higher layers without needing to worry about the impact of those improvements will have on lower layers. Over time, your lower layers become a stable foundation, and your higher layers become more responsive to implementing feature requests.

To speak to your specific example, a lot of classes depend on a Map. The form of that dependency will hopefully be exclusively method calls. To start with, you could make a list of all the methods called on Map that are used by lower layers, and create an interface called Map that holds those methods. The Map class could then be renamed MapImpl and implement that Map interface. The Map interface would be defined at the lower level, the MapImpl would be at the higher level.

Later on, if we notice that the Map interface methods are clustered–one collection is used by one layer in one way, and another collection is used by another layer in another way, then we could split that interface into two interfaces, so that a given layer wouldn’t need to be burdened with knowledge of Map behaviour it is not concerned with.

In general, this is how you use interfaces to break cycles. You create an interface that includes all the methods used by the lower layer, you stick that interface in the lower layer, and then you have the class in the higher layer implement that interface. Bingo, no more cycle.

I would start with a survey of the extensions that are out there. Brent made a comment about custom extensions that really stuck with me, and I agree with. Namely, given the choice between cleaning up the VASSAL code and backwards-compatibility support for extensions, cleaning up the VASSAL code is more important. Old extensions can use old versions of VASSAL. New extensions can code to an API that we’re committed to supporting going forward.

What I’d propose exposing is something like the Java HTTPServlet API. It’s a really well defined API and people have written millions of extensions to it. But it was cleverly designed in such a way that servlet engines retained a lot of flexibility to improve the servlet engine while maintaining this well defined API wall protecting forwards compatibility of servlets as the servlet engines evolved over time.

-K

Thus spake fil512:

Cycles are avoidable and they do matter. Code is more maintainable when
it has no cycles. The less a given class knows about how other packages
work, the less that can go wrong. Layering also gives you a lot of
flexibility because you can improve higher layers without needing to
worry about the impact of those improvements will have on lower layers.
Over time, your lower layers become a stable foundation, and your higher
layers become more responsive to implementing feature requests.

To speak to your specific example, a lot of classes depend on a Map.
The form of that dependency will hopefully be exclusively method calls.
To start with, you could make a list of all the methods called on Map
that are used by lower layers, and create an interface called Map that
holds those methods. The Map class could then be renamed MapImpl and
implement that Map interface. The Map interface would be defined at the
lower level, the MapImpl would be at the higher level.

I agree with you about the details here, but for other reasons. I think
doing all this is a good idea. I just don’t want to become obsessed with
the metrics themselves.


J.

What about a serious effort to fix the cycles, duplication and
complexity?

It’s a matter of time. Sure, as a long term target these are admirable goals. But in the short term, we need to get a working version 3.2 out there without refactoring large amounts of working code at the tail end of the release cycle.

BTW. The average complexity rating is bumped up significantly by the generated code in the Beanshell source and in the wizard source we modify. We can’t control those directly.