Unnecessary Wrappings



  • This feels like a WTF but you guys have beat me down so much, that I don't know anymore ...

    So, I find:

    ParameterChangeEvent extends java.beans.PropertyChangeEvent

    Hrm... And PropertyChangeEvent's constructor looks like:

    public PropertyChangeEvent ( source, propertyName, oldValue, newValue )

    Examining ParameterChangeEvent, I see the constructor looks like:

    public ParameterChangeEvent ( source, propertyName, oldValue, newValue, message )

    Okay, so maybe we're making this look like an informative message being provided? Kind of like throwing exceptions? I proceed to find usages of the message (private member/field). It only appears in one method:

    public String toString() {
      return message;
    }

    :blink: um ... So I search for usages of ParameterChangeEvent, and this is what I find in multiple places:

    if ( ! oldValue.equals( newValue ) ) {
      StringBuffer buffer = new StringBuffer();
      buffer.append("The ");
      buffer.append(component.getName());
      buffer.append(" was changed from ");
      buffer.append(oldValue);
      buffer.append(" to ");
      buffer.append(newValue);
      oldValue = newValue;  // we also have a "happens-before" problem here
      fireParameterChangeEvent(new ParameterChangeEvent ( this, component.getName(), oldValue, newValue, buffer.toString() )); 
    }

    :facepalm: There is no getMessage() method in the newly created PCE. There IS however, a getParameterName() method:

    public String getParameterName() {
      return getPropertyName();
    }

    If they wanted to wrap/rename PropertyChangeEvent and provide different output, WTF is the point for the message field if the results can be calculated from the object itself?!?! Simply override toString() to provide the output you want. Done. And they wouldn't have needed to create a new ParameterChangeListener and fireParamterChangeEvent infrastructure to handle the new events! Ah, but history has shown me/us that they really didn't/don't get inheritance/polymorphism when they created this code.



  • Don't take the beatings personally. It's part being in this ... um ... community.

     

    Keep the WTFery coming.



  • You're right; that is a lump of WTFery, and it's OO WTFery for once.  This shit belongs on the front page, since there's so much more to discuss with an OO WTF.  And I might actually be able to learn something in here again, other than the depths of human depravity as manifested by a certain troll who shall not be named.

    @rudraigh said:

    Don't take the beatings personally.  It's part of being in this ... um ... community.

    Keep the WTFery coming.

    Yeah, I think I remember getting beat down in here a few times. But, [sigh] they're all dead now.

    Speaking of which, how long has it been since your last beat-down?

    @zelmak said:

    And they wouldn't have needed to create a new ParameterChangeListener and fireParamterChangeEvent infrastructure to handle the new events! Ah, but history has shown me/us that they really didn't/don't get inheritance/polymorphism when they created this code.

    Awwwwkwaaaaaard.



  • @zelmak said:

    Okay, so maybe we're making this look like an informative message being provided? Kind of like throwing exceptions? I proceed to find usages of the message (private member/field). It only appears in one method:

    public String toString() {
    return message;
    }

    While it wasn't in this language, and it wasn't in an object built at all like this, I did update some code to something very similar once.

    That having been said, my update was a bugfix to some horribly broken code that was trying far, far too hard to convert a string into a string, with the same value as the original string.  It was also failing miserably in several cases.  Also, my update was step one of a two step process:

    1. Get the code working again as quickly as feasible.
    2. Clean up all of the residual crap that was calling the useless routine, and remove the useless routine.

    Management wasn't interested in doing the second step; they had a lot of other things they felt were higher priority.  Because of this, over a year went by between step one and step two.  However, I didn't let it fall off my radar, and I took care of it during a few brief periods where management did not have identified tasks for me to work on.  Eventually, it was rolled out as part of a larger optimization effort.


    That having been said, I think the above code comes from premature optimization - you know, it's faster to figure this stuff out as things change, even if you're never going to need it, than it is to figure it out every time you need it.  And, in the unlikely case that this value is retrieved several times between each event that could change the string value, caching the return value and deleting the cached value when anything that would affect the correct return value would be very complicated and couldn't possibly work.  (I have a toy program I occasionally mess with that demonstrates how easyimpossible this really is.)



  • @tgape said:

    That having been said, I think the above code comes from premature optimization - you know, it's faster to figure this stuff out as things change, even if you're never going to need it, than it is to figure it out every time you need it.  And, in the unlikely case that this value is retrieved several times between each event that could change the string value, caching the return value and deleting the cached value when anything that would affect the correct return value would be very complicated and couldn't possibly work.  (I have a toy program I occasionally mess with that demonstrates how easyimpossible this really is.)

    It should be trivial if the object is immutable, no? PCE looks quite immutable to me and it isn't storing references to objects I'd expect to be mutable.

    On topic though, what I'd do in the case where the object can get updated at any time, but the value must be cached:

    public class MicroOptimusCachius {
        private class MyData {
            ... many fields ...
            private String message;
            ... a constructor to initialize the fields...
        }
        private AtomicReference<MyData> pData;
    
    public MicroOptimusCachius(...) {
        data.set(new MyData(....));
    }
    
    public setWhatever(....) {
        do {
            MyData oldData = pData.get();
            MyData newData = (MyData) oldData.clone();
            newData.whatever = ...
        while (!pData.compareAndSet(oldData, newData));
    }
    
    public String toString() {
        // no need to do the entire compare-and-set as above, since if more than one thread
        // sets data.message, they'll all set it to the exact same string.
        MyData data = pData.get();
        if (data.message == null) data.message = massageData(data);
        return data.message;
    }
    

    }




    Seems like a completely sane way of dealing with the case when the object is seldom updated, but the value is queried frequently, which is the only time when such caching makes sense. Alternatively, use an AtomicInteger and increment it on each change to the object, then recalculate the string if the value has changed since it was last cached.



  •  Nevermind what I said before... I thought I was looking at an event argument signature rather than an event signature.  They really did break everything!



  • @Mo6eB @tgape

    I'd buy the whole caching thing (I do it myself sometimes) if it weren't for the fact that it is handled OUTSIDE the Event object where the "cache" String message resides. The toString() method should handle this; the 'message' should be set during the first call to toString(), internally. Call it lazy-initialization or something. Done.

    It's fairly obvious they weren't thinking about OO -- letting the object do its own work with its own internal knowledge -- but more as a data-holding object where they set all the fields the way then wanted.

     



  • PSA: "Preview" != "Post" 

    @hoodaticus said:

    Speaking of which, how long has it been since your last beat-down?

    Depends on your intended scope. This codebase has been beating me down for over two years now and every day I find something new to whine/bitch/WTF about. Beat down here? Heck, sometimes the insults are so clever, that I don't know if it is or isn't directed at me!

    @zelmak said:
    And they wouldn't have needed to create a new ParameterChangeListener and fireParamterChangeEvent infrastructure to handle the new events! Ah, but history has shown me/us that they really didn't/don't get inheritance/polymorphism when they created this code.

    Awwwwkwaaaaaard.

    I almost feel like you're dissing me here ... let me explain my thoughts. If they would have extended PropertyChangeEvent with the cached toString() method as we discussed earlier, they would be able to use existing PropertyChangeListener and firePropertyChangeEvent infrastructure instead of creating the ParameterChangeListener interface and all of the fireParameterChangeEvent methods that they did.

    If you were aiming at my predecessors, then, disregard. :)



  • @tgape said:

    (I have a toy program I occasionally mess with that demonstrates how easyimpossible this really is.)

    Share?


Log in to reply