castor
  1. castor
  2. CASTOR-1790

Refactor javasource package to resolve 'Design by extension' complaints of checkstyle

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.1 M2
    • Fix Version/s: 1.1 M3
    • Component/s: XML code generator
    • Labels:
      None
    • Number of attachments :
      2

      Description

      Summary says it all.

      1. patch-C1790-20070102.txt
        394 kB
        Ralf Joachim
      2. patch-C1790-20070102-01.txt
        18 kB
        Ralf Joachim

        Activity

        Hide
        Edward Kuns added a comment -

        I much prefer

        if (jType.isPrimitive()) {

        over

        if (jType instanceof JPrimitiveType) {

        because the former communicates intent and the second communicates only membership. That is, with the second, we cannot transparently have another type return isPrimitive() true. Thus, changes to the concept of isPrimitive are no longer encapsulated.

        I don't like the idea of making this sort of change just to make a checkstyle warning go away.

        Show
        Edward Kuns added a comment - I much prefer if (jType.isPrimitive()) { over if (jType instanceof JPrimitiveType) { because the former communicates intent and the second communicates only membership. That is, with the second, we cannot transparently have another type return isPrimitive() true. Thus, changes to the concept of isPrimitive are no longer encapsulated. I don't like the idea of making this sort of change just to make a checkstyle warning go away.
        Hide
        Ralf Joachim added a comment -

        After I had finished the patch I thought about the same. Will reopen the issue and introduce/use 2 methods at JType:

        boolean isPrimitive()

        { return (this instanceof JPrimitiveType); }

        boolean isArray()

        { return (this instanceof JArrayType); }

        This should not introduce new warnings while it encapsulates implementation. Having said that the extraction of JPrimitiveType may also allow us to get rid of some conditional statements at futur refactorings.

        Show
        Ralf Joachim added a comment - After I had finished the patch I thought about the same. Will reopen the issue and introduce/use 2 methods at JType: boolean isPrimitive() { return (this instanceof JPrimitiveType); } boolean isArray() { return (this instanceof JArrayType); } This should not introduce new warnings while it encapsulates implementation. Having said that the extraction of JPrimitiveType may also allow us to get rid of some conditional statements at futur refactorings.
        Hide
        Ralf Joachim added a comment -

        As commented above.

        Show
        Ralf Joachim added a comment - As commented above.
        Hide
        Ralf Joachim added a comment -

        Additional changes as explained above.

        Show
        Ralf Joachim added a comment - Additional changes as explained above.
        Hide
        Edward Kuns added a comment -

        Yes, introducitng JPrimitiveType is definitely a nice thing.

        Show
        Edward Kuns added a comment - Yes, introducitng JPrimitiveType is definitely a nice thing.

          People

          • Assignee:
            Ralf Joachim
            Reporter:
            Ralf Joachim
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: