Problem
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.
Solution
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.
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)//...