1
\$\begingroup\$

First off, I'm no Java programmer(but I do know the concepts of OO programming), I have this project I need to work on but I currently don't have a lot of time to spend in learning Java and then implementing it, I need to learn it on the fly. I appreciate the concern on letting me know that I need to learn the language first, and even though I want to, I really can't.

With that out of the way, I'm building an Android app in which I will implement thermodynamic equations of state. The basic outline of the app is as follows:

  • It has a menu in which you can choose the equation of your choice. (I will use the simple Ideal Gas Law to exemplify this); this is the MainActivity of the project.

  • Now that you have chosen the Ideal Gas Law, you will be presented with 3 options, to calculate temperature, pressure or volume, each of this will be implemented using a Button widget; this is the IdealGasActivity of the project, and each equation must be like this one (maybe I can inherit this class?).

  • Finally, once you have chosen a variable to calculate, we will use the temperature as an example, you are presented with a set of EditText fields where you can enter the values for the variables that you have and a Button to calculate the temperature, it also has some Spinner widgets to convert to different units; this would be the TemperatureActivity. Here is what the Temperature class looks like:

    public class Temperature extends AppCompatActivity {
    
        Spinner spinner_pres;
        Spinner spinner_vol;
        TextView totalTextView;
        EditText pressure_value;
        EditText volume_value;
        EditText moles_value;
        final double consR = 0.0820574; // In atm*L/mol*K
        double pressure = 0.0, volume = 0.0;
    
        @Override
        protected void onCreate(Bundle savedInstanceState) {
            super.onCreate(savedInstanceState);
            setContentView(R.layout.activity_temperature);
    
            // Can this declarations be implemented on a different method?
            // Gather values from text fields
            totalTextView = (TextView) findViewById(R.id.tempTxt);
            pressure_value = (EditText) findViewById(R.id.pressureTxt);
            volume_value = (EditText) findViewById(R.id.volumeTxt);
            moles_value = (EditText) findViewById(R.id.molesTxt);
    
            // Implement list de volume unit conversions
            spinner_pres = (Spinner) findViewById(R.id.units_pressure_temp);
            ArrayAdapter<CharSequence> adapter_pres = ArrayAdapter.createFromResource(this,
                    R.array.arr_units_pressure, android.R.layout.simple_spinner_dropdown_item);
            spinner_pres.setAdapter(adapter_pres);
            spinner_pres.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener() {
                @Override
                public void onItemSelected(AdapterView<?> parent, View view, int position, long id) {
                    switch (position) {
                        case 0:
                            break;
                        case 1:
                            pressure = Float.parseFloat(pressure_value.getText().toString())/14.696;
                            break;
                        case 2:
                            pressure = Float.parseFloat(pressure_value.getText().toString())/760.0;
                            break;
                    }
    
                }
    
                @Override
                public void onNothingSelected(AdapterView<?> parent) {
    
                }
            });
    
            // Implement list de volume unit conversions
            spinner_vol = (Spinner) findViewById(R.id.units_volume_temp);
            ArrayAdapter<CharSequence> adapter_vol = ArrayAdapter.createFromResource(this,
                    R.array.arr_units_volume, android.R.layout.simple_spinner_dropdown_item);
            spinner_vol.setAdapter(adapter_vol);
            //Can I implement this in a new method so I can override it?
            spinner_vol.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener() {
    
                @Override
                public void onItemSelected(AdapterView<?> parent, View view, int position, long id) {
                    switch (position) {
                        case 0:
                            break;
                        case 1:
                            volume = Float.parseFloat(volume_value.getText().toString())/0.001;
                            break;
                        case 2:
                            volume = Float.parseFloat(volume_value.getText().toString())/1000;
                            break;
                    }
    
                }
    
                @Override
                public void onNothingSelected(AdapterView<?> parent) {
    
                }
            });
    
            // Possible use of this.Calculate(); ??
    
            // Calculate temperature
            Button calcBtn = (Button) findViewById(R.id.calcBtn);
            calcBtn.setOnClickListener(new View.OnClickListener() {
                @Override //Calculate temperature
                public void onClick(View view) {
                    float moles = Float.parseFloat(moles_value.getText().toString());
                    double temperature = pressure*volume/(cteR*moles);
                    totalTextView.setText(String.format(Locale.ENGLISH,"%.4f",temperature));
                }
            });
        }
    
        // Method to calculate temperature
        // I know this won't work if I call it inside OnCreate, but why?
        public void Calculate()
        {
            // Calculate temperature
            Button calcBtn = (Button) findViewById(R.id.calcBtn);
            calcBtn.setOnClickListener(new View.OnClickListener() {
                @Override //Calculate temperature
                public void onClick(View view) {
                    float moles = Float.parseFloat(moles_value.getText().toString());
                    double temperature = pressure*volume/(cteR*moles);
                    totalTextView.setText(String.format(Locale.ENGLISH,"%.4f",temperature));
                }
    
    
    });
    }
    

