0

Ok guys. a newbie here. Just created this thing where the user selects a food from the 'Home List' and clicks on the '>>' button to add it to the list on the left which is the 'Shopping List' and vice versa. It works well although it starts getting a bit dodgy when the user clicks on the button after selecting it. It prints out the whole list again and also it appears as an array. I just want the selected values be added to the JList. Heres the code:

import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.*;
import java.util.*;

import javax.swing.JTextField;

public class MAIN extends JFrame {

    Button ltor, rtol;
    JList homelist, shoppinglist;
    DefaultListModel homefoodlist = new DefaultListModel();
    DefaultListModel shoppingfoodlist = new DefaultListModel();
    JTextField foodlog;

    String[] hfood = {"Tuna", "Mayo", "Ketchup", "Sun Flower Oil", "Buscuits", "Cookies", "Turkey"};
    String[] sfood = {"Chocolate", "bread", "Milk", "Toast", "Beef", "Chicken"}; 

    public static void main(String[] args) {

        new MAIN();

    }

    private MAIN(){
        JPanel thepanel = new JPanel();
        thehandler handler = new thehandler();

        this.setLocationRelativeTo(null);
        this.setSize(400, 400);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        this.setVisible(true);
        this.setTitle("Shopping List");
        this.add(thepanel);

        //Creating the Home List(left list)
        for(String homefood: hfood){
            homefoodlist.addElement(homefood);
        }

        homelist = new JList(homefoodlist);
        homelist.setSelectionMode(ListSelectionModel.MULTIPLE_INTERVAL_SELECTION);
        thepanel.add(homelist);

        //Buttons for moving lists from left to right
        ltor = new Button(">>");
        thepanel.add(ltor);
        ltor.addActionListener(handler);

        rtol = new Button("<<");
        rtol.addActionListener(handler);
        thepanel.add(rtol);

        //Creating the Shopping list(right list)
        for(String shoppingfood: sfood){
            shoppingfoodlist.addElement(shoppingfood);
        }
        shoppinglist = new JList(shoppingfoodlist);
        shoppinglist.setSelectionMode(ListSelectionModel.MULTIPLE_INTERVAL_SELECTION);
        thepanel.add(shoppinglist);

    }

        //ActionListener

        private class thehandler implements ActionListener{
            public void actionPerformed(ActionEvent e){
                //The HomeList to the ShoppingList
                if(e.getSource() == ltor){
                    if(homelist.isSelectionEmpty() == false){
                    shoppingfoodlist.addElement(homefoodlist);
                    homefoodlist.remove(homelist.getSelectedIndex());
                    }else{
                        JOptionPane.showMessageDialog(null, "Select a food from either list");
                    }


                }
                if(e.getSource() == rtol){
                    if(shoppinglist.isSelectionEmpty() == false){
                        homefoodlist.addElement(shoppingfoodlist);
                        shoppingfoodlist.remove(shoppinglist.getSelectedIndex());
                        }else{
                            JOptionPane.showMessageDialog(null, "Select a food from either list");
                        }
                }
            }
        }

}
Tloz
  • 307
  • 1
  • 3
  • 12
  • 1
    is it possible that you are introducing the whole list instead of the selected element? `shoppingfoodlist.addElement(homefoodlist);` – iberbeu Oct 22 '13 at 19:41

2 Answers2

0

Just as iberbeu pointed out I think you wanted to move just the selected items from one side to the other, and since you allow multiple selection you should iterate and add all the selected items:

//ActionListener
private class TheHandler implements ActionListener{
    public void actionPerformed(ActionEvent e){
        //The HomeList to the ShoppingList
        if(e.getSource() == ltor){
            if(homelist.isSelectionEmpty()){
                JOptionPane.showMessageDialog(null, "Select a food from either list");
            }else{
                for(int i : homelist.getSelectedIndices()){
                    shoppingFoodList.addElement(homeFoodList.get(i));
                    homeFoodList.remove(i);
                }
            }


        }
        if(e.getSource() == rtol){
            if(shoppingList.isSelectionEmpty()){
                JOptionPane.showMessageDialog(null, "Select a food from either list");
            }else{
                for(int i: shoppingList.getSelectedIndices()){
                    homeFoodList.addElement(shoppingFoodList.get(i));
                    shoppingFoodList.remove(i);
                }
            }
        }
    }
}

While at it, if I may, I'd like to give you a couple of suggestions:

  • according to java's naming convention, classes should be camel-cased: Main, TheHandler, etc and variables lower-camel-cased: shoppingFoodList, homeFoodList, etc. You can read more here
  • this is debatable because everybody has their own way of doing it. But I believe to make the code more readable you should only negate conditions if absolutely necessary (like if you don't need an else branch), otherwise just put the simplest condition in the if and go with the flow and describe your logic like I did above with the message dialog
  • once you get the code to work, you can start refactoring stuff, like the if blocks common and duplicate for both the actions performed by >> and << but I guess this might be too much info right now

Good luck leveling up from newb to pro

Community
  • 1
  • 1
Morfic
  • 15,178
  • 3
  • 51
  • 61
0

You need to replace in you ActionListener following code:

shoppingfoodlist.addElement(homefoodlist);
homefoodlist.remove(homelist.getSelectedIndex());

into:

List selectedValues = homelist.getSelectedValuesList();
for (Object object : selectedValues) {
    shoppingfoodlist.add(0,object);
    homefoodlist.remove(homelist.getSelectedIndex());
}

And also the second part:

homefoodlist.addElement(shoppingfoodlist);
shoppingfoodlist.remove(shoppinglist.getSelectedIndex());

into:

List selectedValues = shoppinglist.getSelectedValuesList();
for (Object object : selectedValues) {
    homefoodlist.add(0,object);
    shoppingfoodlist.remove(shoppinglist.getSelectedIndex());
}

As you can see you have to make changes in two places. I suggest to refactor your listener code a bit to get rid of conditional logic and duplication using approach like this:

class CustomActionListener implements ActionListener{
    JList source;
    JList sink;
    CustomActionListener(JList source, JList sink){
        this.source = source;
        this.sink = sink;
    }
    @Override
    public void actionPerformed(ActionEvent e) {
        // TODO Auto-generated method stub
        if( !source.isSelectionEmpty() ){
            List selectedValues = source.getSelectedValuesList();
            for (Object object : selectedValues) {
                DefaultListModel sinkModel =  (DefaultListModel) this.sink.getModel();
                sinkModel.add(0, object);
                DefaultListModel sourceModel = (DefaultListModel) this.source.getModel();
                sourceModel.remove(source.getSelectedIndex());
            }       
        }
    }

}

Then you can use it as follows:

ltor.addActionListener(new CustomActionListener(homelist,shoppinglist));
rtol.addActionListener(new CustomActionListener(shoppinglist,homelist));

The code is cleaner and you don't have to maintain two places ( one if(ltor) and second if(rtol) ) in your ActionListener.

Luke
  • 1,236
  • 2
  • 11
  • 16