Vassal code maintainability

Hey guys, I’m new to the Vassal coding world and have a bunch of typical newbie questions.

  1. What’s with all the nio/zip source code? It looks like it has nothing to do with Vassal. Shouldn’t that be a jar dependency instead of source code?

  2. Unit tests anyone? I see a huge whack of unit tests under nio, but I assume those aren’t authored by Vassal coders… Without unit test coverage, changing the Vassal engine code is like flying a trapeze without a net. How will you know if your change breaks any existing modules?

  3. Layering. Why are there awt imports on model classes like TriggerAction and GamePiece. Has anyone ever made an attempt to bring some Model-View-Controller discipline to the Vassal code base? I think this mixing of model and view is part of the reason we have strange things like keystrokes being the way you call a function in Vassal, which is the weirdest thing I’ve seen in a while!

  4. Visitor pattern. A bunch of the confusion I’ve seen about how traits are resolved stems from mixing visting algorithms with the structures that those visiting algorithms operate on. I.e. I don’t think traits should be responsible for doing things to other traits. Traits should just change state and send and receive events. Another layer on top of traits should then be responsible for the order traits are executed, and for managing the event bus. This would get rid of all this confusing “inner” and “outer” stuff. There’d just be lists of things, and rules for how you iterate over those lists. Way easier to understand.

Those are four things off the top of my head that I feel would help improve the maintainability of the code and reduce the chance of code changes introducing bugs in the future. Has anyone else considered introducing disciplines like these to the Vassal code base?

Ken

The road map should answer a lot of stuff for you. What the NIO stuff is for

  • other code your looking at but isn’t in use yet etc…

You can view that here
vassalengine.org/wiki/Roadmap

Joel and Mike can answer in more specifics

-----Original Message-----
From: messages-bounces@vassalengine.org
[mailto:messages-bounces@vassalengine.org] On Behalf Of fil512
Sent: Thursday, July 29, 2010 10:33 PM
To: messages@vassalengine.org
Subject: [messages] [Developers] Vassal code maintainability

Hey guys, I’m new to the Vassal coding world and have a bunch of typical
newbie questions.

  1. What’s with all the nio/zip source code? It looks like it has
    nothing to do with Vassal. Shouldn’t that be a jar dependency instead
    of source code?

  2. Unit tests anyone? I see a huge whack of unit tests under nio, but I
    assume those aren’t authored by Vassal coders… Without unit test
    coverage, changing the Vassal engine code is like flying a trapeze
    without a net. How will you know if your change breaks any existing
    modules?

  3. Layering. Why are there awt imports on model classes like
    TriggerAction and GamePiece. Has anyone ever made an attempt to bring
    some Model-View-Controller discipline to the Vassal code base? I think
    this mixing of model and view is part of the reason we have strange
    things like keystrokes being the way you call a function in Vassal,
    which is the weirdest thing I’ve seen in a while!

  4. Visitor pattern. A bunch of the confusion I’ve seen about how traits
    are resolved stems from mixing visting algorithms with the structures
    that those visiting algorithms operate on. I.e. I don’t think traits
    should be responsible for doing things to other traits. Traits should
    just change state and send and receive events. Another layer on top of
    traits should then be responsible for the order traits are executed, and
    for managing the event bus. This would get rid of all this confusing
    “inner” and “outer” stuff. There’d just be lists of things, and rules
    for how you iterate over those lists. Way easier to understand.

Those are four things off the top of my head that I feel would help
improve the maintainability of the code and reduce the chance of code
changes introducing bugs in the future. Has anyone else considered
introducing disciplines like these to the Vassal code base?

Ken


Read this topic online here:
https://forum.vassalengine.org/t/vassal-code-maintainability/3141/1

Cool! Nice to see layering is already on the list. Even if nio is in beta, it should still be possible to include it as a jar instead of as source no? Didn’t see unit tests on the list. I don’t know how you guys manage to avoid breaking modules without unit tests… And I didn’t see the visitor pattern stuff on the list either.

Here’s another one:

  1. Any thought of moving to Maven as the build engine? With Maven you get a bunch of cool stuff for free like Sonar. Mmmmmm Sonar… Sonar would help dramatically with any layering initiative. You could use JDepend, but the Sonar UI is so much nicer.
  1. Unit tests anyone? I see a huge whack of unit tests under nio, but I
    assume those aren’t authored by Vassal coders… Without unit test
    coverage, changing the Vassal engine code is like flying a trapeze
    without a net. How will you know if your change breaks any existing
    modules?