Ok, so my question is, how can I rearrange this class in order to make use of Object Oriented Programming? I want to be able to extend this class to calculate pressure and volume, and also for the other equations. I know it has a lot of errors and possible bad practices, but I would like to avoid just copying and pasting the code over and over again for the other buttons I plan on implementing.

\$\endgroup\$

migrated from stackoverflow.com Mar 13 '17 at 6:57

This question came from our site for professional and enthusiast programmers.

1
\$\begingroup\$

In my opinion, you should make a "model" class (aka POJOs) that represents this kind of operations.

First we need to see what's common about all the operations. We create an interface that every operation should implement. This is so that we can use one single activity class for all the operations! The interface might look something like:

public interface Operation {
    String[] getInputs();
    float calculateResult(HashMap<String, Float> inputs);
    String getName();
}

A model of the ideal gas law for calculating the temperature can be something like

public class IdeaGasLawTemperature implements Operation {
    final double consR = 0.0820574;
    public String getName() { return "Ideal Gas Law - Temperature"; }
    public float calculateResult(HashMap<String, Float> inputs) {
        float moles = inputs.get("moles");
        float pressure = inputs.get("pressure");
        float volume = inputs.get("volume");
        return pressure*volume/(cteR*moles); // What is cteR here? Did you mean consR?
    }

    public String[] getInputs() {
        return { "moles", "pressure", "volume" };
    }
}

Now in your single activity class (the activity where the calculation happens), add a new field:

private Operation operation;

Pass an instance of IdeaGasLawTemperature to this activity using putExtra and assign it to operation. Now, you can set various properties of this activity by calling methods on operation. For example, set the action bar's title to operation.getName(), create text fields according to operation.getInputs(), then get all the values from the text fields, put them in a hash map and pass it to operation.calculateResult() and display the return value.

\$\endgroup\$
  • \$\begingroup\$ Ok, I see what you mean, it's a very good idea, I'll try to implement it. On another note, what about the Adapter and Spinners? Is there a way to create a method in the interface so I don't have to repeat code with all the switch statements? \$\endgroup\$ – lorenzattractor Mar 12 '17 at 20:51
  • \$\begingroup\$ @lorenzattractor Yes you should add them to the interface as well. But for the units you might need another POJO called Unit. It should have 2 fields with getters, name and conversionRate. After you created Unit you can add a method like Unit[] getAvailableUnitsForInput(String input). \$\endgroup\$ – Sweeper Mar 12 '17 at 20:55
  • \$\begingroup\$ So, while trying to implement this, I don't understand the structure of the interface and where I should write this. The code I posted is the activity where the calculation happens, so your code must be written in another file and then instantiated in the same activity file I posted? That means I would have to write two files for each variable, one that looks similar to your code and the other where I implement the activity? \$\endgroup\$ – lorenzattractor Mar 12 '17 at 21:32
  • \$\begingroup\$ @lorenzattractor no, each operation is represented by one Operation object. You should create one single activity for all operations. \$\endgroup\$ – Sweeper Mar 12 '17 at 21:34
  • \$\begingroup\$ [nitpicking mode] "In my opinion, you should make a "model" class (aka POJOs) that represents this kind of operations." these operations are business logic, not data. Therefore it should be in the controller layer, not in the model... \$\endgroup\$ – Timothy Truckle Mar 13 '17 at 8:31
1
\$\begingroup\$

Formal issues

Naming

Java Naming Conventions

As with any other language there are common naming conventions for Java. You should get to know them when you write Java code. You would expect other developers to do the same when they work in your preferred language.

e.g.: you have a method named Calculate(). This should start with a lower case letter (calculate())

You have variable names containing underscore like pressure_value, they should use camelCaseNaming: pressureValue

Don't use (uncommon) abbreviations

