0

First thing, this code works. Essentially I check for the position clicked in a listview with an if statement, and then I run another check to compare drawables in order to display the proper text (So if a user clicks on an image of a frog, I will populate the EditText field with the text "Frog"). One major issue I have had is that the image is a random image, which ultimately led me to this monster piece of code. 

if(position==0) {

     if (username.getDrawable().getConstantState().equals(getResources().getDrawable(R.drawable.image06).getConstantState())) {
          Toast.makeText(ChatRoom2.this, "Key", Toast.LENGTH_LONG)
                       .show();
          String next = "<font color='#13b602'>@Key </font>";
          editText.append(Html.fromHtml(next));

          editText.setSelection(editText.getText().length());
     }  if (username.getDrawable().getConstantState().equals(getResources().getDrawable(R.drawable.image03).getConstantState())) {

          String next = "<font color='#13b602'>@BlueCube</font>";
          editText.setText(Html.fromHtml(next));

          editText.setSelection(editText.getText().length());
     }
}

That is the check for the first position, and only two nested ifs for two images - I have 30 images to check, so each position would have 30 if statements, and 10 positions. I have a feeling that my solution to this issue is quite poor, as I would need to create 500+ lines of code for this to simply work. Is there any way to create a separate class that just holds the if statements?

M A
  • 65,721
  • 13
  • 123
  • 159
ZombiePartie
  • 27
  • 6
  • 1
    if you've got repeated variables like `image01, image02, ... imageN`, then you should probably consider using an array, so you can do `image[1]`, `image[2]`, etc... which is trivial to loop over. – Marc B Jul 14 '14 at 19:11
  • I would still need many if statements with that change though... – ZombiePartie Jul 14 '14 at 19:18
  • yes, but you could put all your rules into a method pointer. `obj->image[i]->do_your_thing();`. – Marc B Jul 14 '14 at 19:20
  • Generalize the logic, put it in a method that takes _parameters_. – GriffeyDog Jul 14 '14 at 19:45
  • Yea I was thinking about putting the if statements in a method but don't know how to set that up properly. Any tutorials on setting up a method that contains only if statements? – ZombiePartie Jul 14 '14 at 19:59

2 Answers2

1

So I don't code with Android so I'm not even positive exactly what your code is doing but I recognize the pattern you are following and you are correct in recognizing that your design is flawed.

The solution you are following is causing a lot of code duplication which can be eliminated if you follow good Object Oriented practices.

1) Separate out your action using classes and methods. For instance, you are constantly calling this same if statement so that you can contain all of your action in one class. This type of "main" methodology is more of a procedural programmers way of thinking and as you can see can cause you a lot of problems. Instead try splitting unique behavior into it's own class or method and then call that when you need it - such as taking all of those duplicate If clauses and putting them in a single method so it can be listed once but be called however many times you need it.

2) Make proper use of your data structures. I see mention of repeated positioning and images with assigned text. All of this information you would want to store in a data structure (such as a hashmap) or in a class specifically designed for your object (which may contain a data structure as a variable). For instance, you can create an Image class that has a specific text variable which can be returned. Then you can data load your images and each can be assigned its appropriate text (rather than say hard-coding those values). Or you may have a Position class which has an ArrayList of possible position values that can be assigned vertically so that you can reuse it wherever needed.

3) If you really have detailed logic that must be listed out separately and has a common factor, try using a switch statement as opposed to a series of If/Then statements. This is cleaner and reduces the duplication that would otherwise be found in your many if clauses.

Just remember that if you think your design is ugly... there is most likely a way to make it better. Even the best code out there can usually be optimized to eliminate duplication and speed performance. For additional ideas of how to optimize your design, check out these design patterns.

Hope that helps!

DanK
  • 2,282
  • 5
  • 16
  • 35
  • Yea I knew I had to make a method but the issue is that all of this has to happen inside the onItemClickListener, so I cannot create a method outside of that since the variables are within onItemClickListener. Been searching but cannot find a way to create a method inside an onItemClickListener yet. – ZombiePartie Jul 14 '14 at 22:23
  • You might want to look into MVP and MVC (http://stackoverflow.com/questions/2056/what-are-mvp-and-mvc-and-what-is-the-difference) as these are patterns to split your logic from your UI layer. I'm not familiar with onItemClickListener but I'm having a hard time believing you are unable to separate your business logic from this method. You should be able to build your entire system outside of your UI and then call the appropriate methods from inside the listener just as you would a Unit Test case. – DanK Jul 14 '14 at 22:33
0

Turns out that I needed a do-while loop inside a for loop to get this cleaner!

ZombiePartie
  • 27
  • 6