4
\$\begingroup\$

This is my first forray into Java Swing and it's not exactly pretty. I have quite a few methods, and a lot of stuff going on (XML parsing, data conversion, etc) and I'm just wondering what kind of general improvements / redundancies I can eliminate from my code.

test.xml:

<weeks>
    <week>
        <date>2012/01/23 00:00:00</date>
        <riddle>Why Did the chicken Cross the road</riddle>
        <lastWeekSolution>Because I said So</lastWeekSolution>
    </week>
    <week>
        <date>2012/01/30 00:00:00</date>
        <riddle>ipso lorem 1</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/02/06 00:00:00</date>
        <riddle>ipso lorem 2</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/02/13 00:00:00</date>
        <riddle>ipso lorem 3</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/02/20 00:00:00</date>
        <riddle>ipso lorem 4 </riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/02/27 00:00:00</date>
        <riddle>ipso lorem 5</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/03/05 00:00:00</date>
        <riddle>ipso lorem 6</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/03/12 00:00:00</date>
        <riddle>ipso lorem 7</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/03/19 00:00:00</date>
        <riddle>ipso lorem 8</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/03/26 00:00:00</date>
        <riddle>ipso lorem 9</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/04/02 00:00:00</date>
        <riddle>ipso lorem 10</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/04/09 00:00:00</date>
        <riddle>ipso lorem 11</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/04/16 00:00:00</date>
        <riddle>ipso lorem 12</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/04/23 00:00:00</date>
        <riddle>ipso lorem 13</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
        <week>
        <date>2012/04/30 00:00:00</date>
        <riddle>ipso lorem 14</riddle>
        <lastWeekSolution>I'm Going to get this</lastWeekSolution>
    </week>
</weeks>

MoreSwing.java:

package moreswing;
// @author Damien Bell
import java.util.Date;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.io.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.JFrame;
import javax.swing.SwingUtilities;
import javax.swing.JPanel;
import javax.swing.JButton;
import javax.swing.JTextArea;
import javax.swing.JLabel;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.DocumentBuilder;
import org.w3c.dom.Document;
import org.w3c.dom.NodeList;
import org.w3c.dom.Node;
import org.w3c.dom.Element;
import java.util.Scanner;
import org.xml.sax.SAXException;
import org.xml.sax.SAXParseException; 


public class MoreSwing extends JFrame {
    public MoreSwing(){
        initUI();
    }

    private int j;
    private String[] strArray = new String[4]; 
    JTextArea area = new JTextArea(strArray[1]);
    JButton nextButton = new JButton("Next");
    JButton prevButton = new JButton("Previous");
    JButton toggleAnswerButton = new JButton("Previous");
    boolean isToggledNext = false;
    boolean isToggledPrev = false;
    boolean dateLockBool=false;

    public final Date getDateTime(){
        Date date = new Date();
        return date;
    }

    //Loads everything from the selected week in the xml file into an array of strings via xml parsing.
    public final String[] xmlLoader(int s, String[] strArray_m){
        try {
            DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance();
            DocumentBuilder docBuilder = docBuilderFactory.newDocumentBuilder();
            Document doc = docBuilder.parse (new File("test.xml"));

            // normalize text representation
            doc.getDocumentElement ().normalize ();
            NodeList listOfWeek = doc.getElementsByTagName("week");

                Node firstWeekNode = listOfWeek.item(s-1);
                int totalWeeks = listOfWeek.getLength();
                strArray_m[3] = Integer.toString(totalWeeks);


                if(firstWeekNode.getNodeType() == Node.ELEMENT_NODE){
                    Element firstWeekElement = (Element)firstWeekNode;
                    //-------
                    NodeList dateList = firstWeekElement.getElementsByTagName("date");
                    Element dateElement = (Element)dateList.item(0);

                    NodeList textDateList = dateElement.getChildNodes();
                    strArray_m[0]= (((Node)textDateList.item(0)).getNodeValue().trim()).toString();

                    //-------
                    NodeList riddleList = firstWeekElement.getElementsByTagName("riddle");
                    Element riddleElement = (Element)riddleList.item(0);

                    NodeList textRiddleList = riddleElement.getChildNodes();
                    strArray_m[1]= (((Node)textRiddleList.item(0)).getNodeValue().trim()).toString();

                    //----
                    NodeList lWSList = firstWeekElement.getElementsByTagName("lastWeekSolution");
                    Element ageElement = (Element)lWSList.item(0);

                    NodeList textLWSList = ageElement.getChildNodes();
                    strArray_m[2]= (((Node)textLWSList.item(0)).getNodeValue().trim()).toString();

                    //------
                }//end of if clause

        }
            catch (SAXParseException err) {
        System.out.println ("** Parsing error" + ", line " 
             + err.getLineNumber () + ", uri " + err.getSystemId ());
        System.out.println(" " + err.getMessage ());

        }catch (SAXException e) {
        Exception x = e.getException ();
        ((x == null) ? e : x).printStackTrace ();

        }catch (Throwable t) {
        t.printStackTrace ();
        }
        return strArray_m;
    }

