what is ADC2Module?

Development
1

It’s a 1500 line class located here:

VASSAL.tools.imports.adc2.ADC2Module

Is it a part of the VASSAL engine? What does it do?

Ken

2

Thus spake fil512:

It’s a 1500 line class located here:

VASSAL.tools.imports.adc2.ADC2Module

Is it a part of the VASSAL engine? What does it do?

Ken

Everything in the adc2 package is for importing Aide de Camp 2 modules
into VASSAL. This is all Michael’s code.

–
J.

3

This class contains the code to load ADC2 .ops files into VASSAL. It’s not
part of the VASSAL engine, but it does take advantage of the engine code to
generate the appropriate internal structures.

  • M.

On 29 August 2010 23:34, fil512 ken.stevens@sympatico.ca wrote:

It’s a 1500 line class located here:

VASSAL.tools.imports.adc2.ADC2Module

Is it a part of the VASSAL engine? What does it do?

Ken

This class contains the code to load ADC2 .ops files into VASSAL.� It’s not part of the VASSAL engine, but it does take advantage of the engine code to generate the appropriate internal structures.

- M.


On 29 August 2010 23:34, fil512 <ken.stevens@sympatico.ca> wrote:

It’s a 1500 line class located here:



VASSAL.tools.imports.adc2.ADC2Module



Is it a part of the VASSAL engine? �What does it do?



Ken

4

Thus spake Michael Kiefte:

This class contains the code to load ADC2 .ops files into VASSAL. It’s not
part of the VASSAL engine, but it does take advantage of the engine code to
generate the appropriate internal structures.

I think it’s not accurate to say that it’s not part of VASSAL. It’s true
that it’s not something you’ll run in the normal course of using VASSAL,
but it’s not a standalone program, either—you run the importer from the
Module Manager.

–
J.

5

Is there a reason it’s all in one class, and not in a package of its own as
a bunch of classes?

-----Original Message-----
From: messages-bounces@vassalengine.org
[mailto:messages-bounces@vassalengine.org] On Behalf Of Joel Uckelman
Sent: August 30, 2010 5:37 AM
To: messages@vassalengine.org
Subject: Re: [messages] [Developers] what is ADC2Module?

Thus spake Michael Kiefte:

This class contains the code to load ADC2 .ops files into VASSAL.
It’s not part of the VASSAL engine, but it does take
advantage of the
engine code to generate the appropriate internal structures.

I think it’s not accurate to say that it’s not part of
VASSAL. It’s true that it’s not something you’ll run in the
normal course of using VASSAL, but it’s not a standalone
program, either—you run the importer from the Module Manager.

–
J.

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

6

On 30 August 2010 11:27, Ken Stevens ken.stevens@sympatico.ca wrote:

Is there a reason it’s all in one class, and not in a package of its own as
a bunch of classes?

Lack of forethought.

I wrote it as I was reverse engineering the ADC2 file format. It’s more of
an evolution than an intelligent design.

There’s no real reason why it all has to be in one file other than that I
thought it was going to be a lot smaller when I started. It’s not clear what
the advantages would be to have it refactered apart. I think I tried that
once and I quickly realised it was going to be a huge amount of work. Even
with Eclipse.

However, now that I think about it, most of those internal classes really
should be internal classes. Almost all of them refer to subparts of the
ADC2 file format that could never possibly ever be used outside of that
class. If they are not private classes already, they probably should be.
If there is one context in which private inner classes are appropriate, this
would be it. I also seem to recall, that many of those private classes
manipulate fields in the outer class. I’m sure this could have been
designed better, but I’m not sure that getters/setters would have been
helpful at all.

  • M.

On 30 August 2010 11:27, Ken Stevens <ken.stevens@sympatico.ca> wrote:


Is there a reason it’s all in one class, and not in a package of its own as

a bunch of classes?

Lack of forethought.

I wrote it as I was reverse engineering the ADC2 file format.� It’s more of an evolution than an intelligent design.�

There’s no real reason why it all has to be in one file other than that I thought it was going to be a lot smaller when I started. It’s not clear what the advantages would be to have it refactered apart.� I think I tried that once and I quickly realised it was going to be a huge amount of work.� Even with Eclipse.


However, now that I think about it, most of those internal classes really should be internal classes.� Almost all of them refer to subparts of the ADC2 file format that could never possibly ever be used outside of that class.� If they are not private classes already, they probably should be.� If there is one context in which private inner classes are appropriate, this would be it.� I also seem to recall, that many of those private classes manipulate fields in the outer class.� I’m sure this could have been designed better, but I’m not sure that getters/setters would have been helpful at all.


