ASCII to Binary and reverse in java

Question

Here is a simple app that i am trying to implement in java. my goal is to write a program that can convert ASCII to Binary and back.

Questions:

• Am i violating any OOP Rule.
• Is my code readable ?
• How can i make it more efficient ?
• Anything that want to suggest.

Code:

public class FXMLDocumentController implements Initializable {

@FXML
TextArea asciiTextArea;
@FXML
TextArea binaryTextArea;
@FXML
StackPane animationPane;
private static final int AMOUNT_OF_BITS = 8;

@FXML
private void handleBinaryToText(ActionEvent event) {
    Callable<String> task = () -> binaryToAscii(binaryTextArea.getText());
    try {
        runTask(task, asciiTextArea);
    } catch (InterruptedException | ExecutionException ex) {
        asciiTextArea.setText(ex.toString());
    }

}

@FXML
private void handleTextToBinary(ActionEvent event) {
    Callable<String> task = () -> asciiToBinary(asciiTextArea.getText());
    try {
        runTask(task, binaryTextArea);
    } catch (InterruptedException | ExecutionException ex) {
        binaryTextArea.setText(ex.toString());
    }
}

@Override
public void initialize(URL location, ResourceBundle resources) {
    //Validate the Binary TextArea
    final Pattern binaryRegex = Pattern.compile("\\A[01\\n]*\\Z");
    Predicate<String> tester = binaryRegex.asPredicate();
    binaryTextArea.setTextFormatter(new TextFormatter<>(change -> {
        if (!tester.test(change.getControlNewText())) {
            return null;
        }
        return change;
    }));
       //binaryTextArea.setTextFormatter();

}

// Create the Binary To Text Task task
private String binaryToAscii(final String input) {

    if (input.length() % AMOUNT_OF_BITS != 0) {
        String msg = "Input must be a multiple of " + AMOUNT_OF_BITS;
        binaryTextArea.setText(msg);
        throw new IllegalArgumentException(msg);
    }
    final int INPUT_LEN = input.length();
    final int BUILDER_SIZE = INPUT_LEN / 8;
    StringBuilder result = new StringBuilder(BUILDER_SIZE);

    for (int i = 0; i < INPUT_LEN; i += AMOUNT_OF_BITS) {
        char charCode = (char) Integer.parseInt(input.substring(i, i + AMOUNT_OF_BITS), 2);
        result.append(charCode);
    }
    return result.toString();
}

// Create the Binary To Text Task
private String asciiToBinary(String text) {
    final byte[] bytes = text.getBytes();
    StringBuilder result = new StringBuilder(bytes.length * 8);
    for (byte b : bytes) {
        int val = b;
        for (int i = 0; i < AMOUNT_OF_BITS; i++) {
            result.append((val & 128) == 0 ? 0 : 1);
            val <<= 1;
        }
    }
    return result.toString();
}

private void runTask(Callable<String> task, TextArea textArea) throws InterruptedException, ExecutionException {
    ExecutorService thread = Executors.newFixedThreadPool(1);
    final String result = thread.submit(task).get();
    Platform.runLater(() -> {
        textArea.setText(result);
    });
    thread.shutdown();
}

}

Github link

Answer 1

Just a few quick observations:

1. You mix business logic with UI code. Normally, you'd go for separation of concerns and move the conversion functions into a separate class which is used by the UI, but does not belong to the UI.

2. Running a separate thread: how long does the conversion take? 1 ms? 2? I really doubt that the overhead of thread-creation is necessary here. (Though it may indeed serve as an excercise!)

3. Binary to ascii validation: if the amount of bits does not match, you overwrite the user input? Thus, if I, the user, make a little mistake, type 319 bits instead of 320, you actually force me to start over? /me pissed user :-)

That's it for now (probably not complete.)

Answer 2

1. Even if FXMLDocumentController is called a controller it is part of the UI. You can easily argue that: It is bound to the JAVA FX technology as it uses @FXML Annotations. Knowing this: any UI independent logic that may be lost if you change the UI technology is subject to be extracted into separate compile units (Seperation of concerns).
2. Like Swing, JavaFX as well is not thread safe so you have to handle multithreaded access to your UI elements on your own. The Proper way to do this is to put the UI Update into a Runnable or a Task and use JavaFX concurrency mechanisms like you did at one place: Platform.runLater(...). But you should do this ALWAYS to avoid inconsistent states in your UI.

3. As JavaFX is not thread safe and you are using lambda method references as asynchronous tasks you should always pass immutable values. You do not knoww WHEN the Task is executed. The values of your UI component may have changed in between. So do following change:

   final String text = binaryTextArea.getText(); Callable task = () -> binaryToAscii(text);

4. Your runTask-Method is not beneficial. Your task is certainly pooled and maybe executed on another CPU. But as you are waiting for the Future to reveive the value the current thread is blocked. Furthermore to instantiate a Threadpool with only one thread for only one Task is a little too much. My suggestion is to put "Platform.runLater(...)" on the end of your long running Task. As you would mix up concerns (UI logic in Business logic) I would implement a Callback-Mechanism to inform the UI.

   private void binaryToAscii(final String input) {
   
       ...
   
       callback.onBinaryToAsciiConversionSuccessful(result.toString());
   }
   

   "callback" effectively is the UI. But it is hidden behind a callback interface.

   public interface ConversionTaskCallback {
       void onBinaryToAsciiConversionSuccessful(String ascii)
       void onAsciiToBinaryConversionSuccessful(String binary)            
   }
   

   Your UI should implement this interface and register itself as a callback in your "business logic" (thats not really true as it is the use case layer). Then you UI gets notified and you can process the proper UI modifications:

   void onBinaryToAsciiConversionSuccessful(final String ascii) {
       Platform.runLater(() -> {
           textArea.setText(ascii);
       });
   }
   

What you did right is the separation of the threads. You use Platform.runLater to update your UI. "Long running" business tasks like "Converting ascii to binary" you put into a background thread.