4
\$\begingroup\$

I need a little help with improving my first Java program. I have programmed for about a month, so my code is quite messy. The program is a "Musiclist" program, with which you can add songs, edit songs, and remove songs from the list.

The program uses 2 ArrayLists, to store the information about the song (Song name, Artist), which will be stored to the arraylist(indexnumber).

What I need help with is:

  • Make it less messy. Maybe add ActionListeners to another class?
  • The song- and artist ArrayList that shares index seems like a bad idea. I always have to perform 2 actions like song.add(index) and artist.add(index). Maybe a 2D ArrayList?
  • I need to add the songs to a database I can call it from, when I open the program next time.

If you have any new suggestions for my program, I would love to include them.

public class MusicList extends JFrame{

    public JTextField af;
    private JList jl;
    private JButton add;
    private JButton edit;
    private JButton test;
    private JPanel jp;
    private JScrollPane sp;
    private JTextField artist;
    private JButton save;
    private JButton listb;
    private JPopupMenu jpo;
    private JMenuItem ite;
    private JButton editsave;
    private JButton editlist;
    int g;
    //creates a DefaultListModel.. actions at the JList can be made through here. e.g if adding to jlist is m.addElement();
    DefaultListModel<String> m = new DefaultListModel<String>();
    //creates arraylists
    List<String> fl = new ArrayList<String>();
    List<String> art = new ArrayList<String>();