JUnit would be a great idea.

  1. Layering. Why are there awt imports on model classes like
    TriggerAction and GamePiece. Has anyone ever made an attempt to bring
    some Model-View-Controller discipline to the Vassal code base? I think
    this mixing of model and view is part of the reason we have strange
    things like keystrokes being the way you call a function in Vassal,
    which is the weirdest thing I’ve seen in a while!

That’s something else we have to do.

  1. Visitor pattern. A bunch of the confusion I’ve seen about how traits
    are resolved stems from mixing visting algorithms with the structures
    that those visiting algorithms operate on. I.e. I don’t think traits
    should be responsible for doing things to other traits.

I understand what you’re saying, but it doesn’t bother me nearly as much.
It’s a simple linked list. And traits like Obscurable have regulate
keystrokes.

Traits should
just change state and send and receive events. Another layer on top of
traits should then be responsible for the order traits are executed, and
for managing the event bus. This would get rid of all this confusing
“inner” and “outer” stuff.

That’s really part of the design philosophy and not something that’s easily
changed. I suspect traits were thought of like visible layers. Whatever is
underneath can be obscured by something above it. It’s not really as weird
as you make out. My only issue has ever been the behaviour of
TriggerAction.

There’d just be lists of things, and rules
for how you iterate over those lists. Way easier to understand.

Those are four things off the top of my head that I feel would help
improve the maintainability of the code and reduce the chance of code
changes introducing bugs in the future. Has anyone else considered
introducing disciplines like these to the Vassal code base?

What you’re talking about is an overhaul. It sounds great. But if you
don’t want to see existing modules break, then it’s going to take an order
of magnitude more work than you imagine.

I think JUnit tests would be the first order of business…

  • M.





2. Unit tests anyone? �I see a huge whack of unit tests under nio, but I

assume those aren’t authored by Vassal coders… �Without unit test

coverage, changing the Vassal engine code is like flying a trapeze

without a net. �How will you know if your change breaks any existing

modules?

JUnit would be a great idea.




3. Layering. �Why are there awt imports on model classes like

TriggerAction and GamePiece. �Has anyone ever made an attempt to bring

some Model-View-Controller discipline to the Vassal code base? �I think

this mixing of model and view is part of the reason we have strange

things like keystrokes being the way you call a function in Vassal,

which is the weirdest thing I’ve seen in a while!

That’s something else we have to do.


4. Visitor pattern. �A bunch of the confusion I've seen about how traits
are resolved stems from mixing visting algorithms with the structures
that those visiting algorithms operate on. �I.e. I don't think traits
should be responsible for doing things to other traits. �

I understand what you're saying, but it doesn't bother me nearly as much.� It's a simple linked list.� And traits like Obscurable have regulate keystrokes.
Traits should
just change state and send and receive events. �Another layer on top of
traits should then be responsible for the order traits are executed, and
for managing the event bus. �This would get rid of all this confusing
"inner" and "outer" stuff. �

That's really part of the design philosophy and not something that's easily changed.� I suspect traits were thought of like visible layers.� Whatever is underneath can be obscured by something above it.� It's not really as weird as you make out.� My only issue has ever been the behaviour of TriggerAction.
There'd just be lists of things, and rules
for how you iterate over those lists. �Way easier to understand.

Those are four things off the top of my head that I feel would help
improve the maintainability of the code and reduce the chance of code
changes introducing bugs in the future. �Has anyone else considered
introducing disciplines like these to the Vassal code base?

What you're talking about is an overhaul.� It sounds great.� But if you don't want to see existing modules break, then it's going to take an order of magnitude more work than you imagine.

I think JUnit tests would be the first order of business....

- M.

Thus spake fil512:

Hey guys, I’m new to the Vassal coding world and have a bunch of typical
newbie questions.

  1. What’s with all the nio/zip source code? It looks like it has
    nothing to do with Vassal. Shouldn’t that be a jar dependency instead
    of source code?

No, I wrote almost all of that becuase there’s no backport of Java 7’s ZipFS
for Java 6. But presently we’re not using any of in fact, I’ve had so much
difficulty with it that I’m in the middle of backing out all of it.

  1. Unit tests anyone? I see a huge whack of unit tests under nio, but I
    assume those aren’t authored by Vassal coders… Without unit test
    coverage, changing the Vassal engine code is like flying a trapeze
    without a net. How will you know if your change breaks any existing
    modules?

No, I wrote almost all of those tests. I completely agree with you about
making modifications when we have very little test coverage. Most of the
code was written before anyone was thinking about writing tests, and a
good chunk of the code is just untestable. When I get back to my Model-
View project, that will involve ripping out a huge amount of old code,
at which point I intend to write tests for the new code.

If you want to write tests, please do. We would gladly accept them.

  1. Layering. Why are there awt imports on model classes like
    TriggerAction and GamePiece. Has anyone ever made an attempt to bring
    some Model-View-Controller discipline to the Vassal code base? I think
    this mixing of model and view is part of the reason we have strange
    things like keystrokes being the way you call a function in Vassal,
    which is the weirdest thing I’ve seen in a while!

See my comment above. The way these things are mixed together right now
is a catastrophe. This is something I’ve wanted to fix almost since I
started working on VASSAL. If you look at our Development Roadmap,
you can see this stuff under 3.3:

vassalengine.org/wiki/Roadmap

  1. Visitor pattern. A bunch of the confusion I’ve seen about how traits
    are resolved stems from mixing visting algorithms with the structures
    that those visiting algorithms operate on. I.e. I don’t think traits
    should be responsible for doing things to other traits. Traits should
    just change state and send and receive events. Another layer on top of
    traits should then be responsible for the order traits are executed, and
    for managing the event bus. This would get rid of all this confusing
    “inner” and “outer” stuff. There’d just be lists of things, and rules
    for how you iterate over those lists. Way easier to understand.

The “inner” and “outer” come from the decorator pattern. Each trait is
a decorator around the one which is inner with respect to it. What you’re
proposing would be a fundamental change to how traits work.

Those are four things off the top of my head that I feel would help
improve the maintainability of the code and reduce the chance of code
changes introducing bugs in the future. Has anyone else considered
introducing disciplines like these to the Vassal code base?

Regarding #2 and #3, I’m in complete agreement with you, and have been
for some years now. I would love to have more development help in order
to make these happen. (I would also love to have someone to hand over
the website to, so that I have time to do any development at all again.)


J.

Thus spake fil512:

Cool! Nice to see layering is already on the list. Even if nio is in
beta, it should still be possible to include it as a jar instead of as
source no?

The NIO stuff is actually our code. I don’t see the advantage to having
it in a seperate JAR. (BTW, this tells me that you’re looking that the
trunk, which is in a completely broken state right now, and also contains
a lot of changes from 3.1. If you want to see what you’re actually
running, look at the 3.1 brahcn.)

Didn’t see unit tests on the list. I don’t know how you
guys manage to avoid breaking modules without unit tests… And I
didn’t see the visitor pattern stuff on the list either.

We don’t avoid breaking modules without unit tests. We really have no
way of telling whether anything actually works right now. The reason
that unit tests isn’t on the list is that I wasn’t thinking of writing
tests as a separate project—it’s something which should be done as
part of any project where we write new code.

Here’s another one:

  1. Any thought of moving to Maven as the build engine? With Maven you
    get a bunch of cool stuff for free like Sonar. Mmmmmm Sonar… Sonar
    would help dramatically with any layering initiative. You could use
    JDepend, but the Sonar UI is so much nicer.

Maven is based on ant, right? We used to use ant for builds and packaging.
I hated ant, which is why we’re using GNU Make right now.


J.

Good to see it’s being yanked out. It looks like a bunch of code that isn’t concerned with Vassal stuff–looks like something that there should be a jar for.

I have yet to see a piece of code I can’t test. It’s true that with all the UI stuff there it’s harder to test than it should be, but I found that with a few mocks I was able to test everything that needed to be tested. Not sure if you’ve looked at the Vassal module analyzer I wrote, but it’s written completely as JUnit tests and uses mocks to deal with obstacles to unit testing.

It sounds like you agree with most of the points I raised, but this is the one you remain unconvinced of. It’s hard to articulate why I think this is so important and why I don’t think it’s as radical a change as it might sound.

To be clear, what I’m suggesting is to remove keyEvent() from Decorator and instead just have myKeyEvent() on the pieces. Centralizing how key events are issued to traits in a single place will give you two benefits. First it will be clear what the visiting algorithm is (rather than needing to go into every trait to find out) but secondly, and more importantly, it will force the visiting algorithm to be consistent. I’m talking about separating concerns. Have the trait only be concerned with the action it needs to execute. Have the trait visitor be concerned with stuff like what order traits should be visited in under what circumstances, when one trait should block another trait etc. This would not only make existing behaviour more consistent, it would force future behaviour to be consistent too.

Ken

