Getting rid of the singleton anti-pattern

Development
1

I was thinking about how to get rid of the singleton anti-pattern that is being used throughout the Vassal code. This is a huge hindrance to maintainability, right now it prevents all kinds of proper testing, and the code can not be properly maintained and refactored without first putting it in a test corset.

I would like to know what the other developers think of this, and also what the interested non-developers think about this as it directly concerns the future of “Vassal Java”. And it does need a future as “Vassal 4” or whatever will be the name of it is still at least 5 to 10 years away, given the current availability of development resources.

Applying the old programming wisdom that any problem can be solved by adding another level of indirection, I have come up with the following plan:

  • step 1
    – create one god-singleton, e.g. with the name “VassalInstanceFacade”, have it use a classic singleton pattern with “private static final VassalInstanceFacade instance” and “public static VassalInstanceFacade getInstance()”
    – give this facade non-static methods for accessing all other singletons like GameModule, DragBuffer, and also the static methods from PlayerRoster and Map
    – deprecate the current static accessors of the singletons like GameModule.getGameModule(), DragBuffer.getBuffer() etc, tell the clients to use VassalInstanceFacade.getGameModule(), VassalInstanceFacade.getDragBuffer() instead
    – in the Vassal code, replace calls to GameModule.getGameModule() with VassalInstanceFacade.getInstance().getGameModule()

  • step 2
    – replace calls to VassalInstanceFacade.getInstance() with a reference to an actual instance of VassalInstanceFacade that is passed to the clients from above, via constructor
    – start to write unit tests that pass a mocked VassalInstanceFacade to its clients

  • step 3
    – use an IoC container to inject instances of VassalInstanceFacade into its clients

  • step 4
    – split up the VassalInstanceFacade into its subcomponents, have the IoC container inject separate GameModule, DragBuffer etc objects into their clients

  • step 5
    – refactor the shit out of the current spaghetti code, split up all these god classes into a proper GUI-less model, split the rest into views and controllers, do all of this in a proper test corset

Some of these steps will need to have many small sub-steps, some might be done in parallel, this is only a rough outline. As for development resources, I cannot speak for others but I am willing to spend time on executing this plan. I am not a C++ developer and will have zero involvement with this new “Vassal 4 in C++” anyways.

Feedback is very welcome!

2

Lol - I was just reading this article the other day and it persuaded me to desingletonise:
https://sites.google.com/site/steveyegge2/singleton-considered-stupid
Off-topic, another article by that guy I enjoyed:
http://steve-yegge.blogspot.com/2010/12/haskell-researchers-announce-discovery.html
Still, what you are proposing sounds like a lot of work. Especially when there is already a perfectly effective testing mechanism in place - the vassal user.

3

To elaborate on this, in the world of enterprise Java software, and Vassal with its huge codebase is approaching the size of a big enterprise-grade application, singleton is generally considered an anti-pattern, it contradicts all the modern developing paradigms like inversion of control and software testability. The main testing frameworks like mockito don’t support the mocking of public static methods, and the IoC containers like Spring make the injection of dependencies a very convenient replacement for hardcoded dependencies.

Sadly the educational side is not up to date on this and it’s a constant pain in the ass having to teach junior devs to not use terrible things like public static methods and singletons. I keep seeing many juniors who were actually taught that singletons are a good and convenient thing :open_mouth:

4

Sounds completely reasonable to me except that VassalFacadeInstance while possibly technically accurate in an strict academic sense, is both bulky and probably confusing.

