Red Black Tree 2

[JaapWijnen]

A new version of the red black tree. Als the one already posted doesn't implement deletion correctly

[kelvinlauKL]

@JaapWijnen Thanks for the PR.

Just a few comments:

• I liked how you changed color to an enum rather than a boolean flag. Definitely an element to make code "Swiftier".
• I liked how you changed some of the functions to computed variables.

A few changes I'd like to see:

• enum cases should be lowerCamelCased as of Swift 3
• there are a lot of forced unwrapping usage. Let's try using optionals. For instance, your delete method uses ! all the time. It makes reading code uncomfortable, not knowing when sibling would be nil. Use a guard statement early on to guarantee non-nil values.
• can we try changing the properties of RBTNode to optionals rather than implicitly unwrapped optionals? Ex: RBTNode<T>! -> RBTNode<T>?
• It's a bit inconvenient trying to verify how the tree is working. Consider adopting the CustomStringConvertible protocol and configure the output to allow people to get a neat printout of the current values and positions in the tree. Take a look at https://www.raywenderlich.com/138190/swift-algorithm-club-swift-tree-data-structure and look up CustomStringConvertible to see an example of how that works.

Thanks!

[JaapWijnen]

Thanks for the comments! Not sure how this works but can I edit this PR? Or do I need to make a new one? Then ill go ahead and implement your suggestions.

[kelvinlauKL]

You should be able to update this current one.

[JaapWijnen]

I changed the enums and added the CustomStringConvertible protocol per your suggestions. However I'm not too comfortable implementing your third suggestion

can we try changing the properties of RBTNode to optionals rather than implicitly unwrapped optionals? Ex: RBTNode<T>! -> RBTNode<T>?

As especially computed properties like isLeftChild don't react nicely to being called on optionals. Resulting in having to force unwrap a lot in the code.

I also changed the property sibling to be non optional. And rewrote the deletion part of the algorithm such that sibling can never be nil.
Also found a small bug in the insertion part of the algorithm a single line of code was missing, so I fixed that.

Let me know what you think!

[kelvinlauKL]

Solid changes overall. I'm still not comfortable with the implicitly unwrapped optionals. One of the biggest reasons is that those properties are public, meaning if any of them were nil, it'll crash.

Anyway, I'm happy to defer that issue to a later date.

Before we merge, please credit yourself for doing this refactor at the bottom of the README file. Thanks!

[JaapWijnen]

Done! If you have any suggestions what to change for it to work with regular optionals, please let me know! Cause I'm not sure how to get the computed properties to work without force unwrapping them. Thanks for the help!

[kelvinlauKL]

You're going to have to force unwrap them for computed properties, or have the computed properties return an optional. I think we'll have to dig deeper to see which is the better approach.

After reading your README I just remembered you were doing a completely new implementation on an existing algorithm. Could you replace the old Red Black Tree code with your implementation and make sure the README references the methods in your implementation correctly?

[JaapWijnen]

Done!

[kelvinlauKL]

Thanks @JaapWijnen. I'm going to add a line to your README for Ashwin Raghuraman, the original author of this. Otherwise, nice work!