5
\$\begingroup\$

I'm basically a newbie in android app developing. So I'm not sure if this is the right way to write these codes. I have made an app called "Zconverter". It has 9 fragments and the code i m showing here is the smallest of them all. All the fragments have the same type of code. This app works just fine but i m not sure if this is the best way to write this.

public class Speed extends Fragment {
    public View onCreateView (LayoutInflater inflater, ViewGroup container , Bundle savedInstanceState){
    final ViewGroup rootView = (ViewGroup) inflater.inflate(R.layout.speed_fragment, container, false);


    final EditText editMilesHour = (EditText) rootView.findViewById(R.id.editMilesHour);
    final EditText editfeetsec = (EditText) rootView.findViewById(R.id.editfeetsec);
    final EditText editMeterSec = (EditText) rootView.findViewById(R.id.editMeterSec);
    final EditText editKmHour = (EditText) rootView.findViewById(R.id.editKmHour);
    final EditText editKnot = (EditText) rootView.findViewById(R.id.editKnot);

    TextView textSpeed = (TextView) rootView.findViewById(R.id.textSpeed);
    TextView textKmHour = (TextView) rootView.findViewById(R.id.textKmHour);
    TextView textKnot = (TextView) rootView.findViewById(R.id.textKnot);
    TextView textMilesHour = (TextView) rootView.findViewById(R.id.textMilesHour);
    TextView textfeetsec = (TextView) rootView.findViewById(R.id.textfeetsec);
    TextView textMeterSec = (TextView) rootView.findViewById(R.id.textMeterSec);

    Typeface font = Typeface.createFromAsset(getActivity().getAssets(), "CoffeeHouse.ttf");
    textMeterSec.setTypeface(font);
    textMilesHour.setTypeface(font);
    textKnot.setTypeface(font);
    textKmHour.setTypeface(font);
    textfeetsec.setTypeface(font);
    textSpeed.setTypeface(font);


    editMeterSec.setOnFocusChangeListener(new View.OnFocusChangeListener() {
        @Override
        public void onFocusChange(View v, boolean hasFocus) {
            if(hasFocus){
                Button buttonConvertSpeed = (Button) rootView.findViewById(R.id.buttonConvertSpeed);
                buttonConvertSpeed.setOnClickListener(new View.OnClickListener() {
                                                          @Override
                                                          public void onClick(View v) {
                                                              double MeterSec = Double.valueOf(editMeterSec.getText().toString());
                                                              double MilesHour = MeterSec * 2.23694;
                                                              editMilesHour.setText(String.valueOf(MilesHour));
                                                              double feetsec = MeterSec * 3.28084;
                                                              editfeetsec.setText(String.valueOf(feetsec));
                                                              double KmHour = MeterSec * 3.6;
                                                              editKmHour.setText(String.valueOf(KmHour));
                                                              double Knot = MeterSec * 1.94384;
                                                              editKnot.setText(String.valueOf(Knot));

                                                          }

                                                      }
                );

            }else{

            }

        }
    });


    editMilesHour.setOnFocusChangeListener(new View.OnFocusChangeListener() {
        @Override
        public void onFocusChange(View v, boolean hasFocus) {
            if(hasFocus){
                Button buttonConvertSpeed = (Button) rootView.findViewById(R.id.buttonConvertSpeed);
                buttonConvertSpeed.setOnClickListener(new View.OnClickListener() {
                                                          @Override
                                                          public void onClick(View v) {
                                                              double MilesHour6 = Double.valueOf(editMilesHour.getText().toString());
                                                              double MeterSec6 = MilesHour6 * 0.44704;
                                                              editMeterSec.setText(String.valueOf(MeterSec6));
                                                              double feetsec6 = MilesHour6 * 1.46667;
                                                              editfeetsec.setText(String.valueOf(feetsec6));
                                                              double KmHour6 = MilesHour6 * 1.60934;
                                                              editKmHour.setText(String.valueOf(KmHour6));
                                                              double Knot6 = MilesHour6 * 0.868976;
                                                              editKnot.setText(String.valueOf(Knot6));

                                                          }

                                                      }
                );

            }else{

            }
        }
    });

    editfeetsec.setOnFocusChangeListener(new View.OnFocusChangeListener() {
        @Override
        public void onFocusChange(View v, boolean hasFocus) {
            if(hasFocus){
                Button buttonConvertSpeed = (Button) rootView.findViewById(R.id.buttonConvertSpeed);
                buttonConvertSpeed.setOnClickListener(new View.OnClickListener() {
                                                          @Override
                                                          public void onClick(View v) {
                                                              double feetsec7 = Double.valueOf(editfeetsec.getText().toString());
                                                              double MeterSec7 = feetsec7 * 0.3048;
                                                              editMeterSec.setText(String.valueOf(MeterSec7));
                                                              double MilesHour7 = feetsec7 * 0.681818;
                                                              editMilesHour.setText(String.valueOf(MilesHour7));
                                                              double KmHour7 = feetsec7 * 1.09728;
                                                              editKmHour.setText(String.valueOf(KmHour7));
                                                              double Knot7 = feetsec7 * 0.592484;
                                                              editKnot.setText(String.valueOf(Knot7));

                                                          }

                                                      }
                );

            }else{

            }
        }
    });

    editKmHour.setOnFocusChangeListener(new View.OnFocusChangeListener() {
        @Override
        public void onFocusChange(View v, boolean hasFocus) {
            if(hasFocus){
                Button buttonConvertSpeed = (Button) rootView.findViewById(R.id.buttonConvertSpeed);
                buttonConvertSpeed.setOnClickListener(new View.OnClickListener() {
                                                          @Override
                                                          public void onClick(View v) {
                                                              double KmHour8 = Double.valueOf(editKmHour.getText().toString());
                                                              double MeterSec8 = KmHour8 * 0.277778;
                                                              editMeterSec.setText(String.valueOf(MeterSec8));
                                                              double MilesHour8 = KmHour8 * 0.621371;
                                                              editMilesHour.setText(String.valueOf(MilesHour8));
                                                              double feetsec8 = KmHour8 * 0.911344;
                                                              editfeetsec.setText(String.valueOf(feetsec8));
                                                              double Knot8 = KmHour8 * 0.539957;
                                                              editKnot.setText(String.valueOf(Knot8));

                                                          }

                                                      }
                );

            }else{

            }
        }
    });

    editKnot.setOnFocusChangeListener(new View.OnFocusChangeListener() {
        @Override
        public void onFocusChange(View v, boolean hasFocus) {
            if(hasFocus){
                Button buttonConvertSpeed = (Button) rootView.findViewById(R.id.buttonConvertSpeed);
                buttonConvertSpeed.setOnClickListener(new View.OnClickListener() {
                                                          @Override
                                                          public void onClick(View v) {
                                                              double Knot9 = Double.valueOf(editKnot.getText().toString());
                                                              double MeterSec9 = Knot9 * 0.514444;
                                                              editMeterSec.setText(String.valueOf(MeterSec9));
                                                              double MilesHour9 = Knot9 * 1.15078;
                                                              editMilesHour.setText(String.valueOf(MilesHour9));
                                                              double feetsec9 = Knot9 * 1.68781;
                                                              editfeetsec.setText(String.valueOf(feetsec9));
                                                              double KmHour9 = Knot9 * 1.852;
                                                              editKmHour.setText(String.valueOf(KmHour9));

                                                          }

                                                      }
                );

            }else{

            }
        }
    });



    return rootView;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to CR! I suggest you make your title tell a bit more about what the fragments do; everyone on this site is looking for ways to make their code better, saying you want that in your title is somewhat redundant ;) also that last closing brace isn't included in the code block; please make sure your indentation matches that in your IDE (select the relevant code in your post, and then Ctrl+K). \$\endgroup\$ – Mathieu Guindon Feb 22 '15 at 14:56
1
\$\begingroup\$

There are several things that can be improved:

  1. Error handling. Your code does not handle the case when a text does not represent a valid double. It would be nice to have some error handling(for instance, you could show an error message in this case). For instance, this line of code:

    double MeterSec = Double.valueOf(editMeterSec.getText().toString());
    

    will fail with NumberFormatException if editMeterSec.getText().toString() is not a valid representation of a double number. You can do it by catching this exception:

    try {
        double MeterSec = Double.valueOf(editMeterSec.getText().toString());
        ...
    } catch (NumberFormatException ex) {
       // Show an error message or do something else.
    }
    
  2. "Magic" constants. It is not good to use such numbers as 3.28084 or 3.6 directly in your code because their meaning is not clear. You should create variables with meaningful names to hold these constants. For instance, you could have a

    private static final KILOMETERS_PER_HOUR_TO_METERS_PER_SECOND_RATIO = 3.6;
    

    and then use it for the conversion between different units of speed.

  3. Empty else blocks are redundant. Just get rid of them.

    if (predicate) {
        // Do something.
    } else {
        // An empty block.
    }
    

    should become

    if (predicate) {
        // Do something.
    }
    
  4. Whitespaces. It is conventional to have a whitespace after if, for and while keywords, before and after curly brackets.

  5. Variable naming. Non-static, non-final variables' names should start with a lowercase letter(and different words are separated using camelCase). It is not consistent in your code: textSpeed, for instance, follows this convention, but MeterSec or feetsec violates it.

  6. Design of your class. Having one huge method that does everything is a bad practice. Moreover, the entire Speed class does too many things: it handles GUI and the conversion logic at the same time. I would recommend creating a separate utility class that converts speed units into each other. Current design makes your code very hard to test.

  7. Avoiding code duplication. Several event listeners contain the same code:

    new View.OnFocusChangeListener() {
        @Override
        public void onFocusChange(View v, boolean hasFocus) {
            if (hasFocus) {
                Button buttonConvertSpeed = (Button) rootView.findViewById(R.id.buttonConvertSpeed);
                buttonConvertSpeed.setOnClickListener(new View.OnClickListener() {
                                                          @Override
                                                          public void onClick(View v) {
                                                              // Do something.   
                                                          }
                });
            }
        }
    }
    

    Instead of using local anonymous classes and duplicate code, you can create a class that implements the View.OnFocusChangeListener interface so that you can reuse it(it can have a constructor that takes a Runnable interface to customize the behavior of the onClick method). In general, you should try and improve the modularity of your code.

\$\endgroup\$
  • \$\begingroup\$ Your answer was really helpful but I really didn't understand your opinion no. 7....how would i customize the behavior of onClick method?? if u could show me an example how to do so, it would be really helpful. :) \$\endgroup\$ – Rsq Zmn Feb 24 '15 at 16:27
  • \$\begingroup\$ @RsqZmn Something like this: pastebin.com/hfDF2kxb. \$\endgroup\$ – kraskevich Feb 24 '15 at 16:43

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.