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

Permutation and combination calculator

Submitted by: @import:stackexchange-codereview··
0
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:

  • 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 Exception

This 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 possible

Still 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 block
and 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 isEmpty

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