    public MusicList(){
        super("Musiclist");
        setLayout(null);

        jl = new JList(m);
        add(jl);
        //creates a scrollpane, "implements jlist"
        sp = new JScrollPane(jl);
        sp.setBounds(30,20,150,140);
        add(sp);
        //creates the textfield to contain songname
        af = new JTextField(12);
        String afs = af.getText();
        af.setBounds(20,20,170,20);
        add(af);
        af.setVisible(false);
        //opens the add menu
        add = new JButton("add");
        add.setBounds(20,180,80,20);
        add(add);
        //opens the edit menu
        edit = new JButton("edit");
        edit.setBounds(110,180,80,20);
        add(edit);
        //this button checks if art and fl(arraylists) match to the index.
        test = new JButton("test");
        test.setBounds(200, 40, 80, 20);
        add(test);
        //the textfield which will pass the artist string.. used in add and edit
        artist = new JTextField();
        artist.setBounds(20,50,170,20);
        add(artist);
        artist.setVisible(false);
        //adds back button in "add" menu
        listb = new JButton("back");
        listb.setBounds(110,180,80,20);
        add(listb);
        listb.setVisible(false);
        //adds save button on "add" menu
        save = new JButton("save");
        save.setBounds(20,180,80,20);
        add(save);
        save.setVisible(false);
        //adds the back button on "edit" menu
        editlist = new JButton("back");
        editlist.setBounds(110, 180, 80, 20);
        add(editlist);
        editlist.setVisible(false);
        //adds the save button on "edit" menu
        editsave = new JButton ("save");
        editsave.setBounds(20,180,80,20);
        add(editsave);
        editlist.setVisible(false);


        //button to open the add window
        add.addActionListener(
                new ActionListener(){
                public void actionPerformed(ActionEvent event){

                    jl.setVisible(false);
                    sp.setVisible(false);
                    add.setVisible(false);
                    edit.setVisible(false);
                    listb.setVisible(true);
                    save.setVisible(true);
                    af.setVisible(true);    
                    artist.setVisible(true);
                    af.requestFocus();
                    }});


        edit.addActionListener(
                new ActionListener(){
                    public void actionPerformed(ActionEvent event){ 

                        System.out.println(jl.getSelectedIndex());
                        //checks if theres an selected index.. unselected index = -1.
                        if(jl.getSelectedIndex()<0){
                            JOptionPane.showMessageDialog(null, "Please select a song to edit");
                        }else{
                        //open edit window
                        jl.setVisible(false);
                        sp.setVisible(false);
                        add.setVisible(false);
                        edit.setVisible(false);
                        editlist.setVisible(true);
                        editsave.setVisible(true);
                        af.setVisible(true);    
                        artist.setVisible(true);
                        //takes selected index, and set text of textfield af and artists to selected index.
                        final int i = jl.getSelectedIndex();
                        if(i>=0){
                        System.out.println(i);
                        af.setText(fl.get(i));
                        artist.setText(art.get(i));
                        }}}});
        //test button.. checks if index + song + artist match.
        test.addActionListener(
                new ActionListener(){
                    public void actionPerformed(ActionEvent event){

                        final int i = jl.getSelectedIndex();
                        if(i>=0){
                            //String j = (m.getElementAt(i));
                            //System.out.println(j);
                            System.out.println(fl.get(i));
                            System.out.println(art.get(i));
                            }}});



        jl.addMouseListener(new MouseAdapter() {
            public void mouseClicked(MouseEvent evt) {
               //adds a actionlistener to Jlist
                JList jl = (JList)evt.getSource();
                //if double click---
                if (evt.getClickCount() == 2) {
                   //
                    int index = jl.locationToIndex(evt.getPoint());
                    //location to youtube link will be here.

                } else if (evt.getClickCount() == 3) {   // Triple-click
                    int index = jl.locationToIndex(evt.getPoint());
                    }}});
        //listb is the "back to list" button.
        listb.addActionListener(
                new ActionListener(){
                    public void actionPerformed(ActionEvent event){
                    //if u are at add window, listb will take u back to the list of songs.
                        jl.setVisible(true);
                        sp.setVisible(true);
                        add.setVisible(true);
                        edit.setVisible(true);
                        listb.setVisible(false);
                        save.setVisible(false);
                        af.setVisible(false);
                        artist.setVisible(false);
                    }});

        save.addActionListener(
                new ActionListener(){
                    public void actionPerformed(ActionEvent event){
                        //takes the afart, and save it to the JList(first passed to addString)
                        String getart = artist.getText();
                        String getaf = af.getText();
                        String afart = (getaf+" - "+getart);
                        //pass afart to addString method
                        addString(getaf,getart);
                        af.setText(null);
                        jl.requestFocus();
                        artist.setText(null);
                        //set the window back to "list of songs"
                        jl.setVisible(true);
                        sp.setVisible(true);
                        add.setVisible(true);
                        edit.setVisible(true);
                        listb.setVisible(false);
                        save.setVisible(false);
                        af.setVisible(false);
                        artist.setVisible(false);
                        }});
        //adds another mouselistener to jl
        jl.addMouseListener(
                new MouseAdapter(){

                    public void mousePressed(MouseEvent e)  {check(e);}
                    public void mouseReleased(MouseEvent e) {check(e);}
                    //mouse event right click
                    public void check(MouseEvent e) {
                        if (e.isPopupTrigger()) { //if the event shows the menu
                            jl.setSelectedIndex(jl.locationToIndex(e.getPoint())); //select the item
                            //creates a popupmenu.
                              JPopupMenu jpo = new JPopupMenu();
                                //creates a item that links to popupmenu.. JMenuItem works like a button
                              //this JMenuItem is a remove button
                              JMenuItem ite = new JMenuItem("remove");
                                jpo.add(ite);
                                jpo.show(jl, e.getX(), e.getY()); //and show the menu  
           //JMenuItem actionListener.         
          ite.addActionListener(
                  new ActionListener(){
                      public void actionPerformed(ActionEvent e){
                         //takes selectedIndex, and remove it from the two arraylist, + the Modellist(jlist)
                          final int i = jl.getSelectedIndex();
                            if(i>=0){
                                m.removeElementAt(i);
                                fl.remove(i);
                                art.remove(i);
                            }}});}}});
    //ActionListener for the back button in the edit menu
        editlist.addActionListener(
                new ActionListener(){
                    public void actionPerformed(ActionEvent e){
                        jl.setVisible(true);
                        sp.setVisible(true);
                        add.setVisible(true);
                        edit.setVisible(true);
                        editlist.setVisible(false);
                        editsave.setVisible(false);
                        af.setVisible(false);
                        artist.setVisible(false);
                        af.setText(null);
                        artist.setText(null);
                    }});
        //ActionListener for the save buttin in the edit menu
        editsave.addActionListener(
                new ActionListener(){
                    public void actionPerformed(ActionEvent e){
                        //takes 2 string, and the int from getSelected index, and pass it to editSong(); method.
                        String aff = af.getText();
                        String artt = artist.getText();
                        final int f = jl.getSelectedIndex();
                        //System.out.println(f);
                        editSong(f,aff,artt);
                        //close the edit window
                        jl.setVisible(true);
                        sp.setVisible(true);
                        add.setVisible(true);
                        edit.setVisible(true);
                        editlist.setVisible(false);
                        editsave.setVisible(false);
                        af.setVisible(false);
                        artist.setVisible(false);
                    }});
    }   
    //addString method adds new string to JList, and put them at the next avaiable index.
    public void addString(String o, String l){
        //adds the songname and artistname to the arratlist.
        fl.add(o);
        art.add(l);
        String p = (o+" - "+l);
        //adds the artists+songname to the jlist.
        m.addElement(p.toString()); 
    }
    public void editSong(int i, String song, String artt){
        String s = song;
        String a = artt;
        String sa = (s+" - "+a);
        //fl.add(i,null);
        //remove object at the indexnumber "i"(current index selected) from arraylists.
        fl.remove(i);
        art.remove(i);
        //adds the new string passed in from "editsave", and put them to selectedIndex..
        fl.add(i,s);
        art.add(i,a);
        //remove old JList element, and put in the new.
        m.removeElementAt(i);
        m.add(i,sa);
        }
}
\$\endgroup\$
6
\$\begingroup\$

