1
I would like to know how to optimize a large chain of ifs so that the code becomes more efficient and readable.
My Android application uses version 2.3.3 (API 10), in this application I have an Activity responsible for displaying user account information, where it can edit this information and update. This Activity contains 8 fields beside the field has 1 "Edit" button and a hidden "OK" button, if the "Edit" button is clicked then the application hides "Edit" and displays "OK" in the same place. So I have a total of 16 buttons, 8 "Edit" and 8 "OK". That left my onClick(View v) method with 16 ifs.
How can I improve this and make it more efficient and more readable? I know I can use the switch that would be almost the same.
The code below has the method with only 2 of the 16 buttons to simplify, since the other buttons follow the same logic.
public void onClick(View v) {
// TODO Auto-generated method stub
if(v.getId() == R.id.btneditar1) {
camponome.setSelectAllOnFocus(true);
camponome.setFocusable(true);
camponome.setFocusableInTouchMode(true);
camponome.requestFocus();
imm.showSoftInput(camponome, InputMethodManager.SHOW_FORCED);
btneditar1.setVisibility(View.GONE);
btnok1.setVisibility(View.VISIBLE);
}
if(v.getId() == R.id.btnok1) {
CampoValido = ValidaCampo.validateNotNull(camponome, "Campos sem dados são inválidos!");
if(CampoValido) {
CampoValido = ValidaCampo.validateNotEqual(camponome, bnome);
if(CampoValido) {
final String nome = camponome.getText().toString().trim();
atualizar.execute(usuario, nome, null, null, null, null, null, null);
txtnomebd.setText(nome);
}
camponome.setFocusable(false);
camponome.setFocusableInTouchMode(false);
camponome.requestFocus();
btnok1.setVisibility(View.GONE);
btneditar1.setVisibility(View.VISIBLE);
}
}
if(v.getId() == R.id.btneditar2) {
camposobrenome.setSelectAllOnFocus(true);
camposobrenome.setFocusable(true);
camposobrenome.setFocusableInTouchMode(true);
camposobrenome.requestFocus();
imm.showSoftInput(camposobrenome, InputMethodManager.SHOW_FORCED);
btneditar2.setVisibility(View.GONE);
btnok2.setVisibility(View.VISIBLE);
}
if(v.getId() == R.id.btnok2) {
CampoValido = ValidaCampo.validateNotNull(camposobrenome, "Campos sem dados são inválidos!");
if(CampoValido) {
CampoValido = ValidaCampo.validateNotEqual(camposobrenome, bsobrenome);
if(CampoValido) {
final String sobrenome = camposobrenome.getText().toString().trim();
atualizar.execute(usuario, null, sobrenome, null, null, null, null, null);
txtsobrenomebd.setText(sobrenome);
}
camposobrenome.setFocusable(false);
camposobrenome.setFocusableInTouchMode(false);
camposobrenome.requestFocus();
btnok2.setVisibility(View.GONE);
btneditar2.setVisibility(View.VISIBLE);
}
}
}
Try to use switch case... for all ifs of the code. Now there is a lot of content there that is repeated in various functions, optimizing this would create a new logic for you, and we don’t even know your program.
– user28595
@Diego Felipe As said in the question the switch is almost the same thing will not optimize anything.
– Gustavo Almeida Cavalcante
What do you mean by "optimize"? The big problem of the code is not even the ifs, but the number of repeated functions within some conditions. And as I said, optimizing this would be almost the same as recreating your logic. It seems that you can use Arraylist objects as well.
– user28595
@Diego Felipe Yes I had to repeat the functions, because each one besides affecting a different field, it affects a different button. But it is a good idea to make a single method where I pass as parameter the field and button.
– Gustavo Almeida Cavalcante
I have an idea how to solve this, but I do not know if it is optimized, I will post here in the comments and you take a look
– user28595
@Diego Felipe ok.
– Gustavo Almeida Cavalcante
@Gustavoalmeidacavalcante: Are you superimposing the onClick method of Activity? What I see most is something like
findViewById(id).setOnClickListener(new View.OnClickListener() { ... seu código ...} );, which I consider more readable. Anyway, it would be a great advance to separate these bodies from the ifs in their properly appointed private functions. As for performance, unless you’ve noticed some slowness in that part of the code, don’t worry about it hastily.– Pablo Almeida
@Pablo If I were to put the Listener on each button in this way, it would increase the size of the code significantly. I don’t know if it would be more readable, but because it greatly increases the size of the code I prefer to centralize the listener of all buttons in one method.
– Gustavo Almeida Cavalcante
@Gustavoalmeidacavalcante I prefer for debugging, but this point is really matter of taste. But the other two advices I suggest you consider.
– Pablo Almeida
@Gustavoalmeidacavalcante consider using switch or use if.... if Else. The way it is every time the user clicks a button the 16 ifs will be consumed.
– Arubu
@Arubu You’re right at this point, the way it consumes the 16 ifs. If I’m really going to use ifs I’ll do it.
– Gustavo Almeida Cavalcante
@Pablo When you think about your idea of putting the Listener on each button with your method the way you said, it might even increase the size of the code, but I think this way it will consume less processing resources since it will not check which button was pressed but will directly execute what is in the button method. However my onCreate() will get bigger and when Activity is started it will consume more resources at that time. Good is just what I think, I’m not sure.
– Gustavo Almeida Cavalcante
@Yes, it probably does. But the difference is probably so small that, for all intents and purposes, it is the same thing. I suggest you stop worrying about performance that way unless you actually measure some difference. There are tools called profilers that serve to find where are the "bottlenecks" of your application. Use them in these situations and, while it is not necessary, worry more about readability that thus you gain more time in the medium and long term.
– Pablo Almeida
Checking these 16 ifs is virtually nothing for current processors. Focus on what @Pablo said.
– Arubu