Conversation
karineek
requested review from
0xlbr,
AimeeBorda,
NataliaDymnikova,
alice006,
simon-frankau and
veronikakolejak
simon-frankau
left a comment
simon-frankau
left a comment
There was a problem hiding this comment.
I believe I see a real improvement over earlier assignments. The code is simple and clean - easy to understand and it does the job. Nice. Thank you.
| if (permutationB[i] >= permutationB.length) { | ||
| return false; | ||
| } | ||
| } |
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.
| /* 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 |
There was a problem hiding this comment.
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.
| /* 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 */ |
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.
| /* 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) */ |
| } | ||
|
|
||
| /* The return value is pairs of moves ordered from 0 to size of the array */ | ||
| return moveSeries; |
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 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) { |
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.
|
|
||
| /* Update the output moves */ | ||
| Pair<Integer,Integer> move = new Pair<>(locationCurrToSwap, currZeroLocation); | ||
| moveSeries.add(move); |
There was a problem hiding this comment.
I think you could skip the creation of the intermediate variable, without losing any readability:
moveSeries.add(new Pair<>(locationCurrToSwap, currZeroLocation));
|
|
||
| return moves; | ||
| } | ||
| } |
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.
| expectedRes.add(new Pair<>(5,8)); | ||
| assertEquals(expectedRes, (new KemRearrangingCars()).getMovesRearrangingCars(permutationA, permutationB)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The base/small input case coverage is good, and I see you cover a good range of input sizes.
A shorter way of building the expectedRes lists is described at https://stackoverflow.com/questions/1005073/initialization-of-an-arraylist-in-one-line - you can just use Arrays.asList.
A better way is to write a method that performs the sequence of moves, and checks that it achieves what you wanted. Why? There's more than one sequence of moves that will achieve the result, and your tests are brittle - they will fail if you run the test against someone else's algorithm. Brittle tests make a codebase difficult to maintain (you change the algorithm, it still produces a valid series of moves, but your tests break - annoying!).
Finally, when writing tests, try to concentrate on the code paths and corner cases you're trying to exercise - e.g. "does it work if numbers are swapped pairwise (2, 1, 4, 3, 6, 5, etc.)?", "does it work if zero is where it will end up?", "does it work if everything's already in the right place?", "does it work if everything is moved around cyclically by one place left/right?".
It looks to me like the tests you've written have good coverage, and cover most of these kinds of ideas, but it's worth making it explicit in your tests. Think "How could I have got the algorithm wrong, and what test input would discover that kind of mistake?".
2 participants
No description provided.