Personal Minecraft Android app

Question

This won't be released publicly, but that doesn't mean I want to have bad coding habits. I was not too far into this before realizing I could make it better. One of my main concerns is finding an alternative to the if statement halfway down. The toast message is temporary as I was having trouble with something earlier in my development (48 hours ago).

package com.andyroo.mcrecipefinder;

import android.content.Context;
import android.support.v7.app.ActionBarActivity;
import android.os.Bundle;
import android.text.Editable;
import android.text.TextWatcher;
import android.view.Menu;
import android.view.MenuItem;
import android.view.View;
import android.widget.AdapterView;
import android.widget.AlphabetIndexer;
import android.widget.ArrayAdapter;
import android.widget.EditText;
import android.widget.ImageView;
import android.widget.ListView;
import android.widget.TextView;
import android.widget.Toast;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Random;
import java.util.StringTokenizer;


public class MyActivity extends ActionBarActivity {

    /** Defined variables START **/
    ListView listView;
    ImageView image;
    EditText editsearch;
    List<String> recipes;
    TextView itemname;
    /** Defined variables END**/


    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_my);
        /** Defined Variables START **/
        listView = (ListView) findViewById(R.id.list);
        image = (ImageView) findViewById(R.id.imageView);
        editsearch = (EditText) findViewById(R.id.editText);
        itemname = (TextView) findViewById(R.id.textView3);

        /** Defined Variables END **/





        /** Array listview START **/
        recipes = new ArrayList<String>();
        String blocks[] = {"Chest","Bed","Wool","Wood Planks","Redstone Torch","Redstone Lamp","Redstone Repeater"};
        for(int blockC = 0; blockC < blocks.length; blockC++){
            recipes.add(blocks[blockC]);
        }
        /** Array listview END **/

        String mcOreRecipe[] = {"Wood", "Leather", "Stone", "Iron", "Gold", "Diamond"};
        String armorType[] = {"Chestplate", "Leggings", "Helmet", "Boots", "Hatchet",
                              "Sword", "Pickaxe", "Shovel", "Hoe"};

        /** compile iron, diamond, & gold weapons/armor/tools **/
        for (int num2 = 3; num2 < 6; num2++){
            for (int num3 = 0; num3 < 9; num3++){
                recipes.add(mcOreRecipe[num2]+" "+armorType[num3]);
            }
        }

        /** compile leather armor **/
        for(int num = 0; num < 4; num++){

                recipes.add(mcOreRecipe[1]+" "+armorType[num]);


        }

        /** compile wood/stone weapons/tools **/
        for(int num1 = 4; num1 < 9; num1++){
            recipes.add(mcOreRecipe[2]+" "+armorType[num1]);
            recipes.add(mcOreRecipe[0]+" "+armorType[num1]);
        }





        Collections.sort(recipes);



        ArrayAdapter<String> adapter = new ArrayAdapter<String>(this , android.R.layout.simple_list_item_1 , recipes);
        listView.setAdapter(adapter);
        listView.setFastScrollEnabled(true);

        listView.setOnItemClickListener(new AdapterView.OnItemClickListener() {
            @Override
            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
                //String item = ((TextView)view).getText().toString();

                // ListView Clicked item index
                int itemPosition     = position;

                // ListView Clicked item value
                String  itemValue    = (String) listView.getItemAtPosition(position);

                // Show Alert
                Toast.makeText(getApplicationContext(),
                        "Position :"+itemPosition+"  ListItem : " +itemValue , Toast.LENGTH_LONG)
                        .show();






                if (itemValue.compareTo("Chest") == 0){

                    image.setImageResource(R.drawable.recipe_chest);
                    itemname.setText(itemValue);
                }
                if (itemValue.compareTo("Redstone Torch") == 0){

                    image.setImageResource(R.drawable.recipe_redstonetorch);
                    itemname.setText(itemValue);
                }
                if (itemValue.compareTo("Gold Chestplate") == 0){

                    image.setImageResource(R.drawable.recipe_goldchestplate);
                    itemname.setText(itemValue);
                }

            }
        });

        editsearch.addTextChangedListener(new TextWatcher() {
            @Override
            public void beforeTextChanged(CharSequence s, int start, int count, int after) {

            }

            @Override
            public void onTextChanged(CharSequence s, int start, int before, int count) {

                ArrayList<String> temp = new ArrayList<String>();
                int textlength = editsearch.getText().length();
                temp.clear();
                for (int i = 0; i < recipes.size(); i++){
                    if (textlength <= recipes.get(i).length()){
                        if(editsearch.getText().toString().equalsIgnoreCase((String) recipes.get(i).subSequence(0,textlength))){
                            temp.add(recipes.get(i));
                        }
                    }
                }
                ArrayAdapter<String> tempadapter = new ArrayAdapter<String>(MyActivity.this, android.R.layout.simple_list_item_1, temp);
                listView.setAdapter(tempadapter);

            }

            @Override
            public void afterTextChanged(Editable s) {

            }
        });

    }


    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        // Inflate the menu; this adds items to the action bar if it is present.
        getMenuInflater().inflate(R.menu.my, menu);
        return true;
    }

    @Override
    public boolean onOptionsItemSelected(MenuItem item) {
        // Handle action bar item clicks here. The action bar will
        // automatically handle clicks on the Home/Up button, so long
        // as you specify a parent activity in AndroidManifest.xml.
        int id = item.getItemId();
        if (id == R.id.action_settings) {
            return true;
        }
        return super.onOptionsItemSelected(item);
    }
}

