A question: adding setters and getters in ItemPoolView class

[Max mtg]

Hello,

What is the reason that in revision 11666 jendave has added setters and getters in ItemPoolView class to fields that are not supposed to be public?

Code: Select all

    /**
     * @return the cards
     */
    public Map<T, Integer> getCards() {
        return cards;
    }

    /**
     * @param cards the cards to set
     */
    public void setCards(Map<T, Integer> cards) {
        this.cards = cards; // TODO: Add 0 to parameter's name.
    }

    /**
     * @return the myClass
     */
    public Class<T> getMyClass() {
        return myClass;
    }

    /**
     * @return the isListInSync
     */
    public boolean isListInSync() {
        return isListInSync;
    }

    /**
     * @param isListInSync the isListInSync to set
     */
    public void setListInSync(boolean isListInSync) {
        this.isListInSync = isListInSync; // TODO: Add 0 to parameter's name.
    }


[friarsol]

You want someone's blood for... adding getters and setters? Isn't that a bit extreme?

[Max mtg]

You want someone's blood for... adding getters and setters? Isn't that a bit extreme?

Ok, so what would be the appropriate punishment for breaking class design?

I always told that this checkstyle for some unknown reason is put above the common sense. And most of you refuse to cancel this insanity of mandatory public get/set and autogenerated javadocs.

[Chris H.]

I do not believe that some sort of a punishment would be appropriate.

A few months ago some issues came up in reference to beta/snapshot releases. I listened to everyone and posted several versions of a guideline for releases.

Our current release guidelines work fairly well even though we occasionally have to deal with unforeseen circumstances. We seem to be handling this in a calm and non-inflammatory fashion. A win-win situation for everyone.

As far as checkstyle is concerned, a similar response may be the best way to handle this matter.

In the past we have had some devs express an interest in the project but failed to get involved. Some coders do not want to work on a project that appears to be a hack rather than a professionally organized project.

Currently most people turn off checkstyle as they want to concentrate on finding the errors which will prevent a build and run. After running the build and release Maven script I examine the checkstyle report and I try to fix as many of the easy ones as I can.

Occasionally, Dave will step in and will quickly address the remaining checkstyle errors. He probably does not have enough time to thoroughly analyze the code and this could be a reasonable explanation.

I am not sure what sort of a solution to propose though. I guess that creating getters and setters might create a situation where someone else might access the value via a setter and this in turn could make that section of code then produce an unplanned value.

[friarsol]

Instead of being hostile towards another one of the devs, why not just ask a question?

"These object fields were meant to be private and non-accessible, which is why I didn't create accessors for them. I would like to remove the getter/setter that were added, but I'd also like to make sure that it's not giving a warning in checkstyle so it doesn't need to be 'fixed'. What's the best way of going about doing that?"

[silly freak]

I must say that Max is right in this regard, and I found nothing hostile in his original post. I'm not a native speaker, which could make some difference, but I guess that the responses about hostility are mostly "historical".

Occasionally, Dave will step in and will quickly address the remaining checkstyle errors. He probably does not have enough time to thoroughly analyze the code and this could be a reasonable explanation.

Nonfunctional changes, like adding Javadocs, might be done without "thoroughly analyzing" - although I doubt that a descriptive comment emerges from that. Functional changes, like effectively changing the visibility of a field, are different. Encapsulation is not about adding methods to a class, but about handling the visibility of fields, so if checkstyle demands them, I'm all for turning it off.

Now, I haven't looked more thorougly, but it could also be that dave needed these additional methods - I doubt it because the <T, Integer> looks like a generic Bag-like class, not something where a usecase was forgotten in which visibility was necessary - In that case, there might be some point that dave should have asked why the field didn't have accessors.

Having said that, I'm of course in no position to make any decisions; I just felt that I could add a valuable opinion based on programming principles, and I felt that Max deserves a little defense for stating something which is true. I was reading that Max stated a potential error, and did so sufficiently politely, and this combination is a positive thing.

[moomarc]

I think the hostility being referred too was the original title of the topic, something along the lines of "Jendave must die"

In light of that the post was definitely read in a hostile light, not as simply stating a possible error. But you do make some good points otherwise.

[Chris H.]

The original title may have been problematic, but it was changed fairly quickly. I would like for all of us to move beyond the initial problem and instead to focus on addressing the issue itself.

The root folder for the ForgeSVN project has a file named ".checkstyle" and it's contents look look this:

Code: Select all

<?xml version="1.0" encoding="UTF-8"?>