- M.


7

There are quite a few advantages to keeping your classes small (~40 lines).
If you’re concerned about visibility, you can always give the classes
package visibility, so they’re not visible outside of your package.

From: messages-bounces@vassalengine.org
[mailto:messages-bounces@vassalengine.org] On Behalf Of Michael Kiefte
Sent: August 30, 2010 10:39 AM
To: messages@vassalengine.org
Subject: Re: [messages] [Developers] what is ADC2Module?

On 30 August 2010 11:27, Ken Stevens ken.stevens@sympatico.ca wrote:

Is there a reason it’s all in one class, and not in a package of its own as
a bunch of classes?

Lack of forethought.

I wrote it as I was reverse engineering the ADC2 file format. It’s more of
an evolution than an intelligent design.

There’s no real reason why it all has to be in one file other than that I
thought it was going to be a lot smaller when I started. It’s not clear what
the advantages would be to have it refactered apart. I think I tried that
once and I quickly realised it was going to be a huge amount of work. Even
with Eclipse.

However, now that I think about it, most of those internal classes really
should be internal classes. Almost all of them refer to subparts of the
ADC2 file format that could never possibly ever be used outside of that
class. If they are not private classes already, they probably should be.
If there is one context in which private inner classes are appropriate, this
would be it. I also seem to recall, that many of those private classes
manipulate fields in the outer class. I’m sure this could have been
designed better, but I’m not sure that getters/setters would have been
helpful at all.

  • M.
There are quite a few advantages to keeping your classes small (~40 lines).  If you're concerned about visibility, you can always give the classes package visibility, so they're not visible outside of your package.

From: [messages-bounces@vassalengine.org](mailto:messages-bounces@vassalengine.org) [mailto:messages-bounces@vassalengine.org] On Behalf Of Michael Kiefte
Sent: August 30, 2010 10:39 AM
To: [messages@vassalengine.org](mailto:messages@vassalengine.org)
Subject: Re: [messages] [Developers] what is ADC2Module?

On 30 August 2010 11:27, Ken Stevens <[ken.stevens@sympatico.ca](mailto:ken.stevens@sympatico.ca)> wrote:
Is there a reason it's all in one class, and not in a package of its own as
a bunch of classes?

Lack of forethought.

I wrote it as I was reverse engineering the ADC2 file format.  It's more of an evolution than an intelligent design. 

There's no real reason why it all has to be in one file other than that I thought it was going to be a lot smaller when I started. It's not clear what the advantages would be to have it refactered apart.  I think I tried that once and I quickly realised it was going to be a huge amount of work.  Even with Eclipse.

However, now that I think about it, most of those internal classes really should be internal classes.  Almost all of them refer to subparts of the ADC2 file format that could never possibly ever be used outside of that class.  If they are not private classes already, they probably should be.  If there is one context in which private inner classes are appropriate, this would be it.  I also seem to recall, that many of those private classes manipulate fields in the outer class.  I'm sure this could have been designed better, but I'm not sure that getters/setters would have been helpful at all.

- M.


8

Almost all of those inner classes are “one-offs” never to be used again
anywhere else, even elsewhere in the package. I, personally, found it
convenient to have them in the same file as I could completely forget about
them if I was working on something else in the package. They are
essentially one step above anonymous classes.

If you want to have a go at refactoring them, you’re welcome to. But it’s
not going to be easy: a lot of them access outer-class fields directly. I
think I’ve tried to document what’s going on to a certain extent. I’ll even
answer questions!

I don’t personally see an advantage in small class files. If your using any
kind of IDE, it won’t matter much.

  • M.

On 30 August 2010 16:31, Ken Stevens ken.stevens@sympatico.ca wrote:

There are quite a few advantages to keeping your classes small (~40
lines). If you’re concerned about visibility, you can always give the
classes package visibility, so they’re not visible outside of your package.

From: messages-bounces@vassalengine.org [mailto:
messages-bounces@vassalengine.org] *On Behalf Of *Michael Kiefte
Sent: August 30, 2010 10:39 AM

To: messages@vassalengine.org
Subject: Re: [messages] [Developers] what is ADC2Module?

On 30 August 2010 11:27, Ken Stevens ken.stevens@sympatico.ca wrote:

Is there a reason it’s all in one class, and not in a package of its own
as
a bunch of classes?