    //Handles all the data from the next button
    public final void doUpdate(int j, String[] strArray_m, JTextArea area){
        xmlLoader(j, strArray_m); //Load stuff from the xml file
        area.setText(strArray_m[1]); // update the text area
    }

    public final boolean toggleButton(JButton buttonDisable, boolean disableButtonBool){
        buttonDisable.setEnabled(!buttonDisable.isEnabled()); // Swap button state
        if(disableButtonBool){ disableButtonBool = false; } // Swap bool
        else { disableButtonBool = true; }//Swap bool
        return disableButtonBool;//return Bool
    }////End ToggleButton method

    public final void dateLock(JButton nextButton){
            nextButton.setEnabled(!nextButton.isEnabled());
    }//End Datelock Method

    public final void toggleContent(JTextArea area, JButton toggleContent, String[] strArray_m){
        String toggle = toggleContent.getText();
        if(toggle.equalsIgnoreCase("Show Last Week's Answer")){
            toggleContent.setText("This Week's Brain Teaser");
            area.setText(strArray_m[2]);
        }//end if
        else{
            toggleContent.setText("Show Last Week's Answer");
            area.setText(strArray_m[1]);
        }//End else
    }//End ToggleContent method

    public final void initUI() {
        //add panel
        JPanel panel = new JPanel();
        getContentPane().add(panel);
        panel.setLayout(null);

        //Declare Scanner, input the week you want, load that week from XML file
        Scanner input = new Scanner(System.in);
        j = 1;       
        xmlLoader(j, strArray);
        isToggledPrev = toggleButton(prevButton, isToggledPrev);

        //Create text area with returned XML data as default text area text
        area.setBounds(50, 100, 350, 400);
        panel.add(area); // add text area to panel
        area.setText(strArray[1]);


        JLabel labelAuthor = new JLabel("Author: Damien Bell");
        labelAuthor.setBounds(620, 600, 150, 50);
        panel.add(labelAuthor);

        toggleAnswerButton.setBounds(95, 520, 250, 30);
        toggleAnswerButton.setText("Show Last Week's Answer");
        toggleAnswerButton.addActionListener(new ActionListener() {
           public void actionPerformed(ActionEvent ae){
             toggleContent(area, toggleAnswerButton, strArray);  
           }             
        });
        panel.add(toggleAnswerButton);

        nextButton.setBounds(550, 250, 80, 30);
        nextButton.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent ae){
            Date date1 = getDateTime();
            DateFormat dateFormat = new SimpleDateFormat("yyyy/MM/dd HH:mm:ss");
            try{
                Date date2 = dateFormat.parse(strArray[0]);
                if (date1.compareTo(date2) < 0 ){

                    dateLock(nextButton);
                    dateLockBool=true;
                }
            }   
            catch (Exception e) { System.err.println("Error: " + e.getMessage()); }
                if((j+2)> Integer.parseInt(strArray[3])){
                    isToggledNext = toggleButton(nextButton, isToggledNext);
                }
                if(isToggledPrev){
                   isToggledPrev = toggleButton(prevButton, isToggledPrev);
                }
               j++;
               doUpdate(j, strArray, area);
           } 
        });
        panel.add(nextButton);



        prevButton.setBounds(550, 350, 80, 30);
        prevButton.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent ae){
                if((j-2) == 0 ){
                   isToggledPrev = toggleButton(prevButton, isToggledPrev);
                }
                if(isToggledNext){
                   isToggledNext = toggleButton(nextButton, isToggledNext);
                }
                if(dateLockBool){
                   dateLockBool = false;
                   dateLock(nextButton);
                }
            j--;
            doUpdate(j, strArray, area);
          } 
        });
        panel.add(prevButton);


        //Add quit button
        JButton quitButton = new JButton("Quit");
        quitButton.setBounds(10,610,80,30);
        quitButton.addActionListener(new ActionListener() {
            public void actionPerformed (ActionEvent event){
                System.exit(0);
            }
        });

       panel.add(quitButton); // Add quit button to panel


       //Set overall window options for swing window
       setTitle("Quit Button Test");
       setSize(800,700);
       setLocationRelativeTo(null);
       setDefaultCloseOperation(EXIT_ON_CLOSE);
    }

    // Main, lol
    public static void main(String args[]){
        SwingUtilities.invokeLater(new Runnable() {
            public void run(){
                MoreSwing ms = new MoreSwing();
                ms.setVisible(true);
            }
        });
    }
}
\$\endgroup\$
0
3
\$\begingroup\$
  1. Instead of printing stacktraces to the console show the error message to the user. (See later.)

  2. The following code with the ternary operator is really hard to read. Try to avoid it.

    final Exception x = e.getException();
    ((x == null) ? e : x).printStackTrace();
    
  3. What is j?

    private int j;
    

    Try to find a more meaningful name.

  4. The widget fields should be private and they could be final. I don't think that other classes should access these objects.

    private final JButton nextButton = new JButton("Next");
    ...
    

    It would improve cohesion.

  5. Fields should be at the beginning of the class, then comes the constructor, then the other methods. (According to the Code Conventions for the Java Programming Language, 3.1.3 Class and Interface Declarations.)

  6. Variable names such as strArray_m does not fit to the Code Conventions for the Java Programming Language, Chapter 9: Naming Conventions)

  7. Comments on the closing curly braces are unnecessary and disturbing. Modern IDEs could show blocks.

    }// //End ToggleButton method
    

    “// …” comments at end of code block after } - good or bad?

  8. The disableButtonBool parameter of the toggleButton does nothing with the state of the button. I'd remove this parameter.

    public final boolean toggleButton(final JButton buttonDisable, boolean disableButtonBool) {
        buttonDisable.setEnabled(!buttonDisable.isEnabled()); // Swap button
                                                              // state
        if (disableButtonBool) {
            disableButtonBool = false;
        } else {
            disableButtonBool = true;
        }
        return disableButtonBool;// return Bool
    }// //End ToggleButton method
    
  9. I'd move the toggle method a ToggleButton class (which would be a subclass of JButton):

    public class ToggleButton extends JButton {
        private static final long serialVersionUID = 1L;
    
        public ToggleButton(String text) {
            super(text);
        }
    
        public boolean toggle() {
            final boolean newState = !isEnabled();
            setEnabled(newState);
            return newState;
        }
    }
    

    Then I'd use this class in the MoreSwing class:

    private final ToggleButton nextButton = new ToggleButton("Next");
    private final ToggleButton prevButton = new ToggleButton("Previous");
    

    The toggle method belongs here, it manipulates the state of the button. (It increases cohesion.)

  10. The xmlLoader method should have a separate class: XmlLoader. Currently it violates the single responsibility principle.

    Then the XmlLoader.loadXml() method should throw a custom exception in the catch block:

    } catch (final SAXParseException saxpe) {
        final String errorMsg = "** Parsing error" + ...
        throw new WeekException(errorMsg, saxpe);
    }
    

    Your UI have to handle these WeekException-s. For example, show a Swing error box to the user with the message of the exception. (Find a better name for the exception, please.)

  11. The Scanner instance is unnecessary:

    Scanner input = new Scanner(System.in);
    

    (Unused variable.)

  12. I'm not too familiar with Swing, but I think there should be a better way to stop the application than calling System.exit():

    quitButton.addActionListener(new ActionListener() {
        @Override
        public void actionPerformed(final ActionEvent event) {
            System.exit(0);
        }
    });
    
  13. I'd use JAXB for the XML processing.

