HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Finding vowels, digits, whitespaces and consonants

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
whitespacesvowelsdigitsconsonantsfindingand

Problem

I am creating a simple app to find vowels, digits, whitespaces and consonants. the basic idea is to practice multi-threading and writing readable code.

```
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import javafx.scene.control.TextField;
import javafx.application.Platform;
import javafx.scene.control.Button;
import java.util.stream.IntStream;
import javafx.fxml.Initializable;
import java.util.ResourceBundle;
import javafx.event.ActionEvent;
import java.util.Arrays;
import javafx.fxml.FXML;
import java.util.List;
import java.net.URL;

/**
*
* @author Aamir khan
*/
public class FXMLDocumentController implements Initializable {

@FXML
TextField inputField;
@FXML
TextField vowelsField;
@FXML
TextField consField;
@FXML
TextField digitField;
@FXML
TextField spaceField;
@FXML
Button check;
@FXML
Button exit;

private List tasks;

@Override
public void initialize(URL url, ResourceBundle rb) {
tasks = Arrays.asList(
() -> findAndUpdateVowels(),
() -> findAndUpdateCons(),
() -> findAndUpdateDigits(),
() -> findAndUpdateWhiteSpace()
);

exit.setOnAction(e -> System.exit(0));
}

@FXML
void checkBtnHandle(ActionEvent e) {
ExecutorService thread = Executors.newFixedThreadPool(tasks.size());
tasks.forEach(thread::submit);
thread.shutdown();
}

public void findAndUpdateVowels() {
long numberOfVowels = find(Find.VOWELS);
updateField(vowelsField, numberOfVowels);
}

void findAndUpdateWhiteSpace() {
long numberOfWhiteSpaces = find(Find.WHITE_SPACE);
updateField(spaceField, numberOfWhiteSpaces);
}

void findAndUpdateCons() {
long numberOfCons = find(Find.CONSONANTS);
updateField(consField, numberOfCons);
}

void findAndUpdateDigits() {
long numberOfDigits = find(Find.DIGITS);
updateField(digitField, numberOfDigits);

}

public boolean isVowel(int codePoint) {
//checking for lower case because the input will only in lower case
retu

Solution

Your code contains a lot of duplication. It can be made much shorter by passing a predicate to the find method instead of an enum. When you do that, the whole enum Find will become unused. After that, you should rename the find method to count, so that the name matches what it is actually doing.

Your way of organizing the imports looks unusual and funny. But in the rare case that someone really looks at them (like in this code review) it makes more sense to sort them by topic instead of by length. But if it's your point that these lines are usually not looked at, it's fine.

I would make the code as simple as this:

import javafx.application.Platform;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.control.Button;
import javafx.scene.control.TextField;

import java.net.URL;
import java.util.Arrays;
import java.util.List;
import java.util.ResourceBundle;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.function.IntPredicate;

public class FXMLDocumentController implements Initializable {

    @FXML
    TextField inputField;
    @FXML
    TextField vowelsField;
    @FXML
    TextField consField;
    @FXML
    TextField digitField;
    @FXML
    TextField spaceField;
    @FXML
    Button check;
    @FXML
    Button exit;

    @Override
    public void initialize(URL url, ResourceBundle rb) {
        exit.setOnAction(e -> System.exit(0));
    }

    @FXML
    void checkBtnHandle(ActionEvent e) {
        List tasks = Arrays.asList(
                () -> updateField(vowelsField, this::isVowel),
                () -> updateField(consField, c -> Character.isLetter(c) && !isVowel(c)),
                () -> updateField(digitField, Character::isDigit),
                () -> updateField(spaceField, Character::isWhitespace)
        );

        ExecutorService executor = Executors.newFixedThreadPool(tasks.size());
        tasks.forEach(executor::submit);
        executor.shutdown();
    }

    private void updateField(TextField field, IntPredicate predicate) {
        String lowerCaseText = inputField.getText().toLowerCase();
        long count = lowerCaseText.chars().filter(predicate).count();
        Platform.runLater(() -> field.setText(String.valueOf(count)));
    }

    private boolean isVowel(int codePoint) {
        //checking for lower case because the input will only in lower case
        return codePoint == 'a'
                || codePoint == 'e'
                || codePoint == 'i'
                || codePoint == 'o'
                || codePoint == 'u';
    }
}


I moved the creation of the tasks lists into the checkBtnHandle method since I don't know at which exact instant the initialize method is called, and I wanted to avoid NullPointerExceptions.

Code Snippets

import javafx.application.Platform;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.control.Button;
import javafx.scene.control.TextField;

import java.net.URL;
import java.util.Arrays;
import java.util.List;
import java.util.ResourceBundle;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.function.IntPredicate;

public class FXMLDocumentController implements Initializable {

    @FXML
    TextField inputField;
    @FXML
    TextField vowelsField;
    @FXML
    TextField consField;
    @FXML
    TextField digitField;
    @FXML
    TextField spaceField;
    @FXML
    Button check;
    @FXML
    Button exit;

    @Override
    public void initialize(URL url, ResourceBundle rb) {
        exit.setOnAction(e -> System.exit(0));
    }

    @FXML
    void checkBtnHandle(ActionEvent e) {
        List<Runnable> tasks = Arrays.asList(
                () -> updateField(vowelsField, this::isVowel),
                () -> updateField(consField, c -> Character.isLetter(c) && !isVowel(c)),
                () -> updateField(digitField, Character::isDigit),
                () -> updateField(spaceField, Character::isWhitespace)
        );

        ExecutorService executor = Executors.newFixedThreadPool(tasks.size());
        tasks.forEach(executor::submit);
        executor.shutdown();
    }

    private void updateField(TextField field, IntPredicate predicate) {
        String lowerCaseText = inputField.getText().toLowerCase();
        long count = lowerCaseText.chars().filter(predicate).count();
        Platform.runLater(() -> field.setText(String.valueOf(count)));
    }


    private boolean isVowel(int codePoint) {
        //checking for lower case because the input will only in lower case
        return codePoint == 'a'
                || codePoint == 'e'
                || codePoint == 'i'
                || codePoint == 'o'
                || codePoint == 'u';
    }
}

Context

StackExchange Code Review Q#138252, answer score: 2

Revisions (0)

No revisions yet.