(16) New exercise | Permutations

[nikitarevenco]

Because

It was decided to add new recursion exercises

This PR

• Adds exercise 16

Previous

Issue

Related to #27265

Additional Information

Pull Request Requirements

• I have thoroughly read and understand The Odin Project Contributing Guide
• The title of this PR follows the location of change: brief description of change format, e.g. 01_helloWorld: Update test cases
• The Because section summarizes the reason for this PR
• The This PR section has a bullet point list describing the changes in this PR
• If this PR addresses an open issue, it is linked in the Issue section
• If this PR includes any changes that affect the solution of an exercise, I've also updated the solution in the /solutions folder

[MaoShizhong]

Haven't looked at the the solution yet but one (technically 2) important change needed for the test suite below.

[MaoShizhong]

You've got an extra nested array here in the "expected array" that's not needed, but also your outcome function results in the test passing despite this being an incorrect expected outcome.

That being said, even after fixing this, you don't need an outcome helper function with an afterEach that uses .toBe. You can just use the .toEqual matcher that's designed exactly for this purpose.

test('Works for array of size two', () => {
  expect(permutations([1, 2])).toEqual([
    [1, 2],
    [2, 1],
  ]);
});

Having all the tests as per the above format will all pass (and will also fail if you keep the unintended extra nested array in the last test "expected" array.

[nikitarevenco]

You've got an extra nested array here in the "expected array" that's not needed, but also your outcome function results in the test passing despite this being an incorrect expected outcome.

That being said, even after fixing this, you don't need an outcome helper function with an afterEach that uses .toBe. You can just use the .toEqual matcher that's designed exactly for this purpose.

test('Works for array of size two', () => {
  expect(permutations([1, 2])).toEqual([
    [1, 2],
    [2, 1],
  ]);
});

Having all the tests as per the above format will all pass (and will also fail if you keep the unintended extra nested array in the last test "expected" array.

Yes but the problem with that is, what happens if the learner's solution returns the correct array but in a different order?

You've got an extra nested array here in the "expected array" that's not needed, but also your outcome function results in the test passing despite this being an incorrect expected outcome.

That being said, even after fixing this, you don't need an outcome helper function with an afterEach that uses .toBe. You can just use the .toEqual matcher that's designed exactly for this purpose.

test('Works for array of size two', () => {
  expect(permutations([1, 2])).toEqual([
    [1, 2],
    [2, 1],
  ]);
});

Having all the tests as per the above format will all pass (and will also fail if you keep the unintended extra nested array in the last test "expected" array.

OHhhhhhhh wow, I had no idea the following test would pass:

    expect([1, 2]).toEqual([2, 1])

I was thinking that the order that the learner's function returns the array in should not matter, and kind of just assumed that toEqual checks for the order too

[MaoShizhong]

Fair point I missed about the toEqual order. That can be remedied by sorting both sides

test('Works for array of size two', () => {
  expect(permutations([1, 2]).sort()).toEqual(
    [
      [1, 2],
      [2, 1],
    ].sort()
  );
});

Note the .sort() for the expect is after the permutations call, not on the array passed into the permutations call.
Do that, and the order of the matched array won't matter.

[MaoShizhong]

Suggested change

	const permutations = function (original, currentPermutations) {
	if (original.length === 1) return original;
	const perms = currentPermutations
	? currentPermutations
	: original.map((el) => [el]);
	const permutations = function (original, currentPermutations = original.map((num) => [num])) {
	if (original.length === 0) return [];
	if (original.length === 1) return [original];

1. Based on the intent of lines 4-6, instead of the ternary, it'd be clearer to write const perms = currentPermutations ?? original.map((num) => [num]);. || can be used as well, but the intent of that line really is just if currentPermutations doesn't exist, therefore ?? is more semantic. But, since that's the only intent of that line, you should just make it a default parameter.
2. el renamed to num to match the instructions. If you wish for the instructions to not be limited to numbers, then el can be kept and the instructions amended.
3. For an input of [1], it wouldn't make sense to return [1] but rather, [[1]]. The function returns an array of permutations. The only permutation of [1] is [1], so an array containing that would be [[1]], as opposed to [1], which implies the permuted array isn't an array but a number. This is consistent with the other examples, where an array of permuted arrays is returned. The relevant test will need to be updated accordingly.
4. If you're going to do a shortcut for 1-length arrays, then you should probably also include one for empty arrays. The above suggestion treats empty arrays as having no permutations, thus returning only an empty array. An alternative, would be to treat an empty array as having exactly one permutation (one way to arrange emptiness). IF that's the case, the two ifs can be combined to just if (original.length < 2) return [original], so that an input of an empty array returns [[]]. However you decide to treat empty array inputs, you should make it clear in the instructions how they should be treated, and include a test for that.

[nikitarevenco]

Suggested change

	const permutations = function (original, currentPermutations) {
	if (original.length === 1) return original;
	const perms = currentPermutations
	? currentPermutations
	: original.map((el) => [el]);
	const permutations = function (original, currentPermutations = original.map((num) => [num])) {
	if (original.length === 0) return [];
	if (original.length === 1) return [original];

1. Based on the intent of lines 4-6, instead of the ternary, it'd be clearer to write const perms = currentPermutations ?? original.map((num) => [num]);. || can be used as well, but the intent of that line really is just if currentPermutations doesn't exist, therefore ?? is more semantic. But, since that's the only intent of that line, you should just make it a default parameter.
2. el renamed to num to match the instructions. If you wish for the instructions to not be limited to numbers, then el can be kept and the instructions amended.
3. For an input of [1], it wouldn't make sense to return [1] but rather, [[1]]. The function returns an array of permutations. The only permutation of [1] is [1], so an array containing that would be [[1]], as opposed to [1], which implies the permuted array isn't an array but a number. This is consistent with the other examples, where an array of permuted arrays is returned. The relevant test will need to be updated accordingly.
4. If you're going to do a shortcut for 1-length arrays, then you should probably also include one for empty arrays. The above suggestion treats empty arrays as having no permutations, thus returning only an empty array. An alternative, would be to treat an empty array as having exactly one permutation (one way to arrange emptiness). IF that's the case, the two ifs can be combined to just if (original.length < 2) return [original], so that an input of an empty array returns [[]]. However you decide to treat empty array inputs, you should make it clear in the instructions how they should be treated, and include a test for that.

1. I like your idea in your proposed code with the default parameters, however I feel like this could be a good case to show the ?? operator, which the learners are unlikely to use and this seems like a good use case of that.

2. I think its best to keep it to numbers not to overcomplicate this exercise, so renaming el to num is a good call

3. Ohh yeah, cant believe I missed that. Thanks!

4. 0! evaluates to 1, so a set with no elements will always have a single way to arrange it.

[MaoShizhong]

Regarding point 1, we really don't need a new variable to maybe be assigned the parameter value. ?? would only be used in that scenario. The equivalent without a new variable would be

partialPermutations ??= original.map((num) => [num]);

I'm more in favour of the default param, as I think that's more semantic for this specific scenario (value for parameter if it's initialised undefined). I would never personally set a default param via ??=, I'd rather use the tool meant for that purpose. So personally, I think if we wanted to demonstrate the use of ??=, it wouldn't be here.

Also, ?? is introduced in Foundations somewhere in a javascript.info article, IIRC. I'm sure people will come across it at one point. It's a somewhat recent operator anyway, so it's not like it's replacing anything insanely ridiculous.

Regarding point 4, nice justification. In that case, we can return [original] when the length is < 2, then include a test for empty array input, and specify that expectation in the isntructions.

[nikitarevenco]

Regarding point 1, we really don't need a new variable to maybe be assigned the parameter value. ?? would only be used in that scenario. The equivalent without a new variable would be

partialPermutations ??= original.map((num) => [num]);

I'm more in favour of the default param, as I think that's more semantic for this specific scenario (value for parameter if it's initialised undefined). I would never personally set a default param via ??=, I'd rather use the tool meant for that purpose. So personally, I think if we wanted to demonstrate the use of ??=, it wouldn't be here.

Also, ?? is introduced in Foundations somewhere in a javascript.info article, IIRC. I'm sure people will come across it at one point. It's a somewhat recent operator anyway, so it's not like it's replacing anything insanely ridiculous.

Regarding point 4, nice justification. In that case, we can return [original] when the length is < 2, then include a test for empty array input, and specify that expectation in the isntructions.

Also, what do you think about if we used Set instead of Array for this one? I feel like doing that makes sense in regards to it, since we often talk about different ways we can arrange a set of, consecutive integers in this example.

This would have an added benefit of introducing learners to sets, and this seems to me at least a good use case of that. Often times when we are discussing the permutations of something, we are talking about a set. A deck of cards never has duplicates for example.

Sets are cool and to me feel like a topic small enough to be introduced in an exercise's README

Also this would naturally exclude the option with duplicates if we decide to go this route.

[MaoShizhong]

I'll push back on using Sets instead of arrays.
Sets have their own interface to deal with and while they're no complicated by any means, learning what a Set is and how to use one isn't the point of this exercise, which is instead to recursively generate permutations of something. That should be the sole focus of the challenge.

Sets aren't vital to know about and when someone comes across it, by the time they'll be productive to use, the person would be more than capable of using them independently.
They also introduced in the hash map lesson anyway. Just a mention, but even for that lesson, that's more than sufficient.

And even if Sets naturally exclude duplication, we'd still need to explain it. So the only difference would be whether the instructions and any code use Array syntax/interface or Set syntax/interface. Not that valuable in this scenario.

[nikitarevenco]

Okay, sure. I understand where you are coming from and sets can be certaintly unnecessary overhead. In that case, I'm going to change this exercise so that instead of taking an array of consecutive integers, it will now take in an array of random integers, which means that there will be tests for duplication. I like your idea (in another comment, but feels like its better to respond here) and I think that certainly makes the challenge more interesting

Also, we could even expand this challenge to allow for all data types like objects, NaN, strings etc. That shouldn't be too complicated as an element that's an array would not have to have the exercise find that sub-array's permutations

What do you think?

[MaoShizhong]

Had a think about this and in all honesty, I think there's no need to deal with different types and duplicates. I think the complexity with just numbers (the current solution already works fine for random non-duplicated numbers so no changes to the solution are required) is just right. The main challenge will be how to generate a new permutation (or with your approach, build partial permutations num by num), and I think that should be the primary focus of the task.

[MaoShizhong]

I'll be honest, I had quite a bit of trouble understanding this solution as a whole. Not because the method itself is bad, I actually think it's really elegant (like, very). My difficulty primarily came from the naming of things, as several of the things aren't really named very accurately. Hence it threw me off as I was expecting some things to represent one thing, but what they actually represented (and thus how they were ultimately used) was something very different.

Once I clocked what they actually represented, the approach made total sense.

currentPermutations to me, indicates something that contains all actual permutations at this point in time. So for an input array of [1, 2, 3, 4], a currentPermutations array containing [[1, 2, 3, 4], [1, 2, 4, 3]] makes sense to me. Each element is a permutation, and the whole array contains only the permutations currently found.

Instead, what you're doing with each recursive call, is building up an array of partial permutations. For example, [1] and [1, 4] are both partial permutations of [1, 2, 3, 4], but the currentPermutations naming implies they're full permutations.

The below is a suggestion, but implements the above comment's changes as well as different naming. Checking .every for the end base case is fine, but by nature of this approach building up partial permutations one element at a time, all elements will be the same length as each other at every recursive call. Also no strong opinion on perm/permutation, just personally prefer the full. Let me know what you think.

const permutations = function (original, partialPermutations = original.map((num) => [num])) {
  if (original.length === 0) return [];
  if (original.length === 1) return [original];

  const newPartialPermutations = [];
  partialPermutations.forEach((partialPermutation) => {
    const missingNums = original.filter((num) => !partialPermutation.includes(num));
    missingNums.forEach((missingNum) => {
      newPartialPermutations.push([...partialPermutation, missingNum]);
    });
  });

  if (newPartialPermutations[0].length === original.length) {
    return newPartialPermutations;
  }
  return permutations(original, newPartialPermutations);
};

[MaoShizhong]

I should also note that the solution will not work if the input array has duplicates.
You may prefer to not include this additional complexity, so if that's the case, the instructions should also make clear that the learner will not need to account for duplicate numbers.

[nikitarevenco]

I'll be honest, I had quite a bit of trouble understanding this solution as a whole. Not because the method itself is bad, I actually think it's really elegant (like, very). My difficulty primarily came from the naming of things, as several of the things aren't really named very accurately. Hence it threw me off as I was expecting some things to represent one thing, but what they actually represented (and thus how they were ultimately used) was something very different.

Once I clocked what they actually represented, the approach made total sense.

currentPermutations to me, indicates something that contains all actual permutations at this point in time. So for an input array of [1, 2, 3, 4], a currentPermutations array containing [[1, 2, 3, 4], [1, 2, 4, 3]] makes sense to me. Each element is a permutation, and the whole array contains only the permutations currently found.

Instead, what you're doing with each recursive call, is building up an array of partial permutations. For example, [1] and [1, 4] are both partial permutations of [1, 2, 3, 4], but the currentPermutations naming implies they're full permutations.

The below is a suggestion, but implements the above comment's changes as well as different naming. Checking .every for the end base case is fine, but by nature of this approach building up partial permutations one element at a time, all elements will be the same length as each other at every recursive call. Also no strong opinion on perm/permutation, just personally prefer the full. Let me know what you think.

const permutations = function (original, partialPermutations = original.map((num) => [num])) {
  if (original.length === 0) return [];
  if (original.length === 1) return [original];

  const newPartialPermutations = [];
  partialPermutations.forEach((partialPermutation) => {
    const missingNums = original.filter((num) => !partialPermutation.includes(num));
    missingNums.forEach((missingNum) => {
      newPartialPermutations.push([...partialPermutation, missingNum]);
    });
  });

  if (newPartialPermutations[0].length === original.length) {
    return newPartialPermutations;
  }
  return permutations(original, newPartialPermutations);
};

This def makes the code a lot more readable. Misnaming a single variable can lead to so much confusion, I'll certainly take a moment to take this in as probably misnamed variables are harder to understand than a variable that has a random meaningless name lol.

I agree that partialPermutations makes significantly more sense than currentPermutations, thank you for pointing this out.