[josm-dev] Jumbo Patch

[Gabriel Ebner]

On Sun, Dec 16, 2007 at 05:22:25PM +0000, Gervase Markham wrote:
> Ulf Lamping wrote:
> > Well, that argument is wrong IMHO. If there are a few hundred lines of 
> > getters and setters in each file and I fully want to understand the code 
> > I have to read all of it. I don't know what there might be, as there 
> > could be some additional code in the get/set methods.
> 
> If that's true, then it was clearly a good idea to have them, because 
> people have been able to add that code without having to change all the 
> callers :-)

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

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));

Under the proposed change, this would create a leak as we'd be adding the way
to the reverse lookup tables, but never removing it.  So you'd effectively
need to write this:

  // Show the tags from w, the nodes from newNodes
  if (!askUserIfOk(w, newNodes)) {
    return;
  }

  Way wnew = new Way(w);
  wnew.nodes.clear();
  wnew.nodes.addAll(newNodes);
  Main.main.undoRedo.add(new ChangeCommand(w, wnew));

Or this:

  Way wnew = new Way(w);
  wnew.nodes.clear();
  wnew.nodes.addAll(newNodes);

  if (!askUserIfOk(wnew)) {
    wnew.dispose();
    return;
  }

  Main.main.undoRedo.add(new ChangeCommand(w, wnew));

You cannot plan for these things.  You do not want to add dispose() calls all
over the place.  You do not want to make sure twice that all the objects you
create will end up being used.  

> 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.

> One example of appropriate public instance variables is the case where 
> the class is essentially a data structure, with no behavior. In other 
> words, if you would have used a struct instead of a class (if Java 
> supported struct), then it's appropriate to make the class's instance 
> variables public."
> http://java.sun.com/docs/codeconv/html/CodeConventions.doc9.html#177
> 
> In other words, generally don't allow public access to variables unless 
> your class is very simple. But also (and I agree, this speaks against 
> long lists of getters and setters), your classes should have methods 
> which do more than just get and set values - i.e. they should have 
> behaviour as well. And calling the behavioural methods should manipulate 
> the member variables along the way.

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.

There *are* classes with "behavior" in JOSM, like UndoRedoHandler.  Whenever
you add a command to it, the dataset is modified.  And it's got *private*
instance variables.

  Gabriel.