tree-building: refactor vs. rewrite

[junedev]

The README of tree building says the student should refactor the existing code. I imagined that means there is an incremental way of getting from the existing solution to the better solution. Looking at the existing solution I don't see how that is possible. Can someone explain me how this "refactoring" is supposed to work? Can we add some more guidance to the README where to start that process? If there is no incremental process I would suggest changing the description to say "rewrite the Build function" instead of "refactor".

[hilary]

@junedev did the tree building code pass the tests for you? The README says that the code is "working, but ugly and slow", however, when I run the tests, it fails with

--- FAIL: TestMakeTreeFailure (0.00s)
    tree_test.go:242: Build for test case "duplicate node" returned 0:[1:[] 1:[]] but was expected to fail.

Out of curiosity, I checked the python implementation as well, and guess what? That code fails the tests, too.

[hilary]

Ugh. I just realized that the benchmarks for tree building only test valid record sets, however, the tests dig deeply into invalid cases. So even after we get the initial code sorted so it works out the gate, the benchmarks shape the refactoring task in a way that would be ... unfortunate ... in a production setting to say the least.

@sebito91 I think tree-building is so problematic as an exercise that we should replace it in the core exercises until we can give it a thorough overall. What do you think?

[nicomo]

Just completed this exercise. I'm not sure it should be replaced. But I do agree it should at the very least be reframed: I rewrote it entirely, did not "improve" on the provided code. Which also probably makes the point about benchmarking moot.

[mem]

My own solution to that problem is here. I stuck to my interpretation of what "refactoring" means, even if my first impulse was to ditch the provided code and write it from scratch.

That url doesn't show my iterations, but my first submission was basically adding:

if err := checkRecords(records); err != nil {
    return nil, err
}

and the implementation for checkRecords, because that's enough to reject invalid inputs and pass the tests. The implementation for checkRecords only needs the code shown in lines 134-138, like the comment says. That's all that's needed to pass the tests. The rest of the function is there just to provide an implementation for the requirements in the documentation.

You can keep iterating and removing bits and pieces of the provided code, which is what I did at the suggestion of my mentor. I guess the question this issue tries to express is whether the intention is to interpret "refactoring" as "iterate over the provided code". I just saw a very nice solution that ditched all of the provided code and passes all the tests. Is that valid in the spirit of this exercise?

[junedev]

@hilary What you saw is reported here for Go: https://github.com/exercism/go/issues/1119

[junedev]

@mem Thanks for your explaining your approach. I have mentored a lot of tree building exercises and I literally never saw any solution that still had recognizable parts from the original code like you had. Also the example solution seems more like a rewrite: https://github.com/exercism/go/blob/master/exercises/tree-building/example.go

@hilary @sebito91 Besides all the issues with the tests and description I also find this exercise extremely hard and time-consuming to mentor. Some solutions have more than 200 lines and they are all somewhat different. It takes a lot of time to read through them and think about sensible improvements tailored to the approach the student took. If there is something a little less controversial/divers that we could use as core exercise instead that would be great.

[devilleweppenaar]

I agree with @junedev.

Also, as a student, this has been my least favorite exercise so far (I have a few more core exercises on the Go track left). I felt like I lost a lot of momentum when I got to this exercise.

I absolutely agree that refactoring is an invaluable skill to practice, but I think that there may be a better way to go about it, especially for this exercise.

My suggestion would be to take a much simpler problem and then to:

1. badly format the code (make it ugly) to make the solution very hard to read, but in a way that can be fixed by running a formatting tool such as gofmt in Golang, or ESLint in NodeJS
2. negatively affect performance (make it slow) by implementing a bad practice, such as nesting loops unnecessarily, or something similar

For 2, you could also consider adding performance tests where it makes sense to check that the solution needs to run x% faster than the original.

That way you can showcase the importance of having properly formatted code, and that refactoring code becomes much easier once the formatting is consistent.

[hilary]

@mem The example solutions provided for both implementations of this exercise (python and golang) use an alternative algorithm. Switching algorithms under the hood falls within the realm of refactoring, as far as I'm concerned, however, it certainly isn't where I go first! In my solution, I switched algorithms in my implementation because I recognized the underlying intent of the problem, not because it's something I would generally do in a work setting.

I am uncomfortable with having "throw away the original approach and re-implement" be the message conveyed by the first encounter our students have with a refactoring exercise. Throwing out large sections of existing code and starting again is really only appropriate out the gate when the original implementor went down a known-to-be-less-performant rabbit hole, as they did in this case.

Let's take a look over the various linked list exercises and see what else we might want to swap in (could be more than one).

[bitfield]

I agree with all the comments here. This is a pretty hard exercise right out of the gate, especially if you're not familiar with tree structures. To solve it by refactoring the nightmare code that's provided is basically way out of the student's comfort zone at this point in the track.

There's certainly value in providing a refactoring exercise, but I think it should be on a much smaller and simpler piece of code.