PLEASE NOTE:
The code below has "commented out" code; however, no code is actually commented out!! Check my GitHub link below. I did that solely to highlight the relevant code sections. Code in my MainActivity and BaseCalculatorActivity, for example, are tightly linked to Android. Those are irrelevant to the alternative String.format()
approach. What is important there is the Publish
code; therefore, those sections are not commented out.
Original Question
Now, we get taught that reusability is very important in university; however, when I applied what was taught before, I found out by SO users that my code was not reusable - well part of it was at least.
So my main goal here is to create a reusable part of my application. Secondary goal would be to make sure most (all important parts) are reusable.
Here is my entire project on GitHub.
Alternative
I have been working on an enhancement to**String.format, String.printf (and alike function)** that embodies the behavior, but reworks it into the modern age. Java's String.format()
function was inspired by older languages that had printf
and other similar functions. I have liked the idea behind printf-like functions for a while, but I hate having to always look at the JavaDoc and other documentation for the format flags. I have never been able to create a complex (keyword) String.format()
expression on the first go, and I bet most other people cannot either. They try one thing; It fails, they consult the documentation and retry - it fails. It is stressful, does not follow OOP design and is hard to maintain.
So I have been working on a solution that is more readable and encapsulates the behavior into a separate single entity.
Everything has pros and cons. This is no different. Yes, it is more verbose than printf
-like flags; however, it can be incredibly readable and possibly self-documenting if you try. There is very little boiler-plate code, and it follows many of the object-oriented principles.
Let's look at a simple example of some of the issues with String.format()
We have a simple calculator UI. 2 text inputs, 1 text output, and 4 buttons. The UI is based on an Interface
. It needs to...
- Get First Number
- Get Second Number
- Update the result
... The UI buttons should be linked to an encapsulated listener class to perform the calculations. No logic in the UI! Also, the class should decide how to display the result to the user. It is the UI after all.
However, there is the first issue. It breaks SOLID #1 (Single Responsibility Principle)! Look at Objects That Can Print Themselves. This calculator class needs to manage basic UI work like getting the numbers entered in and outputting the result to the UI with no logic. It also needs to perform some basic logic in how the output will be displayed! This gives the class 2 reasons to change. You could argue that in certain other situations, String.format violates other principles, but I won't continue here.
It should be noted that I think my solution violates SOLID #4 (Dependency Inversion Principle), and that could be why my code is not reusable; however, just knowing that does not help me.
OperationClick
Activated by a button click (ie. Add, Subtract, etc..). This builds the State
public class OperationClick {
// private final BinaryOperation operator;
// private View.OnClickListener listener;
//
// public OperationClick(final BinaryOperation operator) { this.operator = operator; }
public final View.OnClickListener listenerOn(final ICalculatorAbstraction UI) {
// if (listener != null) return listener;
//
// return listener = new View.OnClickListener() {
@Override
public void onClick(final View v) {
// final double total, num1, num2;
// final Operation operation = operator.getOperation();
//
// num1 = Double.parseDouble(UI.getFirstNumber());
// num2 = Double.parseDouble(UI.getSecondNumber());
//
// total = operation.execute(num1, num2);
final ExpressionState expression = new ExpressionState.Builder()
.setFirstNumber(num1)
.setSecondNumber(num2)
.setTotal(total)
.setOperator(operator)
.buildFor(UI);
UI.updateResult(expression);
}
};
}
}
State
Holds calculation data. Builder pattern is used for readability in creation of the object. Slightly different from normal Builder Pattern. Please note that the use of the Builder pattern is not needed here. Simple accessors and setters backed my variable are needed.
public class ExpressionState {
private final Builder builder;
private ExpressionState(final Builder builder) { this.builder = builder; }
public double getFirstNumber() { return builder.number1; }
public double getSecondNumber() { return builder.number2; }
public BinaryOperation getOperator() { return builder.operator; }
public double getTotal() { return builder.total; }
public static class Builder {
private double number1, number2, total;
private BinaryOperation operator;
public Builder setFirstNumber(final double number) { number1 = number; return this; }
public Builder setSecondNumber(final double number) { number2 = number; return this; }
public Builder setOperator(final BinaryOperation sign) { operator = sign; return this; }
public Builder setTotal(final double number) { total = number; return this; }
public ExpressionState buildFor(final ICalculatorAbstraction UI) {
operator.setDisplayMode(UI.getOperatorDisplayMode());
return new ExpressionState(this);
}
}
}
MainActivity
Other Activities like MainActivityDoge and MainActivityStatic are very similar!
public class MainActivity extends BaseCalculatorActivity {
// private EditText txtNumber1, txtNumber2, txtResult;
//
// @Override
// protected void onCreate(final Bundle savedInstanceState) {
//
// super.onCreate(savedInstanceState);
// setContentView(R.layout.activity_main);
//
// initializeOperationButtons();
//
// txtNumber1 = (EditText) findViewById(R.id.txtNumber1);
// txtNumber2 = (EditText) findViewById(R.id.txtNumber2);
// txtResult = (EditText) findViewById(R.id.txtResult);
//
// }
//
// @Override
// public String getFirstNumber() { return getNumberAsString(txtNumber1); }
//
// @Override
// public String getSecondNumber() { return getNumberAsString(txtNumber2); }
@Override
public void updateResult(final ExpressionState result) {
// Note: The Publish class is not needed, it is here for readability only. It is simply a wrapper class inside of the absract class BaseCalculatorActivity. This code simply returns this ... IExpression.Display.getFullExpression(Expression.class, state)
// However, I do like how clean and readable this code is!
this.txtResult.setText(new Publish(result).as(Expression.class)); // OR .as(ExpressionDoge.class)
// Could easily have been...
this.txtResult.setText(IExpression.Display.getFullExpression(Expression.class, state));
// Verses the old method...
this.txtResult.setText(String.format("%s %s %s = %s", result.getFirstNumber(), result.getOperator(), result.getSecondNumber(), result.getTotal()))
}
// @Override
// protected Display getDefaultDisplayMode() { return Display.Sign; }
}
Take note that this is very readable and clean. The UI does not care how it is displayed - and, really, why should it? The UI here is just to map out what to do with user interactions. How to display and format the results is a different goal, and could be abstracted out. String.format()
does not allow that!
BaseCalculatorActivity
This class abstracts out all the ugly, boilerplate code that I don't care about and don't want to keep on typing (DRY). In it there is the Publish
class from the above code. I was thinking this could be a seperate class altogether, but right now it is part of the abstract base class.
public abstract class BaseCalculatorActivity extends Activity implements ICalculatorAbstraction {
// Note: The Publish class is not needed, it is here for readability only. It is simply a wrapper class
protected class Publish {
private final ExpressionState state;
public Publish(final ExpressionState state) { this.state = state; }
public CharSequence as(final Class<? extends IExpression> expression) {
return IExpression.Display.getFullExpression(expression, state);
}
}
// private BinaryOperation.Display display = getDefaultDisplayMode();
//
// protected abstract BinaryOperation.Display getDefaultDisplayMode();
//
// protected void initializeOperationButtons() {
//
// final Button btnAdd = (Button) findViewById(R.id.btnAddition);
// final Button btnSub = (Button) findViewById(R.id.btnSubtract);
// final Button btnMul = (Button) findViewById(R.id.btnMultiple);
// final Button btnDiv = (Button) findViewById(R.id.btnDivide);
//
// btnAdd.setOnClickListener(new OperationClick(Add).listenerOn(this));
// btnSub.setOnClickListener(new OperationClick(Subtract).listenerOn(this));
// btnMul.setOnClickListener(new OperationClick(Multiply).listenerOn(this));
// btnDiv.setOnClickListener(new OperationClick(Divide).listenerOn(this));
//
// }
//
// @Override
// public boolean onCreateOptionsMenu(final Menu menu) {
// // Inflate the menu; this adds items to the OPERATION bar if it is present.
// getMenuInflater().inflate(R.menu.main, menu);
// return true;
// }
//
// @Override
// public boolean onPrepareOptionsMenu(Menu menu) {
//
// return super.onPrepareOptionsMenu(menu);
// }
//
// @Override
// public boolean onOptionsItemSelected(final MenuItem item) {
//
// Intent intent = null;
//
// switch (item.getItemId()) {
//
// case R.id.action_calculator_simple: intent = new Intent(this, MainActivity.class); break;
// case R.id.action_calculator_doge: intent = new Intent(this, MainActivityDoge.class); break;
// case R.id.action_calculator_static: intent = new Intent(this, MainActivityStatic.class); break;
//
// case R.id.action_calculator_sign: display = BinaryOperation.Display.Sign; return true;
// case R.id.action_calculator_verb: display = BinaryOperation.Display.Verb; return true;
// case R.id.action_calculator_noun: display = BinaryOperation.Display.Noun; return true;
//
// default: // Do nothing
// }
//
// if (intent != null) startActivity(intent);
//
// return super.onOptionsItemSelected(item);
// }
//
// protected static String getNumberAsString(final EditText textBox) {
//
// final Editable text = textBox.getText();
//
// if (text.toString().isEmpty()) {
//
// textBox.setText("0");
// return "0";
//
// }
//
// return text.toString();
//
// }
//
// public BinaryOperation.Display getOperatorDisplayMode() { return display; }
}
IExpression
public interface IExpression {
String getExpression();
String formatRealExpression();
// Inner class is needed for Java 7. Java 8 might be cleaner with possibly no Inner class?? but Android does not support it!!
class Display {
protected static ExpressionState state;
public static CharSequence getFullExpression(final Class<? extends IExpression> type, final ExpressionState state) {
String replacedString = "";
Display.state = state;
// Here is the KEY code segment!
for (final IExpression expression : type.getEnumConstants()) {
replacedString += expression.getExpression();
}
return replacedString;
}
}
}
Expression Class
Here is a code snippet first! This is to show one aspect of the code. This shows you the same behavior that String.format
achieves with a high-level description of the String with placeholders. Exact behavior! I am building off of printf
's behavior and nothing building something completely new.
Printing out the enum would guarantee this unformatted expression "%s %s %s = %s" ... Just like we want with printf
public enum Expression implements IExpression {
Number1 ("%s"),
Operator(" %s "),
Number2 ("%s"),
Result (" = %s"); // This is NOT complete! Just a snippet
}
Here is the full enum but I commented out code to show the other aspect of my implementation that has he same behavior as String.format()
achieves with a high-level formatting placeholder syntax. Exact behavior! I am building off of printf
's behavior, but provides a more modern and readable approach that is more maintainable because it does not rely on a cryptic or error-prone syntax of formatting. In fact, you could use almost any form of formatting that you are used to. I show 2 different approaches in the enum
constant Number1
. Note that I have a second class below that implements IExpression and is not commented out at all..
public enum Expression implements IExpression {
// Number1 ("%s") {
//
@Override
public String formatRealExpression() {
// Note Number is a custom class, but I could have used DecimalFormat (Number does that internally!)
return Number.parse(Display.state.getFirstNumber()) // IExpression.Display.state.getFirstNumber()
.setMaximumDecimalPlace(2)
.toString();
// Or more simply
return new DecimalFormat("0.##").format(Display.state.getFirstNumber());
// }
// },
// Operator(" %s ") { // " + " or " - ", etc...
//
@Override
public String formatRealExpression() {
return Display.state.getOperator().toString();
// }
//
// },
// Number2 ("%s") {
//
@Override
public String formatRealExpression() {
return Number.parse(Display.state.getSecondNumber())
.setMaximumDecimalPlace(2)
.toString();
// }
// },
// Result (" = %s") {
//
@Override
public String formatRealExpression() {
return Number.parse(Display.state.getTotal())
.setMaximumDecimalPlace(4)
.toString();
// }
// };
//
// private final String defaultFormat;
//
// Expression(final String format) { this.defaultFormat = format; }
//
// // Because every expression has 1 "%s" and no more, or not something else...this method can be done here!
// @Override
// public String getExpression() { return defaultFormat.replace("%s", formatRealExpression()); }
//
}
ExpressionDoge Class
Here is just a short, second example in full. No commented out code...
public enum ExpressionDoge implements IExpression {
Phrase ("Wow. Such %s. ") {
@Override
public String formatRealExpression() {
return Display.state.getOperator().toString();
}
},
Total ("%s") {
@Override
public String formatRealExpression() {
return new com.facebook.rucinskic.calculator.app.logic.display.Number(Display.state.getTotal())
.setMaximumDecimalPlace(2)
.toString();
}
};
private final String defaultFormat;
ExpressionDoge(final String format) { this.defaultFormat = format; }
@Override
public String getExpression() {
return defaultFormat.replace("%s", formatRealExpression());
}
}
NOTES
List of classes above
MainActivity.java
(not shown -MainActivityDoge.java
, andMainActivityStatic.java
)OperationClick.java
BaseCalculatorActivity.java
(plusPublisher
class)IExpression.java
Expression.java
(not shown -ExpressionDoge.java
)
1 Answer 1
OperationClick
About half of the code in this class is commented out. Why? It looks a bit like you did it one way first, and then decided to do it another way. But some of the disabled code is required by the non-disabled code. This is quite confusing.
ExpressionState
This is basically only boilerplate code, a wrapper for a mathematical expression. (The boilerplate part here is not necessarily your fault, Java is a very verbose language)
MainActivity
Again with the disabled code. This makes things very confusing for me as a reviewer.
Here you are creating a Publish
object just to make it available for the garbage collector afterwards:
new Publish(result).as(Expression.class)
Consider a static method something like this:
Publish.publishResult(result, Expression.class)
The old method that you don't like:
this.txtResult.setText(String.format("%s %s %s = %s", result.getFirstNumber(), result.getOperator(), result.getSecondNumber(), result.getTotal()))
Can easily be replaced by result.getFormattedString("%s %s %s = %s");
or simply result.getString();
Display inner class
// Inner class is needed for Java 7. Java 8 might be cleaner with possibly no Inner class?? but Android does not support it!!
First of all, Android supports some Java 8 features if you're using a gradle plugin. (Don't know if it would support this)
Secondly, did it ever occur to you that perhaps the method you are trying to place here does not belong here at all? The Display
class can just as well be placed as it's own class in it's own file. No need to be an inner class inside this interface at all.
Static variable
protected static ExpressionState state;
DING DING DING - (削除) threat (削除ここまで) static variable has been detected. This static variable is a big no-no. When you are accessing this variable you are breaking the Tell, don't ask principle. You should pass methods the information they need to do their job, they should not access static variables outside their scope to grab information.
As there can only be one version of a static variable, this value can never have two separate values, limiting you to only one available ExpressionState
at a time (and don't even think about making a static Map<String, ExpressionState>
to solve that!!)
This static variable is the biggest problem in your current code.
Expression enum
provides a more modern and readable approach that is more maintainable because it does not rely on a cryptic or error-prone syntax of formatting. In fact, you could use almost any form of formatting that you are used to.
Overall, I disagree. This approach is actually very limiting as I can not create a new enum at runtime. What if I wanted to create a dynamic format? I might want to let the user specify how he or she wants to see the output. They can't create an enum at runtime. All possible formats must be specified at compile-time.
Modern? Sure, enums are nice. But enums should not be used for this.
Readable? Sure, it is quite readable.
Maintainable? No. Disagree. Sure, it's easy to create a new enum if I want a new format, but it is quite a lot of boilerplate code needed.
I have liked the idea behind printf-like functions for a while, but I hate having to always look at the JavaDoc and other documentation for the format flags
Once you have used some of the format flags enough, you will remember them. %s
, %d
and %f
is the most common ones you need to know. Specifying number of digits and stuff are a bit more advanced, but after a while you'll learn that %.3f
gives you a float formatted to 3 decimals.
There is very little boiler-plate code, and it follows many of the object-oriented principles.
There is actually quite a lot of boilerplate code involved here.
Other alternatives
I was considering suggesting making a StringPatternBuilder
/ StringPattern
class to programmatically specify a string pattern. However, in the end I don't think it can beat the String.format
method.
I'm sad to say it, but I don't find your code either re-usable or useful enough to be an alternative to String.format
.
-
\$\begingroup\$ The code is not disabled at all. If you look at the GitHub link, you will see all the code is NOT commented out. I wanted to point out all the code that was imperative to the
String.format()
alternative. In my OperationClick class, the only part that was related to my alternative solution was the ExpressionState code...everything else was NOT related. I commented it out so you could see it, but wanted to highlight the most important code \$\endgroup\$Christopher Rucinski– Christopher Rucinski2014年09月10日 14:11:32 +00:00Commented Sep 10, 2014 at 14:11 -
\$\begingroup\$ "but I don't find your code either re-usable" ... yes, my original title to this question specifically stated there were reusability issues before someone edited it out. I already knew that. Also whether you find this useful or not is not related to the question. How can I make this more usable. \$\endgroup\$Christopher Rucinski– Christopher Rucinski2014年09月10日 14:18:30 +00:00Commented Sep 10, 2014 at 14:18
-
1\$\begingroup\$ @ChristopherRucinski I believe my answer has pointed out some flaws in your code that you can change to make it more usable (and re-usable). The most important one is: Get rid of the static variable. The second most important one is: Do not force the use of enums. I think you need to change your approach to this significantly. \$\endgroup\$Simon Forsberg– Simon Forsberg2014年09月10日 14:36:08 +00:00Commented Sep 10, 2014 at 14:36
-
\$\begingroup\$ Yes, I agree, and I am thinking of ways right now. However, I would like to know why you say Don't force enums. Why? Can you link to some good articles? Also, I did mean reusable, not usable. UPDATE: My alternative is not meant to be a silver bullet, I have mentioned in the question some of the issues of it. However, this should be seen as a general-purpose alternative, and I would say code generation isn't in its use cases. \$\endgroup\$Christopher Rucinski– Christopher Rucinski2014年09月10日 14:38:47 +00:00Commented Sep 10, 2014 at 14:38
-
\$\begingroup\$ @ChristopherRucinski The main reason I disagree with your enum approach is that it's not possible to specify it in runtime. Enums should be used for when there is a limited distinct value set, such as
enum Direction { NORTH, SOUTH, EAST, WEST }
. You are using Enums as a list/array, that is not the intended purpose of enums. \$\endgroup\$Simon Forsberg– Simon Forsberg2014年09月10日 14:41:09 +00:00Commented Sep 10, 2014 at 14:41
String.format
alternative is complicated enough for a single CR and it has nothing to do with Android. So do a bit of separation of concerns. \$\endgroup\$String.format
, why not simply use aMessageFormat
? Reinventing the wheel seems overkill... \$\endgroup\$MessageFormat
is a useful replacement. It can do localization, but it's missing many other features, e.g. hex formatting. \$\endgroup\$