Lack of forethought.

I wrote it as I was reverse engineering the ADC2 file format. It’s more of
an evolution than an intelligent design.

There’s no real reason why it all has to be in one file other than that I
thought it was going to be a lot smaller when I started. It’s not clear what
the advantages would be to have it refactered apart. I think I tried that
once and I quickly realised it was going to be a huge amount of work. Even
with Eclipse.

However, now that I think about it, most of those internal classes really
should be internal classes. Almost all of them refer to subparts of the
ADC2 file format that could never possibly ever be used outside of that
class. If they are not private classes already, they probably should be.
If there is one context in which private inner classes are appropriate, this
would be it. I also seem to recall, that many of those private classes
manipulate fields in the outer class. I’m sure this could have been
designed better, but I’m not sure that getters/setters would have been
helpful at all.

  • M.

Almost all of those inner classes are “one-offs” never to be used again anywhere else, even elsewhere in the package.� I, personally, found it convenient to have them in the same file as I could completely forget about them if I was working on something else in the package.� They are essentially one step above anonymous classes.


If you want to have a go at refactoring them, you’re welcome to.� But it’s not going to be easy: a lot of them access outer-class fields directly.� I think I’ve tried to document what’s going on to a certain extent.� I’ll even answer questions!


I don’t personally see an advantage in small class files.� If your using any kind of IDE, it won’t matter much.

- M.

On 30 August 2010 16:31, Ken Stevens <ken.stevens@sympatico.ca> wrote:

There are quite a few advantages to keeping your classes small (~40 lines).� If you're concerned about visibility, you can always give the classes package visibility, so they're not visible outside of your package.

On 30 August 2010 11:27, Ken Stevens <[ken.stevens@sympatico.ca](mailto:ken.stevens@sympatico.ca)> wrote:
Is there a reason it's all in one class, and not in a package of its own as
a bunch of classes?

Lack of forethought.

I wrote it as I was reverse engineering the ADC2 file format.� It's more of an evolution than an intelligent design.�

There's no real reason why it all has to be in one file other than that I thought it was going to be a lot smaller when I started. It's not clear what the advantages would be to have it refactered apart.� I think I tried that once and I quickly realised it was going to be a huge amount of work.� Even with Eclipse.

However, now that I think about it, most of those internal classes really should be internal classes.� Almost all of them refer to subparts of the ADC2 file format that could never possibly ever be used outside of that class.� If they are not private classes already, they probably should be.� If there is one context in which private inner classes are appropriate, this would be it.� I also seem to recall, that many of those private classes manipulate fields in the outer class.� I'm sure this could have been designed better, but I'm not sure that getters/setters would have been helpful at all.

- M.


9

Thus spake Michael Kiefte:

Almost all of those inner classes are “one-offs” never to be used again
anywhere else, even elsewhere in the package. I, personally, found it
convenient to have them in the same file as I could completely forget about
them if I was working on something else in the package. They are
essentially one step above anonymous classes.

If you want to have a go at refactoring them, you’re welcome to. But it’s
not going to be easy: a lot of them access outer-class fields directly. I
think I’ve tried to document what’s going on to a certain extent. I’ll even
answer questions!

I don’t personally see an advantage in small class files. If your using any
kind of IDE, it won’t matter much.

  • M.

This reminds me of something: It’s to bad we don’t have unit tests for
the ADC2 importer. It seems like this is one of those places where unit
tests (or acceptance tests, at least) would do a lot of good.

–
J.

10

Personally, I would vote against large scale refactoring of the code right now, I’m having enough trouble finding the time to get the core functionality finished off. Let’s not try to fix too many things that aren’t broken at the moment. Let’s concentrate on getting 3.2 polished up and out the door and look at refactoring in 3.3.

In any event, I would like to see a plan of what you think needs to be done so we can do some impact analayis, rather than just jumping in here and there as you find things you don’t like. Let’s build up a list of classes you think need attention and why. The ADC2 module importer is a very small aspect of Vassal, is not broken and does not really need resources spent on it at the moment when the trunk isn’t even running properly at the moment!

Ken, Please don’t make a huge amount of changes and then expect Joel or myself to merge them all in to the trunk in one go, the chances are it just won’t get done in the end. Give us bite sized chunks as you work along that are related to a small number of bugs or issues.

Regards,
B.

11

