Reviewing NChord: A codebase that makes me twitch
I was referred to NChord by one of the comments, and it looked very interesting on the surface, so I downloaded the code and took a peek.
It is not typically my style to criticize code openly, especially after a very brief glance, but I thought that it might be a good idea to point some of the things that made me decide that the codebase isn't worth the time to pursue in details.
Single Responsibility Principle doesn't apply to the file level
What you see here are no less than 11(!) files that are all parts of the same class. That is.. bad, from a whole lot of reasons, not least of which is that you have to jump through a lot of files to understand what is going on, and that ChordInstance is at least a demi-god, rapidly moving upward.
Then we have things like this:
Now, this is a matter of taste more than anything else, but it strongly hints that there is lack of threading experience going on here. Indeed, a brief look at the code shows numerous potential race conditions.
Code like this really make me twitch:
All of that trend toward making me doubt the maturity and stability of the codebase.
In short, after a very brief scan, I find numerous issues that I really didn't like in the code base. And I am not willing to put up the effort to try to make headway into this with so many early warning signs already in place.
Comments
I had a similar experience with a popular, I think, open source cms. I couldn't even get the trunk version to compile, and that was all that were provided. I am donating my time to a non-profit in this case, and I don't want to waste it on getting this thing runable. That's a shame, really. The docs and samples are good, the codebase looks decent, and building a site on this also looks easy.
But not when the thing won't compile.
No matter if the code is beautiful or not as long as software works ok and you are interested in its usability and reliability, not internal structure. Remember this is someone's work given to you for free so if it does its job even partially, dont complain. Remember, a bird in the hand is better than two in the bush.
@Rafal: This is a third party library for highly scalable systems that we are talking about, not a personal to-do list. It's the kind of thing where the code base needs to meet a high standard, otherwise you're in for all sorts of problems.
That exception handling code looks particularly painful. When somebody's catching System.Exception without a clear and detailed explanation of why they are doing so, it shows that they are either sloppy coders or else pretty clueless.
I think its pretty important to understand the software structure, things like above examples will make me worry about:
a) maintainability, is this going to be really tricky when I go 'out of the norm'?
b) reliability, are these design quirks going to cause me hard to track / remedy bugs?
Yes, yes, but are there are other ways to cover yourself.
Are there unit tests which cover a large amount of hte code (or branches)?
Are there testimonials of other users?
Have you tried using it to see if it fits?
Oren, I agree, and I wouldn't write software like that, but basically - so what? This, in my mind, is another manifastation of the NIH syndrom, just with another set of more technical excuses.
Disappointing that the code has "issues". So often the case with c# projects that are ported from other languages.
Still, my intent in pointing you at NChord was that Chord itself ( http://pdos.csail.mit.edu/chord/) seems to have put a lot of thought into what you were trying to discover in your previous post on designing Rhino DHT. Namely how to deal with a DHT on a dynamic cluster (scaling, balancing, node failure etc)
See the paper at http://pdos.csail.mit.edu/papers/chord:sigcomm01/
@Arielr: Unit tests don't necessarily guarantee that your code is bug free -- especially not when you're writing things like "catch (Exception ex)" -- which translates into English as "I don't care if this code doesn't do what it's supposed to do" or into unit-test-ese as [IgnoreAttribute].
@Rafal,
If I can't trust the code, I am not going to use it
Arielr,
For NChord:
1) no unit tests
2) no other users that I know of.
3) I don't have confidence in the codebase, when I can find pretty severe bugs in cursory examination, it isn't going to cut it.
Andy,
I read the article with great interest. It is interesting, absolutely, but it has two problems that I don't like:
1) it is O(log N), whereas I want O(1) in most scenarios
2) there is no real handling for actually moving data between nodes as they come up and down, which is the most problematic scenario from my view point
Hi, could you clarify what you mean when you say you want O(1)? Is this to do with the finding of a key's data or the lookup for a node in the DHT or is it a combination of both? I am still not clear on how this could always be O(1)?
Gavin,
I mean that given a key, I know which server to go to.
That can be done if I store the topology of the network on the client side.
I prefer O(N) simply because it provides an orthogonal consistence approach to signatures. The consuming method can always just pull top and it makes looping constructs easier.
That said I'm not hardcore about it and will provide O(1) overloads for specific business rule uses. Generic collections are memory efficient and the nominal allocation is more than made up for by predictable code that can be maintained by a wide range of skillsets.
BTW I'm not making an argument for using this codebase. Just responding to the general philosophy.
Observer,
O(X) is really important when you are talking about distributed apps. Any call is expensive.
Ayende, I'm not sure about the NChord project but the original Chord project objectives is to actually distribute the data and the keys. O(1) is not an objective of Chord based lookup because it's not an objective of DHT. I think, and correct me if I'm wrong, O(1) would require each node to know about all of the other node in the system... therefore requiring each node to contact every other node during a change of membership. Chord limits the updates to something like a dozen RPCs for nodes entering and exiting the network.
Cali,
Yes, you are right.
Different requirements, different rules.