sixth assignment #9
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| package codeu_assignment_6; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import javafx.util.Pair; | ||
|
|
||
| /** | ||
| * | ||
| * @author Karine | ||
| */ | ||
| public class KemRearrangingCars { | ||
|
|
||
| /** | ||
| * Inputs: two arrays, each with a permutation of the numbers 0 to N, | ||
| * where permutationA is the current parking lot state, and permutationB is | ||
| * the desired parking lot state. | ||
| * | ||
| * Output: A series of moves, which leads from permutationA to permutationB | ||
| * | ||
| * Process: | ||
| * | ||
| * Time Complexity: O(n) | ||
| * Memory Compelxity: O(n) | ||
| * (2 arrays of size n + and array of steps size (n-1) max.) | ||
| * | ||
| * @param permutationA | ||
| * @param permutationB | ||
| * @return The return value is pairs of moves ordered from 0 to size of the array, | ||
| * from where the item was to the free location. | ||
| */ | ||
| public ArrayList<Pair<Integer,Integer>> getMovesRearrangingCars(Integer[] permutationA, Integer[] permutationB) { | ||
| if (!isValidInput(permutationA,permutationB)) { | ||
| return null; | ||
| } | ||
|
|
||
| ArrayList<Pair<Integer,Integer>> moveSeries = new ArrayList<>(); | ||
| Integer[] lotCurrState = Arrays.copyOf(permutationA, permutationA.length); /* Deep copy */ | ||
| int n = lotCurrState.length; | ||
|
|
||
| /* Say we wish to swap 3 with 0 (as in the example), we need to find 3 | ||
| We keep the indeces of each element to so in O(1) - we update these | ||
| indeces till the end of the algorithm to keep the performance | ||
| */ // 1,2,0,3 => 2,0,1,3 | ||
| Integer[] currIndexToElements = new Integer[n]; | ||
| for (int i=0; i<n; i++) { | ||
| currIndexToElements[permutationA[i]] = i; | ||
| } | ||
|
|
||
| /* Only for the first time need to find where is the 0 */ | ||
| int currZeroLocation = currIndexToElements[0]; | ||
|
|
||
| /* Till lotCurrState is not equal to permutationB, max steps n-1 */ | ||
|
Collaborator
There was a problem hiding this comment. "max steps n-1" is not terribly clear - iterating from 0 to n-1 is n steps. It might be that the maximum number of swap steps is actually n-1 because each swap involves two numbers, or something like that, but just leaving "n-1" without justification is not very helpful, as you're forcing readers to think more than is necessary (we want the code to be clearly correct to the reader without hard work!). You could just say something like - "single pass through array, O(n)", which gets across what we really care about. |
||
| for (int i=0; i<n; i++) { | ||
| if (lotCurrState[i] != permutationB[i]){ | ||
| /* If location is not free, move the item there */ | ||
| currZeroLocation = swap(lotCurrState[i], currZeroLocation, currIndexToElements, lotCurrState, moveSeries); | ||
|
|
||
| /* swap the right item into location i (excpet in the case of 0) */ | ||
|
|
||
| currZeroLocation = swap(permutationB[currZeroLocation], currZeroLocation, currIndexToElements, lotCurrState, moveSeries); | ||
| } | ||
| } | ||
|
|
||
| /* The return value is pairs of moves ordered from 0 to size of the array */ | ||
| return moveSeries; | ||
|
Collaborator
There was a problem hiding this comment. Nice, simple algorithm. I have couple of comments on swap, but overall, this is very simple and clean. |
||
| } | ||
|
|
||
| /** | ||
| * Return false if the input is invalid (null, not the same size...) | ||
| * | ||
| * If required, you may add here throw of exceptions | ||
| */ | ||
| private boolean isValidInput(Integer[] permutationA, Integer[] permutationB) { | ||
| if ((permutationA == null) || (permutationB == null)) { | ||
| return false; | ||
| } | ||
| if (permutationA.length != permutationB.length) { | ||
| return false; | ||
| } | ||
| for (int i=0;i<permutationA.length;i++) { | ||
| if (permutationA[i] >= permutationA.length) { | ||
| return false; | ||
| } | ||
| if (permutationB[i] >= permutationB.length) { | ||
| return false; | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. It's nice to have these validity checks here. If you're going to do this, I suggest making the checks a little stronger - for example, you check all the numbers are below the limit, but you don't check they're above zero, or that there aren't repeats. For this problem, it's possible to define an "isValidInput" method that is precise - it returns true if and only if it's valid, and this method is just a bit weaker than that. As a general pattern, I recommend making your validation functions as precise as possible, and clearly document it if it's conservative one way or the other - this is particularly important when it affects security, which is more often than you'd expect when working on production systems. |
||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Process: swap two items in location of the currToSwap and the free space | ||
| * @return the location of the free space after the swap | ||
| */ | ||
| private int swap(int currToSwap, int currZeroLocation, | ||
| Integer[] currIndexToElements, Integer[] lotCurrState, ArrayList<Pair<Integer,Integer>> moveSeries) { | ||
|
Collaborator
There was a problem hiding this comment. Your code is written in a very functional style here - all the parameters "swap" needs are passed through, and there are no member variables. When it comes to this many parameters, you might want to bundle them up - put them as member variables on an object, and then this method becomes "swap(int currToSwap)" - all the other variables becoming member variables. I may have given you advice to pass things explicitly before, so this may sound like contradictory advice. The issue is when you have lots of arguments, bundling them up in an object is useful, especially if they're all being used together. If you've got an algorithm that is more hierarchical in its call tree, with only some variables used in some places, and lots of temporary values being passed from one step to another, that's when explicitly passing values through separate parameters is cleaner, since it highlights the way the data flows through the algorithm. In this case, all the data is used every time you call swap, and swap is pretty much the core of the algorithm, so it's less helpful. |
||
| if (currToSwap == 0) { | ||
| return currZeroLocation; | ||
| } | ||
|
|
||
| /* Find the location of the item to swap to current location */ | ||
| int locationCurrToSwap = currIndexToElements[currToSwap]; | ||
|
|
||
| /* Swap! */ | ||
| lotCurrState[currZeroLocation] = currToSwap; | ||
| lotCurrState[locationCurrToSwap] = 0; | ||
|
|
||
| /* Update the output moves */ | ||
| Pair<Integer,Integer> move = new Pair<>(locationCurrToSwap, currZeroLocation); | ||
| moveSeries.add(move); | ||
|
Collaborator
There was a problem hiding this comment. I think you could skip the creation of the intermediate variable, without losing any readability: |
||
|
|
||
| /* Update the indeces */ | ||
| currIndexToElements[currToSwap] = currZeroLocation; | ||
| currIndexToElements[0] = locationCurrToSwap; | ||
|
|
||
| /* Will do out: currZeroLocation = locationCurrToSwap; */ | ||
| return locationCurrToSwap; | ||
| } | ||
|
|
||
| public ArrayList<Pair<Integer,Integer>> getMovesRearrangingCarsAndPrint(Integer[] permutationA, Integer[] permutationB) { | ||
| ArrayList<Pair<Integer,Integer>> moves = getMovesRearrangingCars(permutationA, permutationB); | ||
|
|
||
| if (moves != null) { | ||
| moves.forEach(t->System.out.println("move from " + t.getKey() + " to " + t.getValue())); | ||
| } | ||
|
|
||
| return moves; | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Nice and simple algorithm. You can complete the task in fewer moves - closer to N than 2*N - so it might be worth having a quick think about that, but generally this is good, clean, simple code. Thank you. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Plural of "index" is "indices" ("indexes" also acceptable).
When creating a table like this, a "reverse mapping" (or inverse mapping) is the term most people use - putting that in the comment will help many people understand more easily. Having said that, the comment is clear, and the example is very helpful.