Agreed. VASSAL is pretty big. I am trying to understand it. Asking questions like “Is ADC2Module a part of the VASSAL engine? What does it do?, Is there a reason it’s all in one class?” is me trying to learn about VASSAL. This class stood out because it’s so much larger than the rest of the classes.

At the moment, my only plan is to improve our unit test coverage. I have been writing tests and slowly increasing our unit test coverage. Unit testing will occasionally require changes to the source code–e.g. I needed to add hashCode() and equals() to a bunch of classes so I could test serialization. Also code in static {} blocks is untestable, so I asked about moving code like that out into an initialization method so it can be tested. (I sent you a note about Resources.java static initialization not sure if you’ve had a chance to read it yet–Joel and I discussed it earlier today and I think came to a consensus that my initial suggestion of lazy initialization would have too high a performance hit–we concluded that static singleton instance instantiation with no side-effects and then requiring explicit initialization at startup time would be preferable).

Agreed. As I mentioned above, all I’m doing right now is writing tests. If writing a test requires a code change, then if it’s minor (like adding hashCode() and equals()) I just go ahead and do it, and if it’s more significant (like making Resources a singleton) I discuss it with you and Joel.

In the course of writing tests, I will be asking questions like “why is it done this way?” Often times there’s a reason things are done a particular way.

Ken

12

Small class files are easier to unit test.

13

Maybe I don’t understand what unit test means.

What I had thought of doing was establishing a battery of freely available ADC2 modules as part of a test. However, in a lot of cases, a correct conversion is subjective and difficult to quantify. For example, there’s a whole class devoted to generating maps from ADC2 map files. How do you test whether the conversion was accurate?

  • M

On 2010-08-30, at 8:47 PM, fil512 ken.stevens@sympatico.ca wrote:

“mkiefte” wrote:

I don’t personally see an advantage in small class files.

Small class files are easier to unit test.

Read this topic online here:
what is ADC2Module? - #12 by fil512

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

14

DISCLAIMER AND INDEMNIFICATION: This post is for informational purposes only. The reader of this post agrees to indemnify and hold harmless the author of this post should any irresponsible code changes arise from the execution of recommendations contained within this post. :slight_smile:

What you’re describing is what I would call an “Integration Test” or a “System Test”.