Answer 1

Don't do this:

/** Defined variables START **/
ListView listView;
ImageView image;
EditText editsearch;
List<String> recipes;
TextView itemname;
/** Defined variables END**/

This looks as if you're transitioning from being a C developer to Java. In more modern languages you are not required to declare at the top the variables that you use. And in fact you shouldn't. Variables should be declared as close as possible to where they are used. It makes the code more readable.

A related topic is to limit variables to the smallest possible scope. For example, in the posted code listView is a field of MyActivity, but it's only used within the onCreate method. Therefore it should be a local variable in that method. It seems all the other fields could have been local variables.

When you do need fields, make them private unless you have a specific need to make them more visible. And when possible, make them final, to avoid accidental modifications to their values.

---

Don't do this:

for(int blockC = 0; blockC < blocks.length; blockC++){
    recipes.add(blocks[blockC]);
}

When you don't need the item indexes, you should use a for-each loop instead:

for (String block : blocks) {
    recipes.add(block);
}

---

Don't do this:

if (itemValue.compareTo("Chest") == 0){
    // ...
}
if (itemValue.compareTo("Redstone Torch") == 0){
    // ...
}
if (itemValue.compareTo("Gold Chestplate") == 0){
    // ...
}

Use the .equals method to compare strings:

if (itemValue.equals("Chest")) {

Since these conditions are mutually exclusive, you should use else-if instead:

if (itemValue.equals("Chest")) {
    // ...
} else if (itemValue.equals("Redstone Torch")) {
    // ...
}
// ... and so on

Lastly, to be extra safe, and protect yourself from possible null values of itemValue, it would be better to flip the .equals calls to put the non-null string literals on the left side, like this:

if ("Chest".equals(itemValue)) {
    // ...
} else if ("Redstone Torch".equals(itemValue)) {
    // ...
}
// ... and so on

Answer 2

First of all, I think you should try to respect coding conventions such as indentation, white space, naming, etc.

About the name, if I were you I'd try to use meaningful names (ie: avoid one letter variables for parameters like s for CharSequence).

I would also try to name those temp variables. They probably represent something real anyway.

Finally for you bothering if statement, this is a pretty common pattern that would possibly lead to polymorphism. Usually you would pull each individual check in the appropriate object. However here you're using Strings instead of an actual Item object. Maybe you could consider turning these wild Strings into some Item class.

Answer 3

To address the issue of your 100+ item if statement I think it is best if you use an Enum to store all you items.

public enum Item {
    REDSTONE_TORCH("Redstone Torch",  R.drawable.recipe_redstonetorch) , 
    CHEST("Chest", R.drawable.recipe_chest) ;
    // Etc

    private static final String NAME;
    private static final int DRAWABLE_RES_ID;

    public Items(String name, int drawableResId) {
        this.NAME = name;
        this.DRAWABLE_RES_ID = drawableResId;
    }

   public static Item getItemByName(String name) {
       for (Item item : Item.values()) {
           if (name.equals(item.getName()) {
               return item;
           }
       }

       return null;
   }

    public String getName() {
        return NAME;
    }

    public int getDrawableResId() {
        return DRAWABLE_RES_ID;
    }
}

The getItemByName method simply loops through all of the items checking against the name and returning the item if it matches and null if nothing is found. Then in your Activity code you can do this.

Item item = Item.getItemByName(itemValue);

if (item != null) {
    image.setImageResource item.getDrawableResId());
    itemname.setText(item.getName());
} else {
    // Item does not exist
}

This condenses a 100+ line if statement in an easy couple lines. It is also very extendable, if you want to add a new item just add a new entry in the Enum.