Android ListView adapter loads wrong

Question

For some reason when I use this code it doesn't work properly. Instead of putting each crossroad in its place, it puts the last one in the first and the rest are empty.

package com.Itay.lightalaram.lightalarm;

import android.app.Activity;
import android.os.Bundle;
import android.view.View;
import android.view.ViewGroup;
import android.widget.ArrayAdapter;
import android.widget.ListView;
import android.widget.TextView;

import java.util.ArrayList;
import java.util.List;

public class CrossroadsActivity extends Activity {
    private List<Crossroad> crossroads;
    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.crossroads);

        crossroads = new ArrayList<Crossroad>();

        populateListView();
    }

    private void populateListView() {
        ListView list = (ListView) findViewById(R.id.crossroadsListView);
        crossroads.add(new Crossroad("Oranim", 50, 30));
        crossroads.add(new Crossroad("Evelina", 40, 85));
        crossroads.add(new Crossroad("Sacker", 43, 22));
        crossroads.add(new Crossroad("bagger", 43, 22));


        ArrayAdapter<Crossroad> adapter = new CrossroadAdapter();

        list.setAdapter(adapter);
    }

    private class CrossroadAdapter extends ArrayAdapter<Crossroad> {
        public CrossroadAdapter(){
            super(CrossroadsActivity.this, R.layout.item_crossroad, crossroads);
        }

        @Override
        public View getView(int position, View convertView, ViewGroup parent) {
            View itemView = convertView;
            if(null == itemView) {
                itemView = getLayoutInflater().inflate(R.layout.item_crossroad, parent, false);

                Crossroad currentCrossroad = crossroads.get(position);

                TextView name = (TextView) findViewById(R.id.item_crossroadName);
                TextView redLight = (TextView) findViewById(R.id.item_redLightDuration);
                TextView greenLight = (TextView) findViewById(R.id.item_greenLightDuration);
                try {
                    name.setText(currentCrossroad.getName());
                    redLight.setText("" + currentCrossroad.getRedLight());
                    greenLight.setText("" + currentCrossroad.getGreenLight());
                } catch (Exception e) {
                }
                return itemView;
            }
            return null;
        }
    }
}

Output:

Answer 1

Explanation
The issue occurs because you set the inflated layout only once, when your condition of convertView is null. However, after the first time itemView ins't null. Your items are not updated except the first one. Also, you need to return all the views (itemView) for the list items.

Solution
In your adapter, the getView method needs to inflate a layout (once) and returns this layout:

// inflate the layout to the row (items)
View itemView = convertView;
if(null == itemView) {
    itemView = getLayoutInflater().inflate(R.layout.item_crossroad, parent, false);
}
// Then return the inflated view and all its stuff
return itemView;

Also, you need to attach all the item's views to the layout as follows:

 // itemView.findViewById(...) to retrieve the id which is on the inflated layout
 TextView name = (TextView) itemView.findViewById(R.id.item_crossroadName);

Finally, to improve the performance of your Adapter, you should use a ViewHolder. This will avoid you to call ever and ever all the findViewById methods. Here's a great tutorial and a simple example. Then, your method might be:

 // Init your variables
 public ViewHolder {
     TextView name, redLight, greenLight
 }

 @Override
 public View getView(int position, View convertView, ViewGroup parent) {
      View itemView = convertView;
      // create the ViewHolder variable
      ViewHolderItem viewHolder;

      if(itemView == null) {
           itemView = getLayoutInflater().inflate(R.layout.item_crossroad, parent, false);

           // create a new ViewHolder
           viewHolder = new ViewHolderItem();

           // get your views with the ViewHolder and attached to the inflated layout
           viewHolder.name = (TextView) itemView.findViewById(R.id.item_crossroadName);
           viewHolder.redLight = (TextView) itemView.findViewById(R.id.item_redLightDuration);
           viewHolder.greenLight = (TextView) itemView.findViewById(R.id.item_greenLightDuration);

           // store the ViewHolder
           itemView.setTag(viewHolder);
     }else{
         // this is not the first, reuse the stored ViewHolder
         viewHolder = (ViewHolderItem) convertView.getTag();
     }

     // do your stuff...
     Crossroad currentCrossroad = crossroads.get(position);
     try {
         viewHolder.name.setText(currentCrossroad.getName());
         viewHolder.redLight.setText("" + currentCrossroad.getRedLight());
         viewHolder.greenLight.setText("" + currentCrossroad.getGreenLight());
     } catch (Exception e) { }

     // return the layout inflated
     return itemView;
}  

Answer 2

 private class CrossroadAdapter extends ArrayAdapter<Crossroad> {
    public CrossroadAdapter() {
        super(MainActivity.this, R.layout.item_crossroad, crossroads);
    }

    TextView name, redLight, greenLight;

    @Override
    public View getView(int position, View convertView, ViewGroup parent) {
        View itemView = convertView;
        Crossroad currentCrossroad = crossroads.get(position);
        if (null == itemView) {
            itemView = getLayoutInflater().inflate(R.layout.item_crossroad,
                    parent, false);

            name = (TextView) itemView
                    .findViewById(R.id.item_crossroadName);
            redLight = (TextView) itemView
                    .findViewById(R.id.item_redLightDuration);
            greenLight = (TextView) itemView
                    .findViewById(R.id.item_greenLightDuration);

        }
        try {
            name.setText(currentCrossroad.getName());
            redLight.setText("" + currentCrossroad.getRedLight());
            greenLight.setText("" + currentCrossroad.getGreenLight());
        } catch (Exception e) {
        }
        return itemView;
    }
}

Answer 3

change the getview method to

public View getView(int position, View convertView, ViewGroup parent) {
View itemView = convertView;
if(null == itemView) {
    itemView = getLayoutInflater().inflate(R.layout.item_crossroad, parent, false);
}
    Crossroad currentCrossroad = crossroads.get(position);

    TextView name = (TextView) itemView.findViewById(R.id.item_crossroadName);
    TextView redLight = (TextView) itemView.findViewById(R.id.item_redLightDuration);
    TextView greenLight = (TextView) itemView.findViewById(R.id.item_greenLightDuration);

    name.setText(currentCrossroad.getName());
    redLight.setText("" + currentCrossroad.getRedLight());
    greenLight.setText("" + currentCrossroad.getGreenLight());

    return itemView;         

}

comment me about result