search

steveyen / gkvlite

Simple, ordered, key-value persistence library for the Go Language
MIT License
263 stars 32 forks source link

union code can't be correct #1

Closed dustin closed 11 years ago

dustin commented 11 years ago

I haven't confirmed, but I can't see how this thing could possibly return an error.

In general, I think anything more than two or three levels should be a sign that something is not right. In this case, there's something like nine levels of conditionals nested, each declaring a distinct err var and throwing it away.

It seems like it should be possible to make a correct implementation that's correct and less... wide.

Edit

steveyen commented 11 years ago

Thanks - pushed a quick err assignment fix, but still just as wide and indented until I get some more quality time.

If you want to understand the general union logic more, here's the original (non-persistent / memory-only) code that this was based on...

https://github.com/steveyen/gtreap/blob/master/treap.go#L78

And it was pretty much mechanically adding the reading & error propagation stuff.

dustin commented 11 years ago

that doesn't look bad. Why don't you make the other one look like that? :)

In general, just return the error when you get it. I think that'd clean it up a lot.

Steve Yen wrote:

Thanks - pushed a quick err assignment fix, but still just as wide and indented until I get some more quality time.

If you want to understand the general union logic more, here's the original (non-persistent / memory-only) code that this was based on...

https://github.com/steveyen/gtreap/blob/master/treap.go#L78

And it was pretty much mechanically adding the reading & error propagation stuff.

— Reply to this email directly or view it on GitHub https://github.com/steveyen/gkvlite/issues/1#issuecomment-12207159.

steveyen commented 11 years ago

Hmmm.

Looks like I completely misunderstood the := operator.

dustin commented 11 years ago

It's a little extra confusing inside the if. I don't use the if/;/test idiom all that much. When you do, the bindings don't escape the if/else.

Steve Yen wrote:

Hmmm.

Looks like I completely misunderstood the := operator.

— Reply to this email directly or view it on GitHub https://github.com/steveyen/gkvlite/issues/1#issuecomment-12207222.

steveyen commented 11 years ago

yeah, i just realized that, so time to scrub a lot more code. thanks for catching this.

steveyen commented 11 years ago

Made union() and several other functions look taller and more correct. Plus, now I think I understand :=