props.PropObject - Stub Fields

[Smithor]

There are several methods in PropObject that are stub methods with no logic, namely: getPropsForArchivalExtra, initProps, getPropValue and setPropValue.

getPropsforArchival arguably is fine as is, but I believe it would be improved if it was made to return an empty array instead. Functionality would remain the same, just a reduced null pointer.

The other three are the actual one that I wanted to make the issue about. These three are all basic methods that, given that they are at the base class of every single view in the library, seem quite essential to have, but they are currently stubs that will quietly fail if they weren't implemented by one of PropObject's descendants. This seems to me to be a large problem, as any implementor cannot assume that these are intrinsic parts of the library, even though they should be. To resolve this, I have three potential solutions:

• Provide a default implementation.
• Make these methods abstract.
• Throw some sort of "NotImplementedException" exception by default.

As PropObject is an abstract class, there is no problem with the methods also being abstract, but the stated purpose of PropObject is to provide Prop support to child classes, so I do feel that it should provide a default implementation. Nothing wrong with then overriding it, but a method call shouldn't silently fail on fresh implementation. The third option is a poor replacement, but is better than an empty stub that says nothing.

My making the issue doesn't mean you need to do this btw, I'll slot it into the existing #14 pull request once I know what your thoughts are.

[reportmill]

I actually want to change a few of these prop methods to List from Prop[] (I went through a period where I thought arrays were more elegant for some reason - was hoping Java might change to 'auto-box' arrays to Lists). I'll put that on my short list to refactor and change that getPropsForArchivalExtra() to return an empty list.

I'm not sure if it's worth bothering to make get/setPropValue and initProps abstract and have a DefaultPropObject for some of the same reasons as ViewController. I almost consider PropObject an internal class to give View and friends easy property handling, archival and undo.

[Smithor]

I also usually hand out things as arrays, swapping them to lists when you need to is easy with the streams API, but can be a little harder to go backwards, and they technically are a little lighter in memory. But I've seen it done both ways, list or array, just don't give me an iterator, that's just cruel.

I usually look at it like this:

• Am I meant to just read it and not change? - Array
• Are the number of values set to a certain number for a reason? - Array
• Is this collection a deep copy? - Array
• Am I allowed to change the number of values? - List
• Does the original grouping change if I alter the provided collection? - List
• Am I expected to immediately mutate this grouping in all environments and situations? - List

They both have their place, and think choosing strictly one or the other loses out on the strengths that they both provide. Lists encourage modification, while arrays encourage stability in some sort. They can of course both do the same jobs, but they both have value on their own.

As for the things mentioned in this issue.

Props was my starting point for the deep dive (I'm hoping to do a review of everything in the library as I work through stuff), and I noticed a bunch of things that I hadn't mentioned yet. Had planned to bring them up as I collected more solid info, but you responded to these much faster than I expected haha.

As part of my process, I encountered a situation where I would have benefited from a property being stored in the view, and I thought "instead of just throwing a variable in the class, maybe I could try out one of these Props instead". Upon the dive, it becomes obvious that the Props class has very little to do with Properties, and is actually just a serialization package. I had been planning to cover a bunch of the small stuff first, and then post an issue talking about that, but we might as well discuss it here.

The ViewController is definitely a front-end library feature, it is something a user will be interacting with on a regular basis, but the Prop stuff is still woven in. When I first saw the Props, based on the javadoc comments, I assumed it was some sort of built in Properties object that I could use at runtime. It wasn't until I started to dive into the code to test that hypothesis that I found out that I was wrong, and that it was actually for serialization of the view. It may be a back-end system, but all of its methods are public, and part of the ViewController contract. This does cause some confusion.

There is also the argument that it is not a back-end feature, given that it does have the public API, and is even covered as a language feature in the read-me. My personal opinion is that maybe the package should be refactored to something like "SnapXML" or "SnapSerialization", which would remove the confusion. A Properties object is a built-in, very common system in Java, and I think it would be very easy for someone else to make the same mistake that I did. Even though its still part of the contract, at least then it would be more obvious what its for, and that it is not the thing you are probably looking for.