I would name it “Vassal”:

  • Name not actually taken afaik
  • Easiest to type
  • Least bulky to read
  • Completely intuitive place, in context, to be fetching things from
  • Remember “on ramp” for non-system-architects is a significant goal (this should actually be #1 reason)

Brian

5

Thus spake Flint1b:

To elaborate on this, in the world of enterprise Java software, and
Vassal with its huge codebase is approaching the size of a big
enterprise-grade application, singleton is generally considered an
anti-pattern, it contradicts all the modern developing paradigms like
inversion of control and software testability. The main testing
frameworks like mockito don’t support the mocking of public static
methods, and the IoC containers like Spring make the injection of
dependencies a very convenient replacement for hardcoded dependencies.

Some use of singletons in old code can be chalked up to not knowing
better, and that it wasn’t considered bad practice when the code was
written.

Some, though, may be due to memory constraints from when the code was
written. Did you know that the first release of VASL (which is now a
VASSAL module, but is where all our code originated) was about six months
after the first release of Java, in 1996? Adding an extra reference to the
GameModule (or whatever else is a singleton) to each and every class that
needed it might have been the difference between something usable and
something that isn’t usable on the machines people had at the time. (This
is why the XML we write is so horrible. Parsing nicer, human-readable XML
overwhelmed the computers people had in 1996. Seriously.)

–
J.

6

Thus spake Martinov:

Still, what you are proposing sounds like a lot of work. Especially when
there is already a perfectly effective testing mechanism in place - the
vassal user.

The VASSAL user is far, far from perfectly effective as a testing
mechanism. We’re greateful for the bug reports we get, but it’s much,
much better to have tests that fail when you break something, so that
those bugs never reach the user.

–
J.

7

Thus spake Flint1b:

I was thinking about how to get rid of the singleton anti-pattern that
is being used throughout the Vassal code. This is a huge hindrance to
maintainability, right now it prevents all kinds of proper testing, and
the code can not be properly maintained and refactored without first
putting it in a test corset.

That would be a good thing to do, if you wanted to continue on a long time
with the current codebase.

I would like to know what the other developers think of this, and also
what the interested non-developers think about this as it directly
concerns the future of “Vassal Java”. And it does need a future as
“Vassal 4” or whatever will be the name of it is still at least 5 to 10
years away, given the current availability of development resources.

Disagree strongly. Once I’m clear of 3.3.0, I’ll be devoting all my
efforts to V4 work. It’s not “at least 5 to 10 years away” like you say.

Applying the old programming wisdom that any problem can be solved by
adding another level of indirection, I have come up with the following
plan:

  • step 1
    – create one god-singleton, e.g. with the name “VassalInstanceFacade”,
    have it use a classic singleton pattern with “private static final
    VassalInstanceFacade instance” and “public static VassalInstanceFacade
    getInstance()”
    – give this facade non-static methods for accessing all other
    singletons like GameModule, DragBuffer, and also the static methods from
    PlayerRoster and Map
    – deprecate the current static accessors of the singletons like
    GameModule.getGameModule(), DragBuffer.getBuffer() etc, tell the clients
    to use VassalInstanceFacade.getGameModule(),
    VassalInstanceFacade.getDragBuffer() instead
    – in the Vassal code, replace calls to GameModule.getGameModule() with
    VassalInstanceFacade.getInstance().getGameModule()

You’re going to find that you have a very, very hard time getting rid
of any of the functions you deprecate. You will either be stranding
many users with modules that no longer run, or be stuck maintaining the
deprected functions basically forever.

  • step 2
    – replace calls to VassalInstanceFacade.getInstance() with a reference
    to an actual instance of VassalInstanceFacade that is passed to the
    clients from above, via constructor
    – start to write unit tests that pass a mocked VassalInstanceFacade to
    its clients

  • step 3
    – use an IoC container to inject instances of VassalInstanceFacade into
    its clients

  • step 4
    – split up the VassalInstanceFacade into its subcomponents, have the
    IoC container inject separate GameModule, DragBuffer etc objects into
    their clients

  • step 5
    – refactor the shit out of the current spaghetti code, split up all
    these god classes into a proper GUI-less model, split the rest into
    views and controllers, do all of this in a proper test corset

This looks like what I concluded we needed to do 10 years ago, if we had
thought the effort was worth it.

Some of these steps will need to have many small sub-steps, some might
be done in parallel, this is only a rough outline. As for development
resources, I cannot speak for others but I am willing to spend time on
executing this plan. I am not a C++ developer and will have zero
involvement with this new “Vassal 4 in C++” anyways.

I don’t understand why you want to spend the time it will take to do
this (or, alternativley, the time it will take to attempt it and then
conclude that it is a bad idea) on the current code. It is not worth
it. It will take less effort to make something far better from scratch,
using what we’ve learned.

–
J.

8

No criticism about the origin of the singleton pattern was intended, I understand by now, roughly, how all this came about to be. I also remember 1996 and how much better my friends Pentium was than my 486, and how my first CS teacher at school taught us to think of every single byte and every single statement, and how terribly slow Java was in the first decade or so.

Either way, a big old software, even if its a total spaghetti mess, is still very valuable as it contains decades of experience, additions of good features, removal of bad features and of course bug-fixing, it was fleshed out by generally capable people, and with the help of modern development methods, modern tools and their support for refactoring I believe it is well possible to bring this valuable software into a clean and manageable state while keeping the good parts intact. As for the time estimation, I know us developers are generally bad at estimating time but comparing “complete refactoring of vassal to modern standards” to “complete rewrite”, I don’t think that one will take substantially longer than the other. Yes, it might take only a couple years to solve the current issues by rewriting from scratch, to get a good first 1.0 release, but then whole new problems will inevitably appear, frameworks will change, feature sets will change, new bugs will come up, the original concept/model will need substantial changes etc, and all these new problems will have to be solved and then it will take another 5-10 years to get to a point where the new Vassal will be as fleshed out as the current one is.

I realize that the current Vassal will not be able to go much further while keeping backwards compatibility, but if that compatibility gets lost underway anyways, what does it matter whether it gets lost due to a rewrite or to a complete refactoring.

Besides, what is supposed to happen to Vassal anyways, once this new product will get out which will not be Vassal but a completely new thing? After decades of improving the features and removing bugs, it would be a shame to just throw it away, I believe it should have a future as well.

One other point is the choice of language from the point of view of a “business decision”, and the availability of resources coupled with that choice, and I am quite sure that it is going to be easier getting Java developers on board than C++ developers. Thanks to the success of Java in the business/enterprise field there are simply many more Java developers on this planet, and if the official definition for the current Vassal will change from “going to end soon” to “has a future and we need developers that partake and make this future happen”, more Java devs that are willing to help might come along.

9

At that point, I would argue that your Java super Vassal is also no longer Vassal and should get it’s own name variation and be progressed as a separate project.

I absolutely agree and I expect Vassal in it’s current form to continue ever onward. As I have said before, several times, no-one is planning to throw the current Vassal away. I have a personal stake in seeing Vassal 3 continue as I maintain a series of popular modules with a very heavy custom code load that are not being migrated anywhere in a hurry.

However, there are thousands(?) of existing Vassal users and hundreds(?) of module designers with a significant investment in Vassal as it is and who have the very reasonable expectation that there are not going to be a series of earth shattering changes along with all the bugs and incompatibilities that will introduce.

At the end of the day, you will have spent an enormous amount of time and effort to get back to the same point. You will have an easily maintainable piece of software that is fundamentally flawed by design decisions made 25 years ago. And you will have inflicted the users with countless bugs and issues along the way. This is why Joel and I are advocating against this approach.

I do understand. You’re a Java doctor and Vassal is a broken puppy. I’m a full time IT professional, but programming is also one of my favourite hobbies and it is so much fun to work on a meaty project in an area I am actually interested in. I was so excited when I first discovered Vassal. Sad hey :slight_smile:

Here’s an alternative.

The first stage of the new Vassal project is to design a system specification with a formal underpinning that can be implemented in any given language. The reference implementation will be written in C++, but there is no reason at all why you couldn’t build a Java implementation at the same time. It would be great to have you involved in the design stage, even if you don’t take it further. The problems we need to solve are the same as you would need to solve if you want to take the current Vassal in a different direction.

I believe the current Vassal 3 should be ‘stabilised’. It needs to maintain 100% backward compatibility and be kept free from the inevitable issues that will arise from large-scale refactoring of code with a 0% test coverage. I think further development should be limited to bug fixes, cosmetic changes, consistency changes, quality of life improvements and such. There is scope to add additional Beanshell functions to simplify some currently complex tasks. And things like Brian’s fancy Chatter. The documentation needs substantial work to get it up to date and add examples, tips, best practices. Other than that, I think it needs to pretty much stay as it is.

If you decide not to be involved in our new Vassal project and still want to rebuild the current Vassal, then I suggest the best way forward would be for you to fork the current Vassal project at some point and go for it as a separate project. Once you have it stable and have resolved any migration issues, you can offer it as an alternative client.

Regards,
Brent.

10

I will chime in agreement especially in that I’m much more excited by the “profiling” thread and the idea that all those slow calls to PieceCloner might get replaced with something faster.

Also that bug fix you did in Undo is absolutely golden for quality of life. I would love to see ongoing cleanup work in Vassal 3.

There are also several low-hanging fruit missing items that I guess fall into Brent’s “quality of life improvements” category – like #1 would be a “Search” function in the main Editor window.

Brian

11

Wouldn’t being able to copy from one module to another count as low hanging fruit?

12

Maybe there is some kind of middle ground between keeping 100% backward compatibility and a large scale refactoring.

Right now the only thing I want to change in the code is to write some unit tests for the Deck class, just like I did with the Stack, in part because I suspect I’ve found a nasty source of bugs in the Deck code. In part because I think the Deck should not BE a stack but HAVE a stack inside, and I’d need unit test coverage to make that change in a backward compatible manner. But unlike the Stack, I cannot even instantiate the Deck with the default constructor as the GameModule and the PlayerRoster are in the way, so I was looking for a orderly way to remove these dependencies and this long-term plan kind of naturally evolved out of this need.

As for the modules and their dependency on the current code, I am still very interested in some feedback to this viewtopic.php?f=5&t=11868&start=30#p60978, as it might provide some actual numbers and potentially a definite list of which methods and fields the modules actually depend on. As it is now, this seems like a very vague affair, “some” modules might depend on “something” but no one knows for sure. If we replace this vagueness with some hard numbers, and this is well possible, it would be much easier to talk about the module’s dependencies and the degree of backward compatibility that is needed.