You use the abbreviation pres in one of your variable names. you better use the full word pressure here.

Avoid "magic numbers"

in your switch you have literal numbers in your case statements. You should better convert them to constants with expressive names like:

public class Temperature extends AppCompatActivity {
    private static final int NEWTON_PER_SQUARE_METER =0;
    private static final int PSI =1; // just guessing here... ;o)
    private static final int HECTO_PASCAL =2; 
Choose names from the problem domain, not from the programming context

eg.: your variable pressure_value should better be named userInputPressure and the variable spinner_pres should be named pressureUnitInput.

Then you have a parameter named position This should better be selectedPressureUnit.

In conjunction with the previous hint the switch would read like this:

        switch (selectedPressureUnit) {
                case NEWTON_PER_SQUARE_METER:
                    break;
                case PSI:
                    pressure = Float.parseFloat(pressure_value.getText().toString())/14.696;
                    break;
                case HECTO_PASCAL:
                    pressure = Float.parseFloat(pressure_value.getText().toString())/760.0;
                    break;
            }
Include unit in names

Your variables hold physical values. It is a good idea to add the actual unit you expect the values to have.

Some famous space exploration missions crashed because on team processed length in ISO units (eg. kilometers) passing them to another teams unit which expected the value to have imperial unit (eg. miles).

The problem is that there is no technical solution to avoid that. And you won't expect a bug in this:

double length = someDependency.getLength();

But you might get alerted when you write (or read) this:

double lengthInKm = someDependency.getLengthInMi();

Comments

Comments should always explain why the code is like it is.

Other comment often lie as the first occurrence of your comment // Implement list de volume unit conversions does because after that you calculate pressure. This almost ever happens in code under development since nobody really feels responsible to update the comment while the code changes behavior.

Your comments structure the Method onCreate into logical sections. You should better extract those logical sections into separate methods with names derived from the comments you wrote. This would change the method to something like this:

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_temperature);
    gatherValuesFromTextFields();
    implementListDePressureUnitConversions();        
    implementListDeVolumeUnitConversions();
    calculateTemperature();
 }

The code lines themself would move to that methods:

private void gatherValuesFromTextFields(){ // answeres your question from the code
    totalTextView = (TextView) findViewById(R.id.tempTxt);
    pressure_value = (EditText) findViewById(R.id.pressureTxt);
    volume_value = (EditText) findViewById(R.id.volumeTxt);
    moles_value = (EditText) findViewById(R.id.molesTxt);    
}

private void implementListDePressureUnitConversions(){  
    spinner_pres = (Spinner) findViewById(R.id.units_pressure_temp);
    ArrayAdapter<CharSequence> adapter_pres = ArrayAdapter.createFromResource(this,
            R.array.arr_units_pressure, android.R.layout.simple_spinner_dropdown_item);
    spinner_pres.setAdapter(adapter_pres);
    spinner_pres.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener() {
        @Override
        public void onItemSelected(AdapterView<?> parent, View view, int position, long id) {
            switch (position) {
                case 0:
                    break;
                case 1:
                    pressure = Float.parseFloat(pressure_value.getText().toString())/14.696;
                    break;
                case 2:
                    pressure = Float.parseFloat(pressure_value.getText().toString())/760.0;
                    break;
            }

        }

        @Override
        public void onNothingSelected(AdapterView<?> parent) {

        }
    });
}
// and so on...

BTW: This is a (language independent) implementation of the OO principles Separation of Concerns (SoC) and Single Layer of Abstraction (SLA) which would even work in pure procedural C.

Making the code more OO

IMHO the major principles of OO are:

  • information hiding/encapsulation
  • Separation of Concerns (SoC)/Single Responsibility Pattern (SRP)
  • Tell, Don't ask
  • apply polymorphism instead of branching

The ultimate goal of this is to reduce code duplication, improve readability and support reuse as well as extending the code.

replace switch/if-else cascade with polymorphism

You have two switch statements in your code. They should be resolved to use polymorphism since this is one of the most powerful OO mechanisms.

