But I was the architect



  • As part of some change being made for another system, I needed to trap certain events. Given the existing spaghetti code of the system, there were a couple of setter methods that were ideal places to trigger the event handler. Granted, it'd be a side affect, but it would be one line of code in each of two one-line setters within ten lines of each other in the same class, and I could document it clearly and thoroughly.

    I created a utility class, and two entry methods, one per setter that would be acting as a trigger. I used some maps to hold precomputed values and transformations, so all the handler would need to do was basic error checking, and two map lookups. All the setter had to do was pass this, and the old and new values. It was simple and it got the job done in a minimally invasive manner.

    The guy who originally wrote our system, for whose (new) system this change was being made decided that instead, we should have an interface for the single instance of the handler, and that the handler should have a single method, which would have a switch for the old values, each with nested switches for new values. And instead of calling the handler in just a couple of setters, it should instead be called in all the places that these setters were called.

    All 294 of them.

    Don't get me wrong, I hate side affects - especially if they're not at all obvious in how they're called or what they do.

    But I also believe in balancing code-purity against decentralization so severe that making what would otherwise be trivial changes becomes exceedingly prohibitive.

    Me: Can you please explain to me why you would want to make something so simple into something so widespread and invasive?

    Him: Because I am the architect.

    Me: Not on our project you're not (at least not any more)

    Him: But I was the architect!

    Me: Since I'm the one who will have to support it, I'm doing it in the easiest-to-support manner.



  • Snoofle, your way (based on posted information) is definately the better choice. I would have done one thing different, in that I would have changed the setters (2 of them) to method calls [nearly every refactoring tool makes this a single click operation to update all 294 sites]. This eliminates the "side effect in property consideration" completely.



  • How about writing a Setter Wrapper that does what you need, then calls the origional setter.

    Then you change the 294 places to point to the wrapper.



  • @bgodot said:

    How about writing a Setter Wrapper that does what you need, then calls the origional setter.

    Then you change the 294 places to point to the wrapper.

    I would not do that, it leaves the possibility open of someone calling the setter directly.



  • @TheCPUWizard said:

    Snoofle, your way (based on posted information) is definately the better choice. I would have done one thing different, in that I would have changed the setters (2 of them) to method calls [nearly every refactoring tool makes this a single click operation to update all 294 sites]. This eliminates the "side effect in property consideration" completely.
    I'm not sure what you mean. In the (Java) data object, the setter is a method:

    private Type x;

    public void setX(Type x) { Handler.staticInterfaceMethod(this,this.x, x); this.x = x; }

    ...and the calling code just does: theObject.setX(theValue);

    From your use of the word property, I'm guessing (dunno) you're thinking of a C#-ish language that uses actual properties?



  • @snoofle said:

    @TheCPUWizard said:

    Snoofle, your way (based on posted information) is definately the better choice. I would have done one thing different, in that I would have changed the setters (2 of them) to method calls [nearly every refactoring tool makes this a single click operation to update all 294 sites]. This eliminates the "side effect in property consideration" completely.
    I'm not sure what you mean. In the (Java) data object, the setter is a method:

    private Type x;

    public void setX(Type x) { Handler.staticInterfaceMethod(this,this.x, x); this.x = x; }

    ...and the calling code just does: theObject.setX(theValue);

    From your use of the word property, I'm guessing (dunno) you're thinking of a C#-ish language that uses actual properties?

    You are correct, I was thinking about languages with real properties. [FWIW, although I know many/most of your posts are Java, there was nothing in this one specifically stating so]. So in this case simply changing the name to not follow "property conventions" would have accomplished the same thing....



  •  Presumably, using reflection was not an option? Given the previous stories you've contributed, the performance impact should not be an issue. :)



  • @snoofle said:

    And instead of calling the handler in just a couple of setters, it should instead be called in all the places that these setters were called.

    Code requiring that whenever you call X, you also have to call Y. The worst offence against encapsulation.

    @snoofle said:

    Don't get me wrong, I hate side affects - especially if they're not at all obvious in how they're called or what they do.

    The whole point of wrapping field access in getters and setters is to allow them to have side-effects.



  • Is there going to be enough of this kind of thing to make using java.util.Observable and java.util.Observer worthwhile? Being able to pat your ex-architect on the head and say "don't worry, look, I'm using a standard design pattern" might be worth a little extra fiddliness.



  • Did he cross his arms and pout? I like to imagine he did.@Bulb said:

    The whole point of wrapping field access in getters and setters is to allow them to have side-effects.
    Personally, I've always thought the main point was doing value validation in the setters and hiding the difference between stored and calculated values in the getters.

     



  • @snoofle said:

    Me: Can you please explain to me why you would want to make something so simple into something so widespread and invasive?

    Him: Because I am the architect.

    "...and you're just a contractor".



  • Your architect clearly doesn't know what he's doing. I used to work with one of the most brilliant architects in the industry, and he used to solve problems like this all the time. All without changing a single line of the other team's code.

    Firstly, you should be using .NET, and if you were, what you could do is use an AOP framework to transparently intercept the calls to the setter methods AND/OR use call-site replacement to completely eliminate the side-effects in setter code, without requiring any changes to the code at all. Just add a few aspect classes in some completely unrelated place. 

    The best thing about this approach is that the program now exhibits runtime behavior which is not expressed in the source code. So if the maintenance programmer doesn't know what "bytecode enhancement" is (or that such a thing is even possible), he/she will end up driving a corkscrew through their skull to put an end to the frustration of attempting to debug problems caused by the non-existant side-effects.

    After narrowly avoiding the prospect of self-inflicted trepanation, I can't help but see AOP as nothing more than a structured COMEFROM statement? A ludicrous idea when it was proposed, but now an emerging architectural pattern.


  • :belt_onion:

    @caffiend said:

    Firstly, you should be using .NET, and if you were, what you could do is use an AOP framework to transparently intercept the calls to the setter methods AND/OR use call-site replacement to completely eliminate the side-effects in setter code, without requiring any changes to the code at all. Just add a few aspect classes in some completely unrelated place. 

    The best thing about this approach is that the program now exhibits runtime behavior which is not expressed in the source code. So if the maintenance programmer doesn't know what "bytecode enhancement" is (or that such a thing is even possible), he/she will end up driving a corkscrew through their skull to put an end to the frustration of attempting to debug problems caused by the non-existant side-effects.

    Not sure if you are serious or being sarcastic. Probably both, so I'll post anyway. Aspect orientation should only be used for cross-cutting concerns such as caching, logging, security... never for the actual business processing. These are not the kind of unexpected side-effects that a maintance developer will fail to understand if he has received a decent hand-over. Maybe the first time it could take a while until he realizes that he's data is coming from a cache but after that he will be familiar with that

     



  • @bjolling said:

    Not sure if you are serious or being sarcastic.

    I had sarcasm tags around the "suggestion" originally. Thought it was too cheesey.

    I know what AOP is meant for, and i'm sure it's awesome if you're writing an ORM or need to quickly add instrumentation to every method. Appropriately used in those contexts, it serves a purpose. 

    I was simply pointing out that i've seen it misused, and it was painful. In my naievity i didn't realize that a non-virtual function could be somehow overriden. Unless, such "magic" is a watertight abstraction or prominently documented, it's use can be extremely costly to an organization if knowledge of it's use is "lost to the sands of time" and some poor bastard is left trying to figgure out why the call to a trivial set-method whereby the code obviously does only one thing, then secretly this framework does another thing (transparent to the debugger). My introduction to AOP frameworks was after looking at ILDASM dumps of certain assemblies and having a WTF moment. Not saying they're bad, just they can be misused. I should write a "considered harmful" paper.  



  • @snoofle said:

    the handler should have a single method, which would have a switch for the old values, each with nested switches for new values
    Uh... what? I have no idea what that is even supposed to mean. At first I thought that "switch" meant the same as "parameter" but I have never heard of "nested method parameters"...

    Oh, wait... you mean switch statements? Sorry, blanked out there for a moment. Still, I don't quite see how you could switch on every possible old-value/new-value pair for an event handler, unless the value range is severely limited (enums, or whatever).


  • ♿ (Parody)

    @Anonymouse said:

    Still, I don't quite see how you could switch on every possible old-value/new-value pair for an event handler, unless the value range is severely limited (enums, or whatever).

    Yeah, like we haven't seen lots of these monstrosities around here.



  • @Anonymouse said:

    Still, I don't quite see how you could switch on every possible old-value/new-value pair for an event handler, unless the value range is severely limited (enums, or whatever).
    Precisely! The range is finite, but not at all small (over 200 combinations in all).

    I did it with maps, so the results look basically like this:

    // validate input (convert literals to enums and check for in/validtity)

    ResultType rt = map2.get(map1.get(theParameter));

    // minor business logic (5 lines)

    // store rt someplace for later (1 line)

     



  • @flabdablet said:

    java.util.Observable
    Yes, but the object is already entrenched in a deeply nested hierarchy of huge data objects, so extending Observable is not viable. Ordinarily, with an encapsulated object, I could wrap just the affected fields in local Observable objects and hide them behind getter/setter methods, but this mess is completely decapsulated; the internal fields are directly referenced everywhere.

    Sigh.



  • @toon said:

    @snoofle said:

    Me: Can you please explain to me why you would want to make something so simple into something so widespread and invasive?

    Him: Because I am the architect.

    "...and you're just a contractor".

    True, but he is no longer the architect, and he was never my boss, so I don't need to kiss up or listen to him. My boss wants me to protect HIS system and try to minimize HIS headaches, and has directed and even begged me to push back against stupidity. I get to say no!

     



  • @caffiend said:

    I can't help but see AOP as nothing more than a structured COMEFROM statement? A ludicrous idea when it was proposed, but now an emerging architectural pattern.
     

    @wikipedia said:

    On 1 April 2004, Richie Hindle published an implementation of both GOTO and COMEFROM for the Python programming language.

    PYTHON.



  • @snoofle said:

    @toon said:

    @snoofle said:

    Me: Can you please explain to me why you would want to make something so simple into something so widespread and invasive?

    Him: Because I am the architect.

    "...and you're just a contractor".

    True, but he is no longer the architect, and he was never my boss, so I don't need to kiss up or listen to him. My boss wants me to protect HIS system and try to minimize HIS headaches, and has directed and even begged me to push back against stupidity. I get to say no!

     

    I sort of meant it as something he might think to himself after saying: "Because I'm the architect." I was picturing a Newman type guy, whose last shred of dignity is being able to tell other folks what to do.


  • @Zecc said:

    @Bulb said:
    The whole point of wrapping field access in getters and setters is to allow them to have side-effects.
    Personally, I've always thought the main point was doing value validation in the setters and hiding the difference between stored and calculated values in the getters.

    Both, IMHO. Or more generally:

    The whole point of accessors is maintaining invariants.

    Sometimes you have to validate values, sometimes you have to calculate values on the fly, and sometimes you have to notify other objects so they can update their state accordingly (and cause further* side effects).

    Examples:

    • shoppingCartItem.setQuantity(-5) throws an exception
    • shoppingCart.getTotal() calculates the sum on demand
    • javax.swing.Action.setEnabled(false) automatically disables all associated buttons and menu items

    * The initial change to the original object already is a side effect because it changes observable state. Right?


  • Considered Harmful

    @fatbull said:

    * The initial change to the original object already is a side effect because it changes observable state. Right?

    That's the effect. Side effects happen alongside the effect (imagine that).



  • @Zecc said:

    Personally, I've always thought the main point was doing value validation in the setters and hiding the difference between stored and calculated values in the getters.

    Not just that, you can use them to hide lazy-evaluation or lazy-loading too, which is basically what he's doing in this case.

    BTW, is lazy<T> the best thing by far in .net 4? Yes. Yes it is.



  • @caffiend said:

    Firstly, you should be using .NET, and if you were, what you could do is use an AOP framework to transparently intercept the calls to the setter methods AND/OR use call-site replacement to completely eliminate the side-effects in setter code, without requiring any changes to the code at all.


    He's using Java. The earliest Java AOP framework was publicly released before .Net.



  • @snoofle said:

    Ordinarily, with an encapsulated object, I could wrap just the affected fields in local Observable objects and hide them behind getter/setter methods, but this mess is completely decapsulated; the internal fields are directly referenced everywhere.

    Sigh.

    Ah! So you need java.util.BigBallOfMud and java.util.OnlyVerySlightlyWorse. Fair enough.



  • Just suggest the same thing but call it Aspect Oriented Programming, he will be satisfied.


Log in to reply