<fileset-config file-format-version="1.2.0" simple-config="true" sync-formatter="false">
  <local-check-config name="Forge Checkstyle Config" location="src/main/config/forge_checks.xml" type="project" description="">
    <property name="cacheFile" value="/target/checkstyle-cachefile"/>
    <additional-data name="protect-config-file" value="false"/>
  </local-check-config>
  <fileset name="all" enabled="true" check-config-name="Forge Checkstyle Config" local="true">
    <file-match-pattern match-pattern="." include-pattern="true"/>
  </fileset>
  <filter name="WriteProtectedFiles" enabled="true"/>
  <filter name="UnOpenedFiles" enabled="true"/>
</fileset-config>


`
OK, the src/main/config/forge_checks.xml file looks like this:

Code: Select all

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<!--

  Checkstyle configuration that checks the sun coding conventions from:

    - the Java Language Specification at
      http://java.sun.com/docs/books/jls/second_edition/html/index.html

    - the Sun Code Conventions at http://java.sun.com/docs/codeconv/

    - the Javadoc guidelines at
      http://java.sun.com/j2se/javadoc/writingdoccomments/index.html

    - the JDK Api documentation http://java.sun.com/j2se/docs/api/index.html

    - some best practices

  Checkstyle is very configurable. Be sure to read the documentation at
  http://checkstyle.sf.net (or in your downloaded distribution).

  Most Checks are configurable, be sure to consult the documentation.

  To completely disable a check, just comment it out or delete it from the file.

  Finally, it is worth reading the documentation.

-->

<module name="Checker">
    <!--
        If you set the basedir property below, then all reported file
        names will be relative to the specified directory. See
        http://checkstyle.sourceforge.net/5.x/config.html#Checker

        <property name="basedir" value="${basedir}"/>
    -->

    <!-- Checks that a package-info.java file exists for each package.     -->
    <!-- See http://checkstyle.sf.net/config_javadoc.html#JavadocPackage -->
    <!-- <module name="JavadocPackage"/> -->

    <!-- Checks whether files end with a new line.                        -->
    <!-- See http://checkstyle.sf.net/config_misc.html#NewlineAtEndOfFile -->
    <module name="NewlineAtEndOfFile"/>

    <!-- Checks that property files contain the same keys.         -->
    <!-- See http://checkstyle.sf.net/config_misc.html#Translation -->
    <module name="Translation"/>

    <!-- Checks for Size Violations.                    -->
    <!-- See http://checkstyle.sf.net/config_sizes.html -->
    <!--  <module name="FileLength"/> -->

    <!-- Checks for whitespace                               -->
    <!-- See http://checkstyle.sf.net/config_whitespace.html -->
    <module name="FileTabCharacter"/>

    <!-- Miscellaneous other checks.                   -->
    <!-- See http://checkstyle.sf.net/config_misc.html -->
    <module name="RegexpSingleline">
        <property name="format" value="(^|[^\*])\s+$"/>
        <property name="minimum" value="0"/>
        <property name="maximum" value="0"/>
        <property name="message" value="Line has trailing spaces."/>
    </module>

    <module name="TreeWalker">
        <property name="cacheFile" value="${cacheFile}"/>
        <!-- Checks for Javadoc comments.                     -->
        <!-- See http://checkstyle.sf.net/config_javadoc.html -->
        <module name="JavadocMethod">
            <property name="scope" value="package"/>
        </module>
        <module name="JavadocType">
            <property name="scope" value="package"/>
        </module>
        <module name="JavadocVariable">
            <property name="scope" value="package"/>
        </module>
        <module name="JavadocStyle"/>


        <!-- Checks for Naming Conventions.                  -->
        <!-- See http://checkstyle.sf.net/config_naming.html -->
        <module name="ConstantName"/>
        <module name="LocalFinalVariableName"/>
        <module name="LocalVariableName"/>
        <module name="MemberName"/>

      <!-- Allow test_ methods to have underscores in them. -->
        <module name="MethodName">
          <property name="format" value="^[a-z][a-zA-Z0-9]*$|^test_[_a-zA-Z0-9]*$"/>
        </module>
       
        <module name="PackageName"/>
        <module name="ParameterName"/>
        <module name="StaticVariableName"/>
        <module name="TypeName"/>


        <!-- Checks for Headers                                -->
        <!-- See http://checkstyle.sf.net/config_header.html   -->
        <!-- <module name="Header">                            -->
        <!-- The follow property value demonstrates the ability     -->
        <!-- to have access to ANT properties. In this case it uses -->
        <!-- the ${basedir} property to allow Checkstyle to be run  -->
        <!-- from any directory within a project. See property      -->
        <!-- expansion,                                             -->
        <!-- http://checkstyle.sf.net/config.html#properties        -->
        <!-- <property                                              -->
        <!--     name="headerFile"                                  -->
        <!--     value="${basedir}/java.header"/>                   -->
        <!-- </module> -->

        <!-- Following interprets the header file as regular expressions. -->
        <!-- <module name="RegexpHeader"/>                                -->


        <!-- Checks for imports                              -->
        <!-- See http://checkstyle.sf.net/config_import.html -->
        <module name="AvoidStarImport"/>
        <module name="IllegalImport"/>
        <!-- defaults to sun.* packages -->
        <module name="RedundantImport"/>
        <module name="UnusedImports"/>


        <!-- Checks for Size Violations.                    -->
        <!-- See http://checkstyle.sf.net/config_sizes.html -->
        <!-- <module name="LineLength">
            <property name="max" value="180"/>
        </module>
        <module name="MethodLength"/>
        <module name="ParameterNumber"/> -->


        <!-- Checks for whitespace                               -->
        <!-- See http://checkstyle.sf.net/config_whitespace.html -->
        <module name="EmptyForIteratorPad"/>
        <module name="GenericWhitespace"/>
        <module name="MethodParamPad"/>
        <module name="NoWhitespaceAfter">
         <property name="tokens" value="BNOT, DEC, DOT, INC, LNOT, UNARY_MINUS, UNARY_PLUS"/>
        </module>
        <module name="NoWhitespaceBefore"/>
        <module name="OperatorWrap"/>
        <module name="ParenPad"/>
        <module name="TypecastParenPad"/>
        <module name="WhitespaceAfter"/>
        <module name="WhitespaceAround"/>


        <!-- Modifier Checks                                    -->
        <!-- See http://checkstyle.sf.net/config_modifiers.html -->
        <module name="ModifierOrder"/>
        <module name="RedundantModifier"/>


        <!-- Checks for blocks. You know, those {}'s         -->
        <!-- See http://checkstyle.sf.net/config_blocks.html -->
        <module name="AvoidNestedBlocks"/>

        <!-- Allow empty blocks. <module name="EmptyBlock"/> -->

        <module name="LeftCurly">
          <!-- Place left curly at EOL for one-line clauses,
               and on next line for multi-line clauses.
          -->
          <!--   <property name="option" value="nlow" /> -->
        </module>
         
        <module name="NeedBraces"/>
       
        <!-- Allow for right curly to appear on same or next line. <module name="RightCurly"/> -->


        <!-- Checks for common coding problems               -->
        <!-- See http://checkstyle.sf.net/config_coding.html -->
        <!-- <module name="AvoidInlineConditionals"/> -->
        <module name="DoubleCheckedLocking"/>
        <!-- MY FAVOURITE -->
        <module name="EmptyStatement"/>
        <module name="EqualsHashCode"/>
        <!-- <module name="HiddenField"/> -->
        <module name="IllegalInstantiation"/>
        <module name="InnerAssignment"/>
        <!-- <module name="MagicNumber"/> -->
        <module name="MissingSwitchDefault"/>
        <module name="RedundantThrows"/>
        <module name="SimplifyBooleanExpression"/>
        <module name="SimplifyBooleanReturn"/>

        <!-- Checks for class design                         -->
        <!-- See http://checkstyle.sf.net/config_design.html -->
        <!-- <module name="DesignForExtension"/> -->
        <module name="FinalClass"/>
        <!--  <module name="HideUtilityClassConstructor"/> -->
        <module name="InterfaceIsType"/>
        <module name="VisibilityModifier"/>


        <!-- Miscellaneous other checks.                   -->
        <!-- See http://checkstyle.sf.net/config_misc.html -->
        <module name="ArrayTypeStyle"/>
        <!--  <module name="FinalParameters"/> -->
        <!-- <module name="TodoComment"/> -->
        <module name="UpperEll"/>

    </module>

</module>


`
Someone familiar with this type of settings might be able to remove the lack of getters and setters test. This might help to alleviate the problem that was reported.

I would like to keep the CheckStyle report itself.

[Max mtg]

Chris does not want jendave's blood, so he renamed the topic.
They keep speaking of my "hostility"... Ok, I can be a monster for this season. It's Innistrad time anyway.


Sorry, I see no "win-win situation" when someone adds paths to break other persons' code, without even asking "why there are no accessor methods?".

I greatly doubt that abscense of autogenerated javadocs makes the project hacky. The hardcoded calls to getHumanPlayer() in AI classes do, the 8700+ lines classes (like forge.Card) do, the hardcoded spells and abilities do too, but not the absence of setters for some fields designed to be private.

Many devs do turn off checkstyle, because a "wrong number of spaces around a bracket" is not really an error that prevents us from building the project and since it appears in problems list below the code, it only distracts attention from some really important things.