In order to do this we could introduce an interface:

  public class Temperature extends AppCompatActivity {
      private interface UnitCalculation{
        double calculate(String userInput);
      }

Next we create a a list of known calculations:

private void implementListDePressureUnitConversions(){ 
   List<UnitCalculation> unitCalcualtions = Arrays.asList( 
      // pre Java8: anonymous inner class
      new UnitCalculation(){ // NO_SELECTION
        @Overwrite 
        public double calculate(String userInput){
           return Float.parseFloat(userInput);  // default unit
        }            
      },
      // Java8 Lambda, no semicolon at statement end, comma is list separator
      userInput -> Float.parseFloat(userInput)/PSI_QUOCIENT,
      // userInput -> Float.parseFloat(userInput)/14.696, // PSI -> broke my own suggestion
      userInput -> Float.parseFloat(userInput)/HECTO_PASCAL_QUOCIENT        
   );       
   // ...

then we use this list instead of the switch:

    spinner_pres.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener() {
        @Override
        public void onItemSelected(AdapterView<?> parent, View view, int position, long id) {
         pressure = unitCalcualtions.get(position).calculate(pressure_value.getText()); // getText() already returns a `String` object.
        }
     }

You can do so for the volume respectively.

avoid code duplication / support DRY

Often you don't have pure duplicated code. In most cases you have similar looking code which you usually quite easily can change to equal code by simple refactorings and/or by applying design patterns.

This goes basically in 3 steps.

  • identify similar code
  • make similar code the same
  • remove the duplication

Since this process is expected not to change the behavior of your application you should better have UnitTest that fix the desired behavior so that you get immediate response when you broke something.

Identify similar code

The code for calculating pressure and and volume look quite similar but have certain differences. Most of them are obvious like the variable names used. But others are kinda "hidden": Eg.: both switch statements have 3 cases, but this is just an coincidence!

On the other hand: if you follow the polymorphism approach from above then the difference is only the content of the list. And this is a minor difference.

Make similar code the same

In this step you convert individual similar parts of your code to look exactly the same. Avoid typing as much as possible and rely on your IDEs refactorings like "extract to local variable", "rename in file" and alike.

Assuming you have created the separate methods and implemented the polymorphism approach:

We place the cursor on one of the member variables and apply the IDEs refactoring "extract to local Variable (replace all occurrences)". Move the line created by the IDE up to the beginning of the method.

Do this for all member variables so that at the top of the method you have only assignment to local variables and below that you only access local variables, not member variables. Place the List of the unit calculations directly below them.

Do this in both methods, implementListDePressureUnitConversions() and implementListDeVolumeUnitConversions() an make sure to use the same names for the local variables.

Remove the duplication

After that in one of the methods select all lines except the local variable declarations on top and the list. The apply the "extract method" refactoring of the IDE. It should replace the selected part in the current method and the corresponding part in the other method.


Following your suggestion to implement lists instead of switch statements, I get an error in the lambda expressions that says Float cannot be applied to , how can I solve this? – lorenzattractor

For the records: I suggested a Map, which is something different than a List. Please be exact in that to reduce misunderstandings.

The compile problem is cause by your original code where you assign a float value to a variable of type double. This was no problem since float is the smaller datatype an the compiler accepts this assignment as well as in the implemntation of the anonymous class. I didn't change that to make it for you easier to follow.

However with Lambdas the compiler does a lot of assumptions of what you might want to do. Therfore it cannot be that liberal with the type upcasting (float->double).

To solve this you should use the correct parser for your target datatype:

 Double.parseDouble(userInput)//...
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! This is very useful and thorough, although I will accept the other answer in order to close the question, but I may have some questions about this later. \$\endgroup\$ – lorenzattractor Mar 12 '17 at 23:50
  • \$\begingroup\$ @lorenzattractor You're welcome. \$\endgroup\$ – Timothy Truckle Mar 13 '17 at 5:27
  • \$\begingroup\$ Following your suggestion to implement lists instead of switch statements, I get an error in the lambda expressions that says Float cannot be applied to <lambda parameter>, how can I solve this? \$\endgroup\$ – lorenzattractor Mar 18 '17 at 1:42
  • \$\begingroup\$ updated the answer \$\endgroup\$ – Timothy Truckle Mar 18 '17 at 8:46
  • \$\begingroup\$ Now, I have a question about the spinner code. So, whenever I want to change to a different unit (position in the spinner) and stay in that unit, the result value won't change. For example, say I want to stay in the second position (PSI units) and continue doing calculations with that same unit, I have to change positions on the spinner in the app in order to make the calculation because the result won't update itself. Is there a workaround for this? \$\endgroup\$ – lorenzattractor Mar 19 '17 at 0:58

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.