For cleaning up your code a bit,

  1. Avoid using abbreviations for your variables. If some other coder comes along and looks halfway through your massive class file, they will think what is a 'js'?
  2. Try to create methods for repeating code. For example, your adding lots of JButtons repeatedly. I'm not a UI expert but I think you can try something like this...

    private void addJButton(JButton button, String name, Rectangle rectangle){
       button = new JButton(name);
       button.setBounds(rectangle.getBounds());     
       add(button);     
    }
    

Then your code will look like this, which is much more readable, and will cut out about 3 lines of code per JButton (Note: some JButtons your setting visibility, you could overload a method for those as well, or include a boolean in above method for handling that in ALL JButtons)

addJButton(add, "add", new Rectangle(20, 180, 80, 20));
addJButton(edit, "edit", new Rectangle(110, 180, 80, 20));

As far as seperating your Action listeners, I agree with Roberts above comment. If you decide not to place them in a seperate file, I would at least seperate them into a private method, something like private void setActionListeners.

In general I like to modularize my code so it reads as close to this as possible...

public void someMainMethod(){
   initialize();
   calculations();
   someMoreCalculations();
   iReadLikeABook();
}
\$\endgroup\$
5
\$\begingroup\$

UI code is usually kinda "messy" you could separate out the ActionListeners to new files if you want. Also I'd probably put Artist and Song Title into a class like

public class SongData {
    private String artist;
    private String title;
    public SongData( String title, String artist ) {
        this.artist = artist;
        this.title = title;
    }
}

And then maybe throw those in your array or use an ArrayList to store them. Or if you wanted to look them up later by either field you could put them in a Map

\$\endgroup\$
5
\$\begingroup\$

You are calling a variation of this code five times!

jl.setVisible(false);
sp.setVisible(false);
add.setVisible(false);
edit.setVisible(false);
listb.setVisible(true);
save.setVisible(true);
af.setVisible(true);    
artist.setVisible(true);

The only difference being that you switch false to true and vice-versa on some of the calls.

I would create a method for this,

private void applyVisibilities(boolean sectionAVisible) {
    jl.setVisible(sectionAVisible);
    sp.setVisible(sectionAVisible);
    add.setVisible(sectionAVisible);
    edit.setVisible(sectionAVisible);
    listb.setVisible(!sectionAVisible);
    save.setVisible(!sectionAVisible);
    af.setVisible(!sectionAVisible);    
    artist.setVisible(!sectionAVisible);
}

Then you can replace these lines in the rest of your code with applyVisibilities(true) or applyVisibilities(false).

I suggest naming the sectionAVisible parameter to something that makes more sense for you.


In addition to this, I also suggest adding final to all possible fields in your class. Once your JButtons and JTextFields has been created, they shouldn't be recreated again, right?

private final JButton editlist;
\$\endgroup\$
4
\$\begingroup\$

Just two quick notes:

  1. A great comment from @tb-'s answer:

    Do not extend JPanel, instead have a private JPanel attribute and deal with it (Encapsulation). There is no need to give access to all JPanel methods for code dealing with a UserPanel instance. If you extend, you are forced to stay with this forever, if you encapsulate, you can change whenever you want without taking care of something outside the class.

    (Substitute JPane with JFrame.)

  2. The following fields could be private:

    DefaultListModel<String> m = new DefaultListModel<String>();
    //creates arraylists
    List<String> fl = new ArrayList<String>();
    List<String> art = new ArrayList<String>();
    

    (Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.)

\$\endgroup\$

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.