\$\endgroup\$
7
  • \$\begingroup\$ This is amazing, thank you so much. For the record I do have a few clarifications I'd like to ask / post: 1. That's a good idea, I will definitely change that. -- 2. I didn't even write that exception code. I grabbed a lot of the xml code from a website then tweaked it a LOT to suit my needs. -- 3. I meant to change that a while back, too bad you can't just refactor for a single letter, eh? -- 4. Another good idea, they should definitely be private, and I suppose that quit could be final, all the others have things acting on them. \$\endgroup\$ Jan 27 '12 at 23:14
  • \$\begingroup\$ 5. Can you explain a bit more why this convention exists and why it's the norm in java? I got yelled at for not creating things at the time of initialization / usage in C++ and now it seems like the opposite in Java--- 6. I really struggle to find a good variable name convention that works for me. --- 7. I suppose you have a point, however I am showing this to a teacher (she knows that I check my code on here) so having the distinction of lots of comments will (hopefully) help her to read it. \$\endgroup\$ Jan 27 '12 at 23:15
  • \$\begingroup\$ 8. Yeah, I meant to do something else with that, but then I ended up making datelock which worked the other way around. -- 9. I'm not exactly sure what this code does / what you mean, can you explain this a little more? 10. How would you suggest that I fix the multiple responsibility side of this? I'd never heard of that before, although it does make sense. -- 11. Ahhh, I forgot to remove that, it was part of my debug code from earlier, good catch. -- 12. I never really understood why system.exit(0) was good / bad, can you explain that? \$\endgroup\$ Jan 27 '12 at 23:22
  • \$\begingroup\$ And finally, thanks again for all your help, it really means a lot to see what's wrong with my code. I plan to go back and fix what I understood of what you listed here. \$\endgroup\$ Jan 27 '12 at 23:22
  • \$\begingroup\$ If you copy code from the web and it will be part of the application you will be responsible for that. If it's even just a homework it could lead to awkward situations when the teacher ask how this line works and why you chose that :-) \$\endgroup\$
    – palacsint
    Jan 29 '12 at 23:58
