2
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ which one of the if statements bothers you? \$\endgroup\$ – Pacane Nov 7 '14 at 21:25
  • \$\begingroup\$ I am sorry I meant to add I will have to write (unless I find another way) if statement for each individual item (which will be 100+ individual items) and I am sure 100+ if statements is just bad programming \$\endgroup\$ – andyADD Nov 7 '14 at 21:29
4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ Ok what I did was I add them to be private and final, I put the closest to the first place it was being declared. Thanks for the tip! \$\endgroup\$ – andyADD Nov 7 '14 at 21:59
  • \$\begingroup\$ thanks for all the help, but would you know what alternatives I would use besides if statement? I will be addings easily 100 items to this, and I feel 100+ if statements will be inefficient. \$\endgroup\$ – andyADD Nov 7 '14 at 22:11
  • \$\begingroup\$ @janos if you can view my message above yours, and let me know, and thank you very informative, and helpful pacane thanks for the help, I didn't think those could be flipped to avoid NPE. \$\endgroup\$ – andyADD Nov 7 '14 at 22:12
  • \$\begingroup\$ @andyADD if you have so many items then you might want to create a Map<String, Integer> to store the mapping of item names to resource ids. \$\endgroup\$ – janos Nov 7 '14 at 22:14
  • \$\begingroup\$ @andyADD I think the only alternatives are: a switch, or adding the string in an item object representing the item (but you would still have to write all of this) but this last alternative does respect the Open-closed principle. \$\endgroup\$ – Pacane Nov 7 '14 at 22:14
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I have changed variable parameters into words instead of one letters. \$\endgroup\$ – andyADD Nov 7 '14 at 22:03
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer, as quick "hack" I just pretty much just named the files same as the list and when I clicked on item it would remove the spaces and then find the file like that. \$\endgroup\$ – andyADD Dec 26 '14 at 5:30
  • \$\begingroup\$ @andyADD While that does work that isn't a reliable method. I would consider something different. \$\endgroup\$ – Ryan Printup Dec 26 '14 at 23:47
  • \$\begingroup\$ I haven't got a chance to try yours I have been busy with things. I will try your method soon. \$\endgroup\$ – andyADD Dec 28 '14 at 0:16

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.