2
\$\begingroup\$

I have this piece of code I've been working on for quite some time. It's a ListView with a search filter. Inside each row, it contains a user Image, a User's name and how many friends they have in common. Also the row contains a button each, for actions.

I'm just indeed of some code refactoring, maybe some guidance? Different collections that I could use to tone the heavy code down.

@Override 
public void onCreate(Bundle savedInstanceState)
{
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);

    res = getResources();
    searchField = (EditText) findViewById(R.id.EditText01);
    lv = (ListView) findViewById(android.R.id.list);
    lv.setChoiceMode(ListView.CHOICE_MODE_SINGLE);
    list = new ArrayList<Friend>();
    friendList2 = new ArrayList<Friend>();

    nameBlock = res.getStringArray(R.array.names);
    descBlock = res.getStringArray(R.array.descriptions);

    names = new ArrayList<String>();
    for(int i = 0; i < nameBlock.length; i++) {
        names.add((String)nameBlock[i]);
    }
    descr = new ArrayList<String>();
    for(int i = 0; i < descBlock.length; i++) {
        descr.add((String)descBlock[i]);
    }
    images = new ArrayList<Integer>();
    for(int i = 0; i < imageBlock.length; i++) {
        images.add((Integer)imageBlock[i]);
    }
    //imageBlock = res.getIntArray(R.array.images);

    int size = nameBlock.length;
    for(int i = 0 ; i < size; i++) {
        Log.d("FREINDADD", "Freind Added" + i);
        list.add(new Friend(i, names.get(i), descr.get(i), images.get(i), false));
        friendList2.add(new Friend(i, names.get(i), descr.get(i), images.get(i), false));
        //friendList2.add(new Friend(i, names.get(i), descr.get(i), images.get(i)));
    }
    Log.i("Application", "Application started succesfully...");

    adapter = new CustomAdapter(this);


    setListAdapter(adapter);
    Log.i("VIRTU", "Count" + adapter.getCount());
    //adapter.getCount();


    searchField.addTextChangedListener(new TextWatcher()
    {
        @Override public void afterTextChanged(Editable s) {}
        @Override public void beforeTextChanged(CharSequence s, int start, int count, int after) {}
        @Override public void onTextChanged(CharSequence s, int start, int before, int count)
        {
            list.clear();
            textlength = searchField.getText().length();

            for (int i = 0; i < names.size(); i++)
            {
                if (textlength <= names.get(i).length())
                {
                    if(names.get(i).toLowerCase().contains(searchField.getText().toString().toLowerCase().trim())) {
                        Log.i("VirtuFriendList", "List recyling in process... ");
                        list.add(new Friend(i, names.get(i), descr.get(i), images.get(i), false));

                    }
                  }
            }
            AppendList(list);

            }
    });


}
public void AppendList(ArrayList<Friend> list) {
    setListAdapter(new CustomAdapter(this));
} 

class CustomAdapter extends BaseAdapter {
    private Context context;
    public int pos;
    public CustomAdapter(Context context) {
        this.context = context;

    }
    @Override
    public int getCount() {
        // TODO Auto-generated method stub
        return list.size();
    }

    @Override
    public Object getItem(int position) {
        // TODO Auto-generated method stub
        return list.get(position);
    }

    @Override
    public long getItemId(int position) {
        // TODO Auto-generated method stub
        return list.size();
    }
    class ViewHolder {
        TextView userName;
        TextView userDesc;
        ImageView userImage;
        Button userButton;
        int position;

        ViewHolder(View view) {
            userImage = (ImageView)view.findViewById(R.id.imageview);
            userName = (TextView)view.findViewById(R.id.title);
            userDesc = (TextView)view.findViewById(R.id.mutualTitle);
            userButton = (Button)view.findViewById(R.id.btn);
        }
    }
    ViewHolder holder;
    View row;
    ViewGroup parent;
    int position;
    @Override
    public View getView(final int position, View convertView, ViewGroup parent) {
            row = convertView;
            this.parent = parent;
            this.pos = position;
            if(row == null) 
            {
                // If it is visible to the user, deploy the row(s) - allocated in local memory
                LayoutInflater inflater = (LayoutInflater)context .getSystemService(Context.LAYOUT_INFLATER_SERVICE);
                row = inflater.inflate(R.layout.search_list_item, parent, false);
                holder = new ViewHolder(row);
                row.setTag(holder);

                Log.d("VIRTU", "Row deployed...");
            }
            else 
            {
                // Recycle the row if it is not visible to to the user - store in local memory
                holder = (ViewHolder)row.getTag();
                Log.d("VIRTU", "Row recycled...");
            }
            final Friend temp = list.get(position);

            // Set the resources for each component in the list
            holder.userImage.setImageResource(temp.getImage());
            holder.userName.setText(temp.getName());
            holder.userDesc.setText(temp.getDesc());
            holder.userButton.setTag(position);

            holder.userButton.setOnClickListener(new View.OnClickListener() {

                @Override
                public void onClick(View v) {
                    String text = (String)searchField.getText().toString();

                    searchField.setText("");
                    Integer index = (Integer)v.getTag();
                    Toast.makeText(context, index.intValue() + "..." + temp.getId(), Toast.LENGTH_SHORT).show();
                    try {
                        destroy(list, names, descr, images, temp.getId());      
                    } catch(IndexOutOfBoundsException e) {
                        destroy(list, names, descr, images, index.intValue());
                    }
                    adapter = new CustomAdapter(context);
                    lv.setAdapter(adapter);
                    adapter.notifyDataSetChanged(); 
                    searchField.setText(text);
                    searchField.setSelection(searchField.getText().length());
                }
            });


        return row;
    }
    public void destroy(ArrayList<Friend> list, ArrayList<String> names, ArrayList<String> desc, ArrayList<Integer> images, int position)  {
        list.remove(position);
        names.remove(position);
        desc.remove(position);
        images.remove(position);
    }
}
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Just a few quick thoughts rather than a full review:

  • Your onCreate is too long. Way too long!
  • What is lv? Abbreviations are usually a bad idea. IDEs will do the typing for you, so meaningful names are a must.
  • What is friendList2? Is there a friendList1? Why are there two? Why are they named like this? Names should represent the semantics, not count similar variables.
  • Commented-out code is dead code and dead code rots. Remove it and use something like git instead. The one fear you cannot have is the fear of deleting code.
  • Consistently misspelled: "freind" instead of "friend".
  • Log.d("FREINDADD", "Freind Added" + i); – think about how log messages will look later on. Make them meaningful. This message is just slightly better than useless.
  • Log.i("Application", "Application started succesfully..."); – if it started successfully, then why do we wait for anything? An ellipsis () makes me think a process has started and is waiting now. Why is this log statement in the middle of the code?
  • It is a Java standard to use camelCase, which means a method is named appendList, not AppendList.
  • Use interfaces such as List<T> rather than implementations like ArrayList<T> for types of fields or parameters.
  • Why does AppendList have a parameter, but never uses it?
  • // TODO Auto-generated method stub should be one of the first things to remove. Don't leave dead and useless comments.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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