To be clear, what I’m suggesting is to remove keyEvent() from Decorator
and instead just have myKeyEvent() on the pieces. Centralizing how key
events are issued to traits in a single place will give you two
benefits. First it will be clear what the visiting algorithm is (rather
than needing to go into every trait to find out) but secondly, and more
importantly, it will force the visiting algorithm to be consistent. I’m
talking about separating concerns. Have the trait only be concerned
with the action it needs to execute. Have the trait visitor be
concerned with stuff like what order traits should be visited in under
what circumstances, when one trait should block another trait etc. This
would not only make existing behaviour more consistent, it would force
future behaviour to be consistent too.

You’d be giving up a significant amount of power in doing that. Right now,
an outer trait can hijack the behaviour of an inner piece. I agree that,
from the point of view of VASSAL itself, it makes it confusing. However,
from the point of view of someone who makes custom VASSAL classes, it means
you can do some very powerful things. In addition, you have to realise that
there are many modules that you will break because they use custom classes.
While we can’t be responsible for each and every one of those, you will
break a large number if you do what you propose.

The current model is unpleasant, but because it does hold some virtues, it
would be very difficult to overhaul. Even doing something like changing the
way triggers work would be disastrous.

myKeyEvent() does what you say. However, I would be loathe to get rid of
keyEvent(). It’s actually come in handy.

  • M.



To be clear, what I’m suggesting is to remove keyEvent() from Decorator

and instead just have myKeyEvent() on the pieces. �Centralizing how key

events are issued to traits in a single place will give you two

benefits. �First it will be clear what the visiting algorithm is (rather

than needing to go into every trait to find out) but secondly, and more

importantly, it will force the visiting algorithm to be consistent. �I’m

talking about separating concerns. �Have the trait only be concerned

with the action it needs to execute. �Have the trait visitor be

concerned with stuff like what order traits should be visited in under

what circumstances, when one trait should block another trait etc. �This

would not only make existing behaviour more consistent, it would force

future behaviour to be consistent too.


You’d be giving up a significant amount of power in doing that.� Right now, an outer trait can hijack the behaviour of an inner piece.� I agree that, from the point of view of VASSAL itself, it makes it confusing.� However, from the point of view of someone who makes custom VASSAL classes, it means you can do some very powerful things.� In addition, you have to realise that there are many modules that you will break because they use custom classes.� While we can’t be responsible for each and every one of those, you will break a large number if you do what you propose.


The current model is unpleasant, but because it does hold some virtues, it would be very difficult to overhaul.� Even doing something like changing the way triggers work would be disastrous.

myKeyEvent() does what you say.� However, I would be loathe to get rid of keyEvent().� It’s actually come in handy.


- M.

I’m a big believer in source code only being concerned about one thing. So I believe that Vassal source should be concerned with Vassal stuff. If you need a tool to help you with file i/o or zip files then it’s best to find a file i/o or zip tool that does what you need and depend on it as a jar. The jar will have been tested, used by lots of other ppl, in general will be reliable so you don’t need to worry about it and can focus on making Vassal better.

Well given how much legacy code has no test coverage, I think the only way it’s going to get test coverage is to make that a separate project.

Maven has nothing to do with ant. Most if not all of the big open source projects all use Maven now. Apache, JBoss, Google, Eclipse.

make and ant are both scripting languages: do this, and then do that.

Maven is a declarative language–Kind of like VASSAL that way. You tell it “traits” of your project, and then it knows what to do. It would be a considerable effort to move to Maven, but the advantages are many. For starters, you’re now building the way everyone else in the java world is building. But more importantly, Maven guarantees all sorts of integrity to your releases that you wouldn’t otherwise have. And then you get the sweet sauce like Sonar. Given that VASSAL has different builds for different platforms, I wouldn’t enter Maven lightly. Maybe after I’ve made some progress with documentation and unit tests I could take a whack at Mavenizing Vassal.

A short summary of advantages can be found here. maven.apache.org/benefits-of-using-maven.html

But if you’ve got something that works, I recommend you stick with that until someone joins the dev team who knows Maven and then have them make the switch for you. Once you’ve worked in Maven for a while, you’ll never want to go back.

Ken

I didn’t realize that players made their own custom traits. Well you’re stuck with that API then. That’s unfortunate.

There’s a software design theory that says that programming structures should mirror user-interface structures. So if users think of a list of traits as a list, then they should be implemented as a list.