A single unit test would typically only test a small handful of related methods. For example, if I were unit testing ADC2Module.ForcePoolIterator, I would instantiate a Pool object with a list of pieces and then test that hasNext(), next() and remove() all behave as expected. To be thorough, I’d test a few different edge cases: an empty list, a list with an unusable piece in the middle, at the start, at the end etc. You could use a code coverage tool like sonar to verify that your unit tests cover all possible paths through your code. (Code coverage tools run your unit tests and then paint lines of your code red and green, indicating whether or not those lines were exercised by your unit tests. E.g. you can see the effect of the serialization tests I wrote on the code coverage here: http://www.strategicinitiative.org:9000/drilldown/measures/1955?highlight=line_coverage&metric=uncovered_lines&rids[]=2459#. Pick a Decorator that starts with a letter between A to H…I’ve only gotten up to the Hideable trait so far…)

The advantage of unit tests over integration/system tests is that they cover more paths through your code. When you test the code as a whole system, there will be all sorts of edge cases that don’t get tested–and you won’t learn about bugs in those edge cases until a bug appears in released code.

Does that help answer your question?

Ken
P.S. Given how ForcePoolIterator is a private inner class right now, I’d have trouble unit testing it. I would recommend moving it out to its own file with “package” visibility so its behaviour can be unit tested, and nothing outside of adc2 sees it. But per Brent’s note earlier, testing ADC2Module is not a high priority right now.

15

Thanks for explaining this. It sounds like a very useful tool which I will
have to investigate at some point.

However, I agree that the importer is not high priority right now and here’s
the reason why: not many people are making new ADC2 modules so there are
therefore a finite number of modules for which the importer should work. In
theory, if the importer worked for all of them, then you would never have to
touch the code again. There are a few remaining bugs that are difficult to
solve, but, eventually, I suspect ADC will die off completely at which point
the importer is only of historical interest.

  • M.

What you’re describing is what I would call an “Integration Test” or a
“System Test”.

A single unit test would typically only test a small handful of related
methods. For example, if I were unit testing
ADC2Module.ForcePoolIterator, I would instantiate a Pool object with a
list of pieces and then test that hasNext(), next() and remove() all
behave as expected. To be thorough, I’d test a few different edge
cases: an empty list, a list with an unusable piece in the middle, at
the start, at the end etc. You could use a code coverage tool like
sonar to verify that your unit tests cover all possible paths through
your code. (Code coverage tools run your unit tests and then paint
lines of your code red and green, indicating whether or not those lines
were exercised by your unit tests. E.g. you can see the effect of the
serialization tests I wrote on the code coverage here:
strategicinitiative.org:9000 … lines&rids[1]=2459#. Pick
a Decorator that starts with a letter between A to H…I’ve only gotten
up to the Hideable trait so far…)

The advantage of unit tests over integration/system tests is that they
cover more paths through your code. When you test the code as a whole
system, there will be all sorts of edge cases that don’t get tested–and
you won’t learn about bugs in those edge cases until a bug appears in
released code.

Does that help answer your question?

Ken
P.S. Given how ForcePoolIterator is a private inner class right now,
I’d have trouble unit testing it. I would recommend moving it out to
its own file with “package” visibility so its behaviour can be unit
tested, and nothing outside of adc2 sees it. But per Brent’s note
earlier, testing ADC2Module is not a high priority right now.

[1]
strategicinitiative.org:9000 … lines&rids

Thanks for explaining this.� It sounds like a very useful tool which I will have to investigate at some point.

However, I agree that the importer is not high priority right now and here’s the reason why: not many people are making new ADC2 modules so there are therefore a finite number of modules for which the importer should work. In theory, if the importer worked for all of them, then you would never have to touch the code again.� There are a few remaining bugs that are difficult to solve, but, eventually, I suspect ADC will die off completely at which point the importer is only of historical interest.


- M.



What you're describing is what I would call an "Integration Test" or a
"System Test".

A single unit test would typically only test a small handful of related
methods. �For example, if I were unit testing
ADC2Module.ForcePoolIterator, I would instantiate a Pool object with a
list of pieces and then test that hasNext(), next() and remove() all
behave as expected. �To be thorough, I'd test a few different edge
cases: an empty list, a list with an unusable piece in the middle, at
the start, at the end etc. �You could use a code coverage tool like
sonar to verify that your unit tests cover all possible paths through
your code. �(Code coverage tools run your unit tests and then paint
lines of your code red and green, indicating whether or not those lines
were exercised by your unit tests. �E.g. you can see the effect of the
serialization tests I wrote on the code coverage here: [strategicinitiative.org:9000](http://www.strategicinitiative.org:9000) ... lines&rids[1][]=2459#. �Pick
a Decorator that starts with a letter between A to H...I've only gotten
up to the Hideable trait so far...)

The advantage of unit tests over integration/system tests is that they
cover more paths through your code. �When you test the code as a whole
system, there will be all sorts of edge cases that don't get tested--and
you won't learn about bugs in those edge cases until a bug appears in
released code.

Does that help answer your question?

Ken
P.S. �Given how ForcePoolIterator is a private inner class right now,
I'd have trouble unit testing it. �I would recommend moving it out to
its own file with "package" visibility so its behaviour can be unit
tested, and nothing outside of adc2 sees it. �But per Brent's note
earlier, testing ADC2Module is not a high priority right now.

[1] [strategicinitiative.org:9000 ... lines&rids](http://www.strategicinitiative.org:9000/drilldown/measures/1955?highlight=line_coverage&metric=uncovered_lines&rids)

16

Ken
P.S. Given how ForcePoolIterator is a private inner class right now,
I’d have trouble unit testing it. I would recommend moving it out to
its own file with “package” visibility so its behaviour can be unit
tested, and nothing outside of adc2 sees it. But per Brent’s note
earlier, testing ADC2Module is not a high priority right now.

Things like that probably don’t really need to be tested. I imagine that
unit testing is really for public API stuff.

  • M.





Ken

P.S. �Given how ForcePoolIterator is a private inner class right now,

I’d have trouble unit testing it. �I would recommend moving it out to

its own file with “package” visibility so its behaviour can be unit

tested, and nothing outside of adc2 sees it. �But per Brent’s note

earlier, testing ADC2Module is not a high priority right now.



Things like that probably don’t really need to be tested. �I imagine that unit testing is really for public API stuff.

- M.
�

17

Thus spake Michael Kiefte:

Things like that probably don’t really need to be tested. I imagine that
unit testing is really for public API stuff.

There is some sense to testing internal stuff as well; my usual saw is that
you should test it if it needs to work. :slight_smile:

But I agree with you that there are things which are higher-priority than
writing tests for the ADC2 code right now.

–
J.