Announcement Announcement Module
No announcement yet.
Spring code remarks Page Title Module
Move Remove Collapse
Conversation Detail Module
  • Filter
  • Time
  • Show
Clear All
new posts

  • #16
    Thanks for your reply Juergen, it's always a pleasure to talk to you.

    I might want to add something to clearify some point of views.

    However, type dependency counting is an academic exercise to some degree. Many of those types that BeanWrapperImpl depends on are default PropertyEditors registered by it; that's perfectly feasible from my point of view.
    There are 2/3 of the couplings refering to java types and the editor properties are as you said default property editors. I had a mearly extense look at this class and did anything but academic exercises.

    For example the default property editors are stored in a map. Why not going for a small inner type (which is more likely to be extended or reused). In terms of complexitivity (feeled and measureable) it would be a good headstart. This would take 5min to do so. I think I should have just showed the concrete suggestions instead of using abstract language terms.

    I guess we follow the "Don't fix it if it ain't broken" principle here.
    And that is what makes me a bit disappointed. I learned that something that isn't easy to access (meaning easy to understandable) is something everyone tries to avoid to touch as long as possible. It has to become a big threat before it is actually touched. So the big thing has much time to breed its flaws to become bugs.

    In my opinion, this is not a valid argument against Spring's code quality: The packaging is actually well thought-out, I would argue, just not following your own view.
    I agree on this. I still prefer the 'Interface belongs to client' packaging style but I was never intended to say Spring is not well packed. That's why I added that I love to use Spring. It wouldn't be that useable if the package structure (or anything else) is totally screwed off.

    Discussing Spring's overall package structure is pointless to some degree, because we cannot change it without completely breaking backwards compatibility. Aside from that quite obvious fact (that you don't seem to be fully aware of), I do believe that the current package structure serves the current purposes well enough.
    I think we simply use the terms 'packaging' and 'repackaging' a bit diffrent. Moving packages between projects and subprojects without renaming them (no breaking classpaths here) is also referred as packaging by me (and some other people of cause, I just learned it by taking a extense look at the Eclipse project). Maybe this is what causes most of the irritations about this.

    I guess, most of all, I object to your generalizing: code metrics are just an indication for code quality, and by no means the only one.
    I know. So I looked at any spot I mentioned. The problem is that I have many refactoring-ideas (and I really mean refactoring here and not restructurizing or partly-reimplementation) to improve readability by simply taking a short look. So that's why I can't understand it is not already done. (I thought being a user myself at this time).

    How many interfaces and classes are there in Spring that are well thought-out, documented, polished?
    Around anything that is directly visible to the user on an everyday use. Actually I liked much of the stuff you all did with the AOP implementation. But I don't like some namings in the bean package, too. But it wasn't rarly distrurbing my reading flow anyways.

    Yes, a couple of implementation classes deserve to be refactored, but that definitely doesn't make Spring's overall code quality as bad as you tried to make it look like.
    That's what I ment. When I have a problem utilizing Spring, which I don't understand, I just take a look at the code. I take a look at the archtectural border (the border between Spring and the user), than a look at the implementation of that borderline types then at this and that implementation and woops, I find myself sometimes just lost in the woods, then I have to take a step back and need to spent valuable time to understand something, that should be understandable with ease. I don't like this as much, as I don't like extense debugging sessions. It apears to be wasted time, since in an ideal world, I wouldn't have to spend that much time.

    And for the record. I think Spring is well craftet, nearly everything that is intended to be used from within the outside, looks very well and apears to be truely high quality, but under the hood there is something left to do and lurking. This something might maybe 10 to 20% of the overall implementation lines of code but it is there. It is the part that makes me afraid of using something that I havn't done before within the Spring framework. I might run into a problem I can not easily solve.

    So at the bottom line you all still doing very well but with a little afford, I wouldn't had to spend so many times reading and reviewing certain parts of the code. And I also would like to bet that you would have saved some time also. Maybe it is like sitting infront of a beautiful car and wining about a scratch. But that scratch shouldn't be simply there.

    Second, the famous SessionImpl class in Hibernate 2.1. >4000 lines of code in one class, with all the core processing going on there, and with references to virtually everything else in the codebase. How can a code metrics lover like you, Martin, survive that without going insane?
    I didn't sumbled agross it, when I reviewed the parts I was interested. I do metrical research only when I am intended to criticise something I dislike and to take a look if this is a one timer or part of a broader problem. Hopefully I will take a look at the Hibernate 3.0 source at the weekend and maybe I can provide them some value by doing so (by concrete example or concrete solution of cause! I have learned my lesson, I guess - Thanks again!).


    Martin (Kersten)


    • #17
      Please, no Martin-style type counts from code metrics that blame BeanWrapperImpl for registering a bunch of default PropertyEditors Wink Examples for concrete use cases would be good!
      Dear Juergen, I guess now we are even. You are referring to a style using my name and the best of all is, you referring to it using a negative and offensive tone. - Don't worry, i know it was in the heat of the moment, and I am not going to take it personal! :-D

      I already wrote that I took a look at all the bits I criticise. So to prove that I am not counting academic nuts here and to defend myself, here is the BeanWarperImpl example, you might indirectly wanted me to illustrate: (code Spring R1.1.4).


      Name: BeanWarperImpl
      Type: Concrete implementation
      Afferent Dependencies: 76/78 (suggested solution by experience: decomposition)

      class BeanWarperImpl {
              /** The wrapped object */
      	private Object object;
              /** Registry for default PropertyEditors */
              private final Map defaultEditors;
              /** Map with custom PropertyEditor instances */
              private Map customEditors;
              BeanWarperImpl() {
                 // Register default editors in this class, for restricted environments.
                 // We're not using the JRE's PropertyEditorManager to avoid potential
                 // SecurityExceptions when running in a SecurityManager.
                 this.defaultEditors = new HashMap(20);
                 // Simple editors, without parameterization capabilities.
                 this.defaultEditors.put(byte[].class, new ByteArrayPropertyEditor());
                 .... 8 editor setups here 
                 // Default instances of boolean and number editors.
                 // Can be overridden by registering custom instances of those as custom editors.
                 this.defaultEditors.put(Boolean.class, new CustomBooleanEditor(false));
                 //7 more editors here 
                 // Default instances of collection editors.
                 // Can be overridden by registering custom instances of those as custom editors.
                 this.defaultEditors.put(Collection.class, new CustomCollectionEditor(Collection.class));
                 //four more editors here
      Before we can takle the problem also indicated by the metric, we need to properly prepair for the session.

      (I don't look at the unit-tests for now, I just do the session off-line)

      Here is how a refactoring session might look like:

      1st. Rename some attributes to get rid of documentation lines: (using documentation is always a pointer that not everything is clearified by the code / unit-tests)

      class BeanWarperImpl {
      	private Object warpedObject;
              private final Map defaultPropertyEditors;
              private Map customPropertyEditors; 
      The question goes to the customPropertyEditors. It is used in a meta state stance (non-existance (null) has a special meaning here). So it goes on the toDo list for later tackling (get's solved).

      2nd step: Refactor the constructor to move complexitivity to non-constructor methods (constructors can not be reused that simple, also a constructor has no special semantical meaning (it simply is about creation of an object and not nothing more)) + restructure the default property editors setup to substitute the semantics of the inlined documentation with semantic expressed by Java code directly.

              BeanWarperImpl() {
                 this.defaultPropertyEditors=new HashMap(20?);
              void registerDefaultPropertyEditors() {
      This alone whould increase the readability quite much. But yet again this BeanWarperImpl depends on 76/78 types.

      3rd step: Now we can take a good look at the couplings more closely. It is noticable that the used property editors come from the same package, and share a common super-type. (also 1/2 of the couplings are 3rd party (java etc)) related, which means something more can be done -> goes on the toDo list for now and is beyond this little refactoring session).

      After all the coupling metric is a true indicator, that this is a subject of refactoring / restructuring using decomposition.

      -> The type PropertyEditors is born. It's task? Simply allow to add/remove (register) property editors being mapped to diffrent classes.
      -> This will lead to a replacement of the Map type using aPropertyEditors implementation
      -> the result is more concrete (easier to read),
      reduced documentation needs (self explaining compared to Map),
      reuse of Map related code (PropertyEditors carries task related semantically, so we can use a more powerful language)
      -> requires less code to interact with, so we will be able to remove some lines from the BeanWarperImpl

      -> DefaultPropertyEditors subtype would be also born (inner class or global?)
      -> Simplified setup,
      possible caching (who cares, but it might be seen as expression of flexibility (note I am only talking about the possibility, I rarely done any caching that way)),
      possible reuse of PropertyEditors (extend it?)

      4th step: Optimize the PropertyEditors
      Question: Can PropertyEditors and the DefaultPropertyEditors be better related?
      Much likely answer: Indeed, it might end with DefaultPropertyEditors being nothing else then a composition of two distinct PropertyEditors (one for the defaults(static) ones and one for the mutable once)? (PS: I like the Composition pattern anyways).

      -> The distinction between default and custom would be negaleted so only an attribute propertyEditors or registeredPropertyEditors would remain.

      5th step: The unit-tests of PropertyEditors will tell us something (of cause I do my refactoring / restructoring sessions the TDD way! :-D).

      -> There should be an abstract type (expressed by an interface) called PropertyEditors. (three methods or four)
      -> So we need to extract an interface, a factory will be much likely to be created
      -> The BeanWarperImpl will only fiddle with the factory and the abstract type (interface),
      No need to impose the required knowledge of all particular property editors to the reader of the BeanWarperImpl.

      So we would be able to reduce 12+4 couplings (Map etc also) by adding two other one (propery editors, factory). I wouln't had spend time to review this piece of code since seeing the PropertyEditors meaning in a hover (thanks, IDE thanks!), and also seeing the name of the attributes, I wouldn't have to take a look of anything that concrete. Bean warper Impl would loose about 20 lines of code or more (depends of the code duplication needed to access the diffrent maps (shouldn't be much in this case, I guess)) and the next time someone comes around he would much likely to stay for a while and review some code part of interests in a more efficient mannor.

      You know this is what I refer as being code quality. Ease of library use is somewhat diffrent and might be only refer to the Archtectural quality. Maybe some people use the architactual quality and code quality interchangable, but I don't.

      And also I am quite sure, that, if I would had suggested this concrete example, instead of simply talking about the need of instant refactoring, I wouldn't have covered myself with dirt that much and there would be no need for me to rob on my knees. Also I should have suggested it one after one instead all at once. - lesson learned, but I didn't felt that it is my task as a user to do this kind of refactoring/restructuring myself.

      So hopefully the term 'Martin-style type counts from code metrics' is now reputated to be something positive rather than negative. ;-)


      Martin (Kersten)

      PS: I can do such a refactoring and commit it but I don't want to polute the dev-maillist. Is it ok, if I just deliver some patches within the CVS? I would like to do it as quiently as possible. And maybe it is enough to touch the implementations as far as I can do it? But yet again I am trained to run a refactoring session to the later end but I usally lack some detail knowledge here and there to be able to do so... .


      • #18
        At the moment I`m trying to understand the BeanWrapperImpl again.. I must say... the code is realy terrible... this is a nightmare.

        And why do you want to use the PropertyEditors from the sun spec? There are a lot of methods nobody uses, the editor is statefull. So for every request a lot of editors have to be build.. I`m thinking about dropping this dirty peace of code and use the BeanUtils from apache... this is unworkable..


        • #19
          At the moment I`m trying to understand the BeanWrapperImpl again.. I must say... the code is realy terrible... this is a nightmare.
          Watch your language dude! Believe me using such words doesn't effect your overall love-income in a positive manor... .

          Also I have analysed the Hibernate 3.0 code-base, lately. Seeing it, I now believe there are quite different schools of programming out there. (And I have signed for a quite different one.) Hibernate has a class with around 80 attributes(fields), 256 methods and around 1000 SLOCs, if I remember correctly. I didn't even tried to review this completely (would simply take too long).

          But I understood the purpose of that class (BasicEntityBeanPersister). I wouldn't touch the code of that class for sure but I guess there are other methaphors used for complexitivity control, I might simply be not aware of. And well I have to admit that these guys are just tougher than I am. I can not manage such code but they can. - It doesn't matter. It works and if there is a bug, just post a bug report. I wouldn't invest much time in finding the bug myself, anyways.

          It's simply not your butter on that toast, mister! ;-)


          Martin (Kersten)