In that other thread, question about delete and order, I think a big part of the reason it took 20 posts to get the answer to, what I felt was a relatively straightforward question, is that there is a disconnect between how the user interacts with traits and how the java code implements them. If dispatching keystrokes to traits were implemented using a visitor pattern, then I bet instead of 20 posts, it would have only taken 2 posts to find the answer to my question, because all you’d need to do is read the answer straight off of the trait walker method. As it stands now, to get the answer, you need to go into each and every trait to see how they interact with each and every other trait. That’s nasty.

But you’re right. If there’s custom code out there that is using this (unintuitive) API, then we’re stuck with it. I know it makes perfect sense to the VASSAL engine coders. But to the average module designer, it’s baffling.

Ken

They don’t. Most custom traits are written by Engine coders for specific module requests by developers/players. In some few cases it is done by others though - the OCS mods come to mind there

It may be nasty, but the way it works is also very powerful and versatile. Most have gotten used to it too and after awhile you know exactly what to look for in most troubleshooting issues and can zero in pretty quick. To keep that level of current abilities modders can utilize would probably require such an overhaul coding wise it would take a couple of years at least of time to get back to the same level.

Thus spake fil512:

No, I wrote almost all of that becuase there’s no backport of Java 7’s
ZipFS
for Java 6. But presently we’re not using any of in fact, I’ve had so
much
difficulty with it that I’m in the middle of backing out all of it.

Good to see it’s being yanked out. It looks like a bunch of code that
isn’t concerned with Vassal stuff–looks like something that there
should be a jar for.

I understand what you’re saying now—not that this code should be
repackaged, but that we should be relying on an external library.
I agree, but that library doesn’t exist. I looked for a ZipFS backport
before sinking a month into writing all of this, but there wasn’t one.
It was all supposed to be only temporary until we can use what will
be part of Java 7. I was not expecting the ZipFS code I inherited to be
so crappy, so in retrospect, I wouldn’t have done it had I known what I
know now.

I have yet to see a piece of code I can’t test. It’s true that with all
the UI stuff there it’s harder to test than it should be, but I found
that with a few mocks I was able to test everything that needed to be
tested. Not sure if you’ve looked at the Vassal module analyzer I
wrote, but it’s written completely as JUnit tests and uses mocks to deal
with obstacles to unit testing.

No, I haven’t had time to look at it yet, still fighting with site stuff.


J.

But you’re right. If there’s custom code out there that is using this
(unintuitive) API, then we’re stuck with it. I know it makes perfect
sense to the VASSAL engine coders. But to the average module designer,
it’s baffling.

The documentation is sparse, unfortunately, and that’s where we need a lot
of work.

  • M.



But you’re right. �If there’s custom code out there that is using this

(unintuitive) API, then we’re stuck with it. �I know it makes perfect

sense to the VASSAL engine coders. �But to the average module designer,

it’s baffling.



The documentation is sparse, unfortunately, and that’s where we need a lot of work. �

- M.�

There are over 1600 tests in packages called

VASSAL.tools.nio.file.realfs
and
VASSAL.tools.nio.file.zipfs

Did you really write almost all of those 1600 tests? That’s a lot of tests. The few I glanced at appear to have nothing to do with VASSAL–they look like they’re concerned with the minutia of reading/writing zip files.

-K

Thus spake fil512:

“uckelman” wrote:

Thus spake fil512:

  1. Unit tests anyone? I see a huge whack of unit tests under nio,
    but I
    assume those aren’t authored by Vassal coders… Without unit test
    coverage, changing the Vassal engine code is like flying a trapeze
    without a net. How will you know if your change breaks any existing
    modules?

No, I wrote almost all of those tests.

There are over 1600 tests in packages called

VASSAL.tools.nio.file.realfs
and
VASSAL.tools.nio.file.zipfs

Did you really write almost all of those 1600 tests? That’s a lot of
tests. The few I glanced at appear to have nothing to do with
VASSAL–they look like they’re concerned with the minutia of
reading/writing zip files.

Yes, I did write all those tests, along with the supporting test harness
classes.

Almost all of those tests are actually for path handling; the ZIP I/O is
still left to the java.util.zip classes.


J.

Wow. That’s a lot of work. And you say it’s not being used and you’re planning on yanking it out?

Ken

Thus spake fil512:

Wow. That’s a lot of work. And you say it’s not being used and you’re
planning on yanking it out?

It’s not being used in the sense that it never started to be used. I
thought that it would be worth backporting the NIO ZipFS classes from
JDK 7, but I changed my mind—it’s a huge API and there’s no benefit
to switching to it when the underlying ZIP handling is so crappy.


J.