2
\$\begingroup\$

Given that you already appear to have an error in your XML file (because the solution presented in the first week appears to be the answer to itself, not lastWeekSolution as claimed), I'd recommend revising your structure. It's also going to cause problems with your edge cases. The first one logically can't have a solution element at all (because a blank solution is still a solution), and the last element can only have a solution element. It's also going to cause problems if you read it into something that ignores the order in the XML file - that is, treats it as set data.

I'd revise it to:

<riddle>
    <postingTimestamp>2012/01/23 00:00:00</postingTimestamp>
    <query>Why Did the chicken Cross the road?</query>
    <solution>Because I told him to!</solution>
</riddle>

I'd recommend making your XSD (you did define one, right?) allow between 0 and some multiple number of solutions (some riddles have no answer, some may have multiple). (Of course, I'd also probably store this in a database, not an XML file, but whatever).

\$\endgroup\$
5
  • \$\begingroup\$ In this case the XML file was really just a filler, I should've mentioned that. The first week's solution should've been more like <week> <riddle>Why did the chicken cross the road</riddle> <LWS>There was no solution since this is the first week</LWS> <date>whatever</date> \$\endgroup\$ Jan 27 '12 at 18:41
  • \$\begingroup\$ Although you do make a good point about the edge cases being a pain, I haven't really conceptualized a solution to the final week solution quite yet. \$\endgroup\$ Jan 27 '12 at 18:43
  • \$\begingroup\$ @HunderingThooves - There's also the problem that you're explicitly connecting this riddle to the previous one. You can't simply insert a new riddle between existing ones, you have to update multiple entries; I'd rather grab two entries as necessary, than one that I hope contains the correct answer. Also, I'd drop the week element, and just key things by date - you should change your program to aggregate them by weeks as necessary (what happens if you put an entry in the wrong week?). \$\endgroup\$ Jan 27 '12 at 18:55
  • \$\begingroup\$ How would you do that X-Zero, aggregate them by weeks that is? The best I could think of is the date comparison that I used above, but if there's a better way to go about this I would love to learn it. \$\endgroup\$ Jan 27 '12 at 20:28
  • \$\begingroup\$ Once you have a Date object (or, because I really like the Joda Time library, DateTime) you should be able to query it for week number. I haven't really looked through your Java code, I was focusing more on the problems generated by your data-model; however, I can already tell you're violating seperation of concerns - your display code ABSOLUTELY SHOULD NOT be loading your backend data. Design patterns will help you here - MVC will be a good place to start. \$\endgroup\$ Jan 27 '12 at 20:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.