r10022 Discussion - do you support copy/pastes?

[Max mtg]

Dear colleagues, what do you think of copying and pasting code?

Gui_Winlose.java:

Code: Select all

    void quitButton_actionPerformed(ActionEvent e) {
        // issue 147 - keep battlefield up following win/loss
        JFrame frame = (JFrame) AllZone.getDisplay();
        frame.dispose();
        frame.setEnabled(true);

and

Code: Select all

    void continueButton_actionPerformed(ActionEvent e) {
        // issue 147 - keep battlefield up following win/loss
        JFrame frame = (JFrame) AllZone.getDisplay();
        frame.dispose();

and

Code: Select all

    void restartButton_actionPerformed(ActionEvent e) {
        // issue 147 - keep battlefield up following win/loss
        JFrame frame = (JFrame) AllZone.getDisplay();
        frame.dispose();

 
 
 
Why wasn't not the code written in this whay?

Code: Select all

void releaseGameScreen() {
        JFrame frame = (JFrame) AllZone.getDisplay();
        frame.dispose();
}

with calls from appropriate places.

[Rob Cashwalker]

There's no great impact there. It's actually worse to clutter the class with an extra method just to house 2 lines of code. And for the record, I wrote each of those lines by hand, no copy/pasting this time.

[Max mtg]

The impact is not on performance, but on efforts needed to support this code.

* What if AllZone changes its name or interface? How much less efforts are needed to update a single reference instead a number of them?
* Why keep issue numbers in comments? What will be their use in 6 months?
* Why are you receiving Display interface and casting to JFrame? Why at all

The extra method to "clutter" will instead provide information on code purpose by itsName.

I am very surprised to see you consider copying same code over different locations normal.
http://en.wikipedia.org/wiki/Duplicate_code

[Rob Cashwalker]

If you use eclipse, refactor -> rename will automatically hunt down all references and update them.

Comments aren't usually one of my strong suits. If something breaks due to these changes, it will be easier for other devs to see and understand what the change was in reference to.

The AllZone.getDisplay doesn't provide an interface which exposes dispose() nor setEnabled(). I was using the code in GameAction as a reference, which does the cast as JFrame.

Copy and paste should be avoided for sure, on a grand scale.... If you consider what CardFactory USED to look like, there were literally hundreds of duplicate blocks of code, save for a couple numbers. Shock and Lightning Bolt had two blocks of code, one with a value of 2 and the other with 3, everything else was the same.
In this case, does it really make sense for a couple lines?

If it bothers you that much you're welcome to change it.

[Max mtg]

Copy and paste should be avoided for sure, on a grand scale.... If you consider what CardFactory USED to look like, there were literally hundreds of duplicate blocks of code, save for a couple numbers. Shock and Lightning Bolt had two blocks of code, one with a value of 2 and the other with 3, everything else was the same.
In this case, does it really make sense for a couple lines?

If it bothers you that much you're welcome to change it.

Do you mean to say, every second card was hardcoded? That's awful! So, you've made a great effort advancing CardFacroty... that's a huge amount of work.

Yet, this does not mean the current or new duplicates, small or large, are somewhat insignifcant. Each copy is a pain in the ass for the one who is going to develop it further. Are you going to move along with this code? Then each copy will become an obstacle at some moment.

Well, consider what deckeditor now is, now when I have to refactor it, make this code correct and somehow merge the two deckeditors. It would be a few-lines change if it was written properly.
This is why I object to any copypastes, even the smallest.

Code: Select all

    private void jbInit() throws Exception {

        border1 = new EtchedBorder(EtchedBorder.RAISED, Color.white, new Color(148, 145, 140));
        titledBorder1 = new TitledBorder(BorderFactory.createEtchedBorder(Color.white, new Color(148, 145, 140)),
                "All Cards");
        border2 = BorderFactory.createEtchedBorder(Color.white, new Color(148, 145, 140));
        titledBorder2 = new TitledBorder(border2, "Deck");
        this.getContentPane().setLayout(null);
        jScrollPane1.setBorder(titledBorder1);
        jScrollPane1.setBounds(new Rectangle(19, 20, 726, 346));
        jScrollPane2.setBorder(titledBorder2);
        jScrollPane2.setBounds(new Rectangle(19, 458, 726, 218));
        removeButton.setBounds(new Rectangle(180, 403, 146, 49));
        // removeButton.setIcon(upIcon);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            removeButton.setFont(new java.awt.Font("Dialog", 0, 13));
        removeButton.setText("Remove Card");
        removeButton.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(ActionEvent e) {
                removeButton_actionPerformed(e);
            }
        });
        addButton.setText("Add Card");
        addButton.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(ActionEvent e) {
                addButton_actionPerformed(e);
            }
        });
        // addButton.setIcon(downIcon);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            addButton.setFont(new java.awt.Font("Dialog", 0, 13));
        addButton.setBounds(new Rectangle(23, 403, 146, 49));

        analysisButton.setText("Deck Analysis");
        analysisButton.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(ActionEvent e) {
                analysisButton_actionPerformed(e);
            }
        });
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            analysisButton.setFont(new java.awt.Font("Dialog", 0, 13));
        analysisButton.setBounds(new Rectangle(578, 426, 166, 25));

        changePictureButton.setText("Change picture...");
        changePictureButton.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(ActionEvent e) {
                changePictureButton_actionPerformed(e);
            }
        });
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            changePictureButton.setFont(new java.awt.Font("Dialog", 0, 10));
        changePictureButton.setBounds(new Rectangle(765, 349, 118, 20));

        removePictureButton.setText("Remove picture...");
        removePictureButton.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(ActionEvent e) {
                removePictureButton_actionPerformed(e);
            }
        });
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            removePictureButton.setFont(new java.awt.Font("Dialog", 0, 10));
        removePictureButton.setBounds(new Rectangle(885, 349, 118, 20));

        /**
         * Type filtering
         */
        Font f = new Font("Tahoma", Font.PLAIN, 10);
        landCheckBox.setBounds(340, 400, 48, 20);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            landCheckBox.setFont(f);
        landCheckBox.setOpaque(false);
        landCheckBox.addItemListener(new ItemListener() {
            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        creatureCheckBox.setBounds(385, 400, 65, 20);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            creatureCheckBox.setFont(f);
        creatureCheckBox.setOpaque(false);
        creatureCheckBox.addItemListener(new ItemListener() {
            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        sorceryCheckBox.setBounds(447, 400, 62, 20);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            sorceryCheckBox.setFont(f);
        sorceryCheckBox.setOpaque(false);
        sorceryCheckBox.addItemListener(new ItemListener() {
            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        instantCheckBox.setBounds(505, 400, 60, 20);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            instantCheckBox.setFont(f);
        instantCheckBox.setOpaque(false);
        instantCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        planeswalkerCheckBox.setBounds(558, 400, 85, 20);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            planeswalkerCheckBox.setFont(f);
        planeswalkerCheckBox.setOpaque(false);
        planeswalkerCheckBox.addItemListener(new ItemListener() {
            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        artifactCheckBox.setBounds(638, 400, 58, 20);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            artifactCheckBox.setFont(f);
        artifactCheckBox.setOpaque(false);
        artifactCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        enchantmentCheckBox.setBounds(692, 400, 80, 20);
        if (!OldGuiNewGame.useLAFFonts.isSelected())
            enchantmentCheckBox.setFont(f);
        enchantmentCheckBox.setOpaque(false);
        enchantmentCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });

        /**
         * Color filtering
         */
        whiteCheckBox.setBounds(340, 430, 40, 20);
        whiteCheckBox.setOpaque(false);
        whiteCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        blueCheckBox.setBounds(380, 430, 40, 20);
        blueCheckBox.setOpaque(false);
        blueCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        blackCheckBox.setBounds(420, 430, 40, 20);
        blackCheckBox.setOpaque(false);
        blackCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        redCheckBox.setBounds(460, 430, 40, 20);
        redCheckBox.setOpaque(false);
        redCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        greenCheckBox.setBounds(500, 430, 40, 20);
        greenCheckBox.setOpaque(false);
        greenCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {
                updateDisplay();
            }
        });
        colorlessCheckBox.setBounds(540, 430, 40, 20);
        colorlessCheckBox.setOpaque(false);
        colorlessCheckBox.addItemListener(new ItemListener() {

            public void itemStateChanged(ItemEvent e) {

                updateDisplay();
            }
        });

[friarsol]

In the case above, isn't the "right" solution just to call these functions directly? Why are we creating an intermediary function for a one line function all?

Code: Select all

((JFrame) AllZone.getDisplay()).dispose();


Edit: Or just have a function that returns the JFrame.

Code: Select all

AllZone.getDisplayFrame().dispose();


[Max mtg]

In the case above, isn't the "right" solution just to call these functions directly? Why are we creating an intermediary function for a one line function all?

Code: Select all

((JFrame) AllZone.getDisplay()).dispose();


Edit: Or just have a function that returns the JFrame.

Code: Select all

AllZone.getDisplayFrame().dispose();


One line without comments is... well.. okay.
But having a function lets:
1. understand from its name what the code does - and there's no need to write comments
2. change the code in one place to have the behaviour updated at all cases
3. have much less headache when refactoring this code.

[Max mtg]

Rob, check please http://cardforge.org/bugz/view.php?id=187 and 188 too...

[Rob Cashwalker]

Sol, the frame reference is used twice. Once for the dispose and then a few lines down for the setEnabled. Yes, it would've been better if AllZone.getDisplay gave me an object that exposed the right methods, but I went with a minimal modification approach.

187 makes sense, I don't see how I'm related to 188, but I'll see what I can find for it.

[Braids]

@Max mtg, Forge is an old project, and its participants have not always used best practices. that is history.

what we do now matters. so, everyone is working to fix things.

as to code being self documenting, i disagree. we need more javadoc. methods' contracts should be specified in natural language. that's why javadoc comments are required by our CheckStyle configuration.

[Max mtg]

as to code being self documenting, i disagree. we need more javadoc. methods' contracts should be specified in natural language. that's why javadoc comments are required by our CheckStyle configuration.

We cannot rely much on javadoc - it gets outdated too fast. Refactor does not reflect updates in javadoc.
Now, while I'm working with editors, the javadocs I see there just bloat the code. They are senseless and take much space. Like this one:

Code: Select all

    /**
     * <p>
     * clearFilterButton_actionPerformed.
     * </p>
     *
     * @param e
     *            a {@link java.awt.event.ActionEvent} object.
     */
    void clearFilterButton_actionPerformed(ActionEvent e) {


This particular javadoc is just a copy of the what I would learn from function header much faster. Moreover, once you've read javadoc, you'll have to check whether it actually matches the code... if it up to date, up to code...

So, I doubt presence of javadoc should be a requirement, otherwise we'll end up having docs like this.

[Braids]

yes, that javadoc is skeletal and not very helpful. but eclipse can apply some refactoring to javadoc. also, the checkstyle plugin will help keep your params, throws, and returns congruent with code.

it has been my experience that the code with the best javadoc tends to change the least.

if you're really going to try for self documenting code, you're definitely going to have to come up with better variable names than "numCopies" or "cntCopies" to indicate the number of different illustrations a card has in a given set.

does anyone else feel that you can't be bothered with keeping class and method javadocs up to date {e.g., as you edit methods} in Forge?

[Rob Cashwalker]

{raises hand}Through the entire process of writing the BugzReporter and New NewGame screen, I haven't attempted any javadocs. I see little point in them personally, as just about all of them so far have the skeletal information in them, since it looks like they were just auto-generated. The few I've come across that seem like some human thought was put into them are only slightly more helpful than the method name itself, or regular linear comments near the header.

[Max mtg]

yes, that javadoc is skeletal and not very helpful. but eclipse can apply some refactoring to javadoc. also, the checkstyle plugin will help keep your params, throws, and returns congruent with code.

it has been my experience that the code with the best javadoc tends to change the least.

if you're really going to try for self documenting code, you're definitely going to have to come up with better variable names than "numCopies" or "cntCopies" to indicate the number of different illustrations a card has in a given set.

Anyway, to be useful javadoc has to contain extra data, which is either not reflected in the code below or it's is quite tricky to figure out that the code does. But as Rob said, a couple of comment lines would do the same thing without repeating method's name.

Yes, I'm always open for discussion and comments on my code.
As for this particular case numCopies stands for "How many times the given card was printed in the given set". I did not mean number of illustrations but the number of cards themselves, counting physical objects but pictures for them.
I'm not very sure about it - didn't have a change to discuss and prove that idea. But what I mean is that referring to real-life objects in classes or variables makes them easier to operate. In real world we have cards, they belong to some sets. In each set I can (just without any computers) count how many cards have the same name but different art, number, and artist. So not the only illustration but the whole card is different, that's why I started counting them, not number-of-illustrations. "Illustration" is something abstract and thus harder to operate with.

[Braids]

wow, i've been updating javadocs for nothing. ever get a warm fuzzy feeling? i'm getting the opposite.