[josm-dev] Jumbo Patch

[Gervase Markham]

Gabriel Ebner wrote:
> You'll end up having to change the callers anyhow, as they assume a certain
> behavior of how primitives work.

Then that's your mistake; those assumptions should be documented, or 
unnecessary :-)

> E.g. until now, this was the common way to change the node list of a way
> (which you'd use if you want to split a way, combine ways, simplify a way,
> ...):
> 
>   Way wnew = new Way(w);
>   wnew.nodes.clear();
>   wnew.nodes.addAll(newNodes);
>   Main.main.undoRedo.add(new ChangeCommand(w, wnew));
> 
> But the following was also legal:
> 
>   Way wnew = new Way(w);
>   wnew.nodes.clear();
>   wnew.nodes.addAll(newNodes);
> 
>   if (!askUserIfOk(wnew)) {
>     return;
>   }
> 
>   Main.main.undoRedo.add(new ChangeCommand(w, wnew));

Right. So the correct answer is to make adding nodes to a Way and to the 
undo/redo list an atomic operation, if the two must be coupled together.

Ignoring for the moment the fact that undoRedo is a "global", it might 
go something like this:

Way wnew = new Way(w);
wnew.replaceNodes(newNodes);

class Way {
   ...

   function replaceNodes(nodeList newNodes) {
     Way oldway = this.clone();
     this.nodes = newNodes;
     Main.main.undoRedo.add(new ChangeCommand(oldway, this));
     ...
   }
}

>> Skimming over them using page-down, it's clear which ones are just 
>> "getFoo { return foo; }" and which are more complicated.
> 
> I'm perfectly OK with accessor functions that actually do something and that
> make the code clearer (and/or shorter).  But we're talking here about
> one-liners of the form:
> 
>   public int nrNodes() {
>     return nodes.size();
>   }
> 
> They just serve to enforce a certain access pattern.

Yes, indeed! And that's the point.

(Note: the following examples are imaginary; I haven't read the JOSM 
code close enough to know if my detail is correct. But I'm sure you can 
see the points.)

Say you have
public NodeList getNodes() { return nodes; }
in your class. Now, the class can _never_ assume anything about the 
state of the NodeList, because you are handing it out to all and sundry, 
who could do anything with it. Can you assume that all the nodes are in 
a consistent state? No. Can you assume that all the nodes all reference 
you as the correct Way? No. Can you assume that they are in the order 
you left them in? No. Other code could have changed any of these things. 
So assumptions get violated, crashes or weird behaviour appears, and 
when you've tracked it down you end up increasing the size of your class 
because you have to check a whole bunch of things before you do anything 
on the nodelist.

public int StartEndLengthAsCrowFlies() {
   // Make sure node list isn't empty
   ...
   // Make sure nodes are in the right order
   ...
   // Make sure there are no duplicate nodes so we don't loop for ever
   ...
   // Now start doing your trigonometry
   ...
}

But this doesn't solve the problem because, unless your entire app is 
single-threaded (and you can guarantee that it will be forever more) you 
get race conditions. After you've done the checks, and before you've 
done the maths, someone modifies the NodeList via the reference you 
happily gave out and it doesn't work right one time in forty. And that 
sort of bug is an absolute pain to track down.

Private data, controlled by accessor methods which can maintain 
consistency, state, concurrency etc. is best practice for _so_ many good 
maintainability and robustness reasons. The runtime overhead is minimal, 
the on-disk overhead is tiny, and if you are really allergic to them you 
can use various design patterns to avoid having to read the files that 
contain them too often.

> The key word here is behavior:  And OsmPrimitive doesn't have any, it's a data
> structure, which the quotes exempts from the getter and setter business.

I'm not arguing about a particular class, just about the principle. If 
it's true that there's a single class in JOSM which has public member 
variables and all it is is a struct, then this is a storm in a teacup. 
But from my own brief inspection of the code, that doesn't seem to be 
the case.

Gerv