> Basically, I'm suggesting making more changes now (though still very
> minor ones, just pervasively) to (a) ensure the code adheres to the DRY
> principle, (b) reduce the chance of the changes being incomplete
> (through missing checks that would need to be updated with approach 1)
> and (c) reduces the chance of future mistakes creeping in.

I don't understand how the second approach furthers (a) and (c). 

(a) It sounds like we would be repeating the same checks in every
accessor rather "saying it once" in a shared method in a base class.

(c) With the second approach, a very common mistake might be to
neglect applying the conversion or to apply conversions
inconsistently.

As to (b), I'd think that the first approach does imply the same level
of code review you'd make with the second approach, even if we would
be changing fewer lines of code.

IMHO, the preferred approach would be the one that combined ease of
maintenance for the development team and ease of modifcation for
individual users. That sounds like the first approach to me, since it
would seem that we would centralize our expectations for the
attribute's characteristics: null or empty, trimmed or verbatim. We
would have one point of access to maintain, and individual users would
have one point of access to modify.

Whether to return empty or null strings *is* an important issue. To
streamline our business logic, my own team has to convert the empty
strings to null sometimes. Though, in other cases, we do want an empty
string to pass through, since the property might correspond to a field
on the database that doesn't accept nulls, but does accept empty
strings. We could adjust our interface between the presentation and
application layers easily enough, but other teams might not be so
agile.

If we centralize the logic, as Niall suggested, then it seems like it
would be easier to managage a common switch attribute, like
"nullOnEmpty". If we decentralize the logic, then it would seem like
we'd have to make that test over and over again, creating new
opportunities for oversight and inconsistency.

Having a switch attribute, as Laurie has mentioned several times,
seems important, since people will want to test how well the changed
behavior will play with other taglibs that extend or utilize the
Struts Taglib. If there is a problem, we will need to be able to
quickly isolate the empty-vs-null behavior change, to discover whether
it is the culprit.

Of course, it would be nice to see both patches, so that we have a
factual basis for comparison.

-Ted.

On 9/17/05, Laurie Harper <[EMAIL PROTECTED]> wrote:
> Ted Husted wrote:
> > In general, we work  by lazy consensus. If no one squawks, we sally forth.
> 
> OK, before I sally ;-) I've been thinking a little more about this
> change. There are two approaches that seem reasonable, one of which
> minimized changes in the code and one of which minimizes the amount of
> code that will have to be reviewed in addition to testing as well as the
> potential for mistakes to creep in later. Guess which I'm leaning toward ;-)
> 
> The first approach would be what Niall already proposed: changing the
> checks for 'null' in BaseHandlerTag and a few other places. As I said,
> that has the advantage of minimizing the amount of code change.
> 
> The second approach would be to modify the attribute setters to convert
> empty-String arguments to null right away. This has a few advantages:
> 
>   - it makes it very clear and explicit that empty strings are being
> thrown away, and the decision of whether or not to ignore empty strings
> can be made once per attribute, rather than everywhere that attribute
> value is referenced
> 
>   - it ensures that no existing checks for null that should be changed
> into checks for empty-or-null get missed; whereever there's an 'if (x ==
> null) now, the code would remain unchanged
> 
>   - more importantly, it ensures no checks for null get added later that
> should be checks for empty-or-null, particularly since the checking
> semantics could so easily get out-of-sync if the same check is made in
> more than one place
> 
> Basically, I'm suggesting making more changes now (though still very
> minor ones, just pervasively) to (a) ensure the code adheres to the DRY
> principle, (b) reduce the chance of the changes being incomplete
> (through missing checks that would need to be updated with approach 1)
> and (c) reduces the chance of future mistakes creeping in.
> 
> Am I over-selling? ;-) This is the approach I'll take unless I hear an
> objection.
> 
> L.

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to