7
\$\begingroup\$

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...

  1. Get First Number
  2. Get Second Number
  3. 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, and MainActivityStatic.java)
  • OperationClick.java
  • BaseCalculatorActivity.java (plus Publisher class)
  • IExpression.java
  • Expression.java (not shown - ExpressionDoge.java)
\$\endgroup\$
  • 1
    \$\begingroup\$ I'd really suggest to leave out all the Android stuff. A 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\$ – maaartinus Sep 10 '14 at 2:33
  • \$\begingroup\$ If you don't like String.format, why not simply use a MessageFormat? Reinventing the wheel seems overkill... \$\endgroup\$ – Boris the Spider Sep 10 '14 at 9:08
  • 1
    \$\begingroup\$ Regarding “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” – I recently read an interesting essay on the topic of reusability. To summarize: reusability has no meaning. What matters is usability. \$\endgroup\$ – amon Sep 10 '14 at 15:37
  • \$\begingroup\$ @BoristheSpider I doubt that MessageFormat is a useful replacement. It can do localization, but it's missing many other features, e.g. hex formatting. \$\endgroup\$ – maaartinus Sep 14 '14 at 15:11
  • \$\begingroup\$ I actually have an updated version that looks very promising and does not have the reusability issues the posted version has \$\endgroup\$ – Christopher Rucinski Sep 14 '14 at 15:18
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\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 Sep 10 '14 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 Sep 10 '14 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 Sep 10 '14 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 Sep 10 '14 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 Sep 10 '14 at 14:41

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.