patternjavaMinor
Permutation and combination calculator
Viewed 0 times
andpermutationcombinationcalculator
Problem
I wanted to make something for the Community Challenge, so I made a simple permutation and combination calculator. I'd like input on:
```
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class PermutationCombinationCalculator extends Application {
@Override
public void start(Stage stage) {
TextField entry = new TextField();
entry.setPromptText("Enter a value to permutate/combine here");
TextField limiter = new TextField();
limiter.setPromptText("Enter S, where S is the number of selections possible");
Label permutationResult = new Label("Result will be here!");
Label combinationResult = new Label("Result will be here!");
Button permutate = new Button("Permutate");
permutate.setOnAction(e -> {
permutationResult.setText(
computePermutation(entry.getText(), limiter.getText())
);
});
Button combine = new Button("Combine");
combine.setOnAction(e -> {
combinationResult.setText(
computeCombination(entry.getText(), limiter.getText())
);
});
HBox permutationLayout = new HBox();
permutationLayout.getChildren().addAll(permutate, permutationResult);
HBox combinationLayout = new HBox();
combinationLayout.getChildren().addAll(combine, combinationResult);
VBox layout = new VBox();
layout.getChildren().addAll(entry, limiter, permutationLayout, combinationLayout);
stage.setScene(new Scene
- The overall design - any suboptimal choices that could be improved?
- Naming - This is hard, I know. But is it confusing to follow? Some names seem repetitive, others seem overly long.
- Any general improvements I could make.
```
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class PermutationCombinationCalculator extends Application {
@Override
public void start(Stage stage) {
TextField entry = new TextField();
entry.setPromptText("Enter a value to permutate/combine here");
TextField limiter = new TextField();
limiter.setPromptText("Enter S, where S is the number of selections possible");
Label permutationResult = new Label("Result will be here!");
Label combinationResult = new Label("Result will be here!");
Button permutate = new Button("Permutate");
permutate.setOnAction(e -> {
permutationResult.setText(
computePermutation(entry.getText(), limiter.getText())
);
});
Button combine = new Button("Combine");
combine.setOnAction(e -> {
combinationResult.setText(
computeCombination(entry.getText(), limiter.getText())
);
});
HBox permutationLayout = new HBox();
permutationLayout.getChildren().addAll(permutate, permutationResult);
HBox combinationLayout = new HBox();
combinationLayout.getChildren().addAll(combine, combinationResult);
VBox layout = new VBox();
layout.getChildren().addAll(entry, limiter, permutationLayout, combinationLayout);
stage.setScene(new Scene
Solution
Catch specific exceptions, avoid
This is a good example where it's a bad idea to catch
The
but what if something goes wrong in
The bug will get masked under an "Invalid input" response.
It's good to catch the most specific exception type.
It also communicates to readers of the code what exactly can go wrong with the code.
Limit the body of the
Still on the previous example,
catching
You should also move the
to avoid catching
Sure, if you look at the implementation of
but who knows, you might add a change later.
In general, it's not practical to verify all calls going out from a
It's a lot easier to just move such calls outside of the
and eliminate all suspicions and potential future bugs.
Avoid repeated logic
The
You could rearrange the code to do it only once.
Avoid duplicated string literals
The message "Invalid input." appears multiple times.
It would be good to move it to a constant.
Check empty strings with
Instead of
a more idiomatic way is
ExceptionThis is a good example where it's a bad idea to catch
Exception:try {
total = Integer.parseInt(entry);
return Integer.toString(permutate(total));
} catch(Exception e) {
return "Invalid input";
}The
catch here was clearly intended for parsing errors,but what if something goes wrong in
permutate due to some bug?The bug will get masked under an "Invalid input" response.
It's good to catch the most specific exception type.
It also communicates to readers of the code what exactly can go wrong with the code.
Limit the body of the
try block as much as possibleStill on the previous example,
catching
NumberFormatException is a good first step, but not enough.You should also move the
permutate call outside the try-catch block,to avoid catching
NumberFormatException thrown by permutate instead of the Interger.parseInt(entry) line.Sure, if you look at the implementation of
permutation this is impossible now,but who knows, you might add a change later.
In general, it's not practical to verify all calls going out from a
try block for potential hidden bugs.It's a lot easier to just move such calls outside of the
try blockand eliminate all suspicions and potential future bugs.
Avoid repeated logic
The
computePermutation method has some duplication of logic in the parsing of total.You could rearrange the code to do it only once.
Avoid duplicated string literals
The message "Invalid input." appears multiple times.
It would be good to move it to a constant.
Check empty strings with
isEmptyInstead of
limit.length() == 0,a more idiomatic way is
limit.isEmpty().Code Snippets
try {
total = Integer.parseInt(entry);
return Integer.toString(permutate(total));
} catch(Exception e) {
return "Invalid input";
}Context
StackExchange Code Review Q#88525, answer score: 4
Revisions (0)
No revisions yet.