[OSM-dev] java applet reformatted

[Immanuel Scholz]

Hi,


>> - Tab size is 4
>
> Mine is 8 :p  It's 2 in Processing, which is where the applet originates.

Okok.. "Indent size is 4".

But I use spaces anyway. (Most Java editors default the tab size to 4.)


> But actually, I'd argue that dead code can be a good form of
> documentation whilst things are under active development.  Like "I
> tried it this way, but that didn't work" or "we used to do it this
> way", etc.

Please, drop a comment then. The main problem is to differentiate between
"The commented code should not be used, it does not work" and "The
commented code is the future but currently under development".

This makes code very hard to understand (for me ;).



>> - Use base classes or interfaces instead of the concrete objects, if
>> possible. Collection instead of Vector, Map instead of Hashtable...
>
> This is OOP class now, and not OSM?  Really!  What about things like
> Vector is synchronised by default, so it can't just be swapped out for
> a standard ArrayList?  These things aren't always interchangeable.

Yes. But Hashtable and Vector was used, where synchronous stuff does not
matter (e.g. GpxParser).

If you use Vector, you make a statment: "Access to this need to be
synchronous". If this is wrong, don't use Vector.

And I am not convinced, that the pointer to the collection should be a
vector too, since there are more than one implementations of synchronized
collection classes.


> Also, Collection isn't necessarily orderered, so I would say use List
> there instead of Vector if you must.

If a routine does not need ordered access, it should not require a List.

Surely, this is silly:

  public void specialSort(Collection c);

But this is silly too (except within sumup there are multi-threaded access
to the parameter argument):

  public void sumup(Vector v);



> Is the default modifier protected then? (Or is it package... I'll look
> it up). I thought it was private but I stand corrected.

Default modifier is package protected which means, full access is granted
to all classes in the same package.


>> - Iterator is better than Enumeration.
>>
> Yeah, but if an Enumeration is used then it means Steve wrote that bit
> of the code :)  Seriously, that can actually be pretty useful.

Yea.. and very funny... :-|... As I replaced Enumeration with Iterator, I
got several ConcurrentModifyExceptions which let me reverse my changes.

Concurrent modification on Iterators are guaranteed to throw a
ConcurrentModificationException (however, the state of the Collection may
be corrupted. The exception is for debug purpose! (it is a
RuntimeException)).

Concurrent modification on Enumerations means undefined behaviour.


> If you over-zealously enforce your coding standards, you neglect the fact
> that the code base is somewhat organic and that collaborations have
> tell-tale signs that help them function more efficiently than
> rigourous formatting ever can.

Tell tale signs are always there. You don't need to sacrifice corectness
for them to happen ;-).

I just don't like leaving broken windows behind.


> Steve and I benefitted from meeting up in person

Hey, no problem. See you all at IoW! ;-)


> but feel free to ask if you want to bounce ideas around, and let us know
> what you're up to.

Steve asked me to fix the problem that currently the applet is launching
every server connection in an own thread which may cause later actions to
run faster than earlier ones. This is believed to be the problem of at
least one critical trac item (forgot the number.. something about line
segments vanishing).

I will rewrite this stuff. Only one additional thread which executes every
action queued will be used. I will keep using the command pattern to
deploy the actions, just the executor get replaced from launching a new
thread every now and then.

The access to the internal data will be restructured too, so that it is
not concurently modified. (NO! Vector is NOT the allmighty answer to every
mutli-threading problem ;-)


Oh, and I could start adding some junit test cases? Is this what the newly
test package is for?

Ciao, Imi.