current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I wanted to make something for the Community Challenge, so I made a simple permutation and combination calculator. I'd like input on:

  1. The overall design - any suboptimal choices that could be improved?
  2. Naming - This is hard, I know. But is it confusing to follow? Some names seem repetitive, others seem overly long.
  3. 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(layout));
        stage.show();
    }

    private static String computePermutation(String entry, String limit) {
        int total;
        int select;

        if (limit.length() == 0) {
            try {
                total = Integer.parseInt(entry);
                return Integer.toString(permutate(total));
            } catch(Exception e) {
                return "Invalid input";
            }
        }

        try {
            total = Integer.parseInt(entry);
            select = Integer.parseInt(limit);
        } catch(Exception e) {
            return "Invalid input";
        }
        return Integer.toString(permutate(total, select));
    }

    private static String computeCombination(String entry, String limit) {
        int total;
        int select;

        try {
            total = Integer.parseInt(entry);
            select = Integer.parseInt(limit);
        } catch(Exception e) {
            return "Invalid input.";
        }
        return Integer.toString(combine(total, select));
    }

    private static int permutate(int p, int r) {
        int result = 1;
        for (int i = p; r > 0; r--, p--) {
            result *= p;
        }

        return result;
    }

    private static int permutate(int p) {
        return permutate(p, p);
    }

    private static int combine(int total, int select) {
        return permutate(total) / (permutate(select) * permutate(total - select));
    }

    public static void main(String[] args) {
        launch(args);
    }
}

Download a runnable Jar here (Requires JRE 8)

share|improve this question

2 Answers 2

up vote 3 down vote accepted

Better way for combinations

When I first saw that you had a two argument permutate() function, I thought it was a smart idea because you would use it for computing combinations. But it fact, you never use the two argument permutate function for doing combinations.

Here's how you could write the combine() function to make use of the two argument permutate():

private static int combine(int total, int select) {
    select = min(select, total-select);
    return permutate(total, select) / permutate(select);
}

Overflows

As written, your combine() function can overflow and produce the wrong result, even if the result would fit in an integer. The improved combine() function above has a much smaller chance of doing so since it doesn't do the full permute. Still, you could do better by either:

  1. Having permutate() use longs and return a long.

  2. Do the combine calculation one numerator at a time instead of computing the full numerator first. This is the best way of avoiding overflow. Here is how the code would look:

    private static long combine(int total, int select) {
    long result = 1;
    select = min(select, total - select);
    for (int i=0; i<select; i++)
        result = result * (total-i) / (i+1);
    return result;
}

    Note that the division by (i+1) is guaranteed to divide evenly at every iteration. I'll leave the proof for that as an exercise to the reader.

share|improve this answer
up vote 3 down vote

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.