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