Date: Mon, 11 Jul 2011 00:37:15 +0200 (CEST)
From: "Michael Offel" <Michael.Offel@web.de>
To: bitcoin-development@lists.sourceforge.net
Message-ID: <97305540.4426247.1310337435268.JavaMail.fmail@mwmweb052>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Importance: normal
Sensitivity: Normal
Subject: [Bitcoin-development] overall bitcoin client code quality
Precedence: list

Hello,
=C2=A0
I would like to start a discussion about code quality to catch some opinion=
s and create an codebase cleanup plan.
=C2=A0
Let me just start with some points I've seen while reading and stepping thr=
ow bitcoin:
=C2=A0
=C2=A0
1. nearly no code documentation
=C2=A0
All I found was the original paper and some partial wiki pages and the over=
all coding style does not help much here. I would love to see some class hi=
erarchy, method descriptions and thoughts in the code. Instead one can find=
 comments like this...
=C2=A0
>=C2=A0=C2=A0=C2=A0 // Map ports with UPnP
>=C2=A0=C2=A0=C2=A0 if (fHaveUPnP)
>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MapPort(fUseUPnP);
=C2=A0
This comment is just waste of valuable disk space and time for anyone who r=
eads it. While I can guess what the CScript class does I would more like to=
 understand the thoughts behind the decision to implement some crypto macro=
s in a compile time interpreter and why Berkeley db 4 is used at all.
=C2=A0
=C2=A0
2. isolation of modules
=C2=A0
It would be a lot easier to understand parts of the code if they would use =
well defined interfaces to communicate. Bitcoin makes use of global variabl=
es, public member variables in classes and global external functions what m=
akes understanding the code a lot harder. To give an example here, the irc =
module has no interface at use it or to use. It gets some kind of magic thr=
ead started and pushes received addresses directly to some global function =
in net.cpp. The code is full of concepts like this. A well defined interfac=
e would be an bitcoin unrelated IRC module interface and some kind of trans=
lation class between the IRC and peer2peer network interface.
=C2=A0
=C2=A0
3. poor use of threads
=C2=A0
Bitcoin starts a new thread for every little module it has and as soon as t=
here is some serious work to do, it locks some kind of global mutex. While =
this eliminates nearly every performance advantage, it introduces a high di=
fficulty in writing bug free code. Not only that one has to know which mute=
x to lock when, there is no documentation about that, one may also accident=
ly add dead locks. This kind of bug is very hard to find, it may run well f=
or a million of tests and crash or just hang on the next one. And my experi=
ence tells me that it will not be an developer nor an little user when it o=
ccurs. The fist user who hit's the bug will be mt gox doing an emergency tr=
ansfer. ;) My suggestion is to remove all threads and critical sections and=
 build a sequential easy to follow model. Some modules like the cpu miner m=
ay still require to spawn threads, but he should do this internally and hid=
e any synchronization.
=C2=A0
=C2=A0
4. long build times
=C2=A0
It takes longer to build Bitcoin than building some of the million lines of=
 code projects I'm working on. The reasons I did see so far is the use of b=
oost, lack of module isolation and implementations in header files. While t=
he rest is just bad coding style the use of boost is arguable. As far as I =
can see it is mainly used for threading and FOREACH. I already put threadin=
g on the table, but using pthread or making an platform dependent startthre=
ad and mutex would be much more lightweight and nobody needs FOREACH. Boost=
 is also an heavy non standard dependency that is an unnecessary barrier fo=
r new developers.
=C2=A0
=C2=A0
5. style guide
=C2=A0
There is a style guide but it does not include anything about readability.
I'm talking about one file per class, no methods and single code line longe=
r than a screen page. It should be natural to write code like this and I di=
slike having a lot of rules but the code shows that there is a need for suc=
h thing.
=C2=A0
=C2=A0
6. hardcoded values
=C2=A0
There are tons of hardcoded values for the official and the testnet block c=
hain. It would be a great step to move all chain related settings to a chai=
n description file. This would allow custom chains and clean up the code.
=C2=A0
=C2=A0
All this is just the tip of the iceberg. Bitcoin is a widely used applicati=
on and users are transferring millions of dollars. The current code quality=
 is very prone to bug's. To be clear I'm not saying there are a lot of bugs=
 nor do I blame someone for the code quality. It is still a beta version an=
d it was necessary to bring out a working version to attract more developer=
s. And it is hard to analyze the code or just see a bug during development.=
 One can be happy to understand what a method does, but this gives not the =
confidence to see and report obvious bugs.
=C2=A0
Let me also say that I'm not pointing to someone to do all this. I'm willin=
g to spend a lot of time on this promising project but this kind of cleanup=
 is simply too large for one person who is new to the code.
My overall suggestion is to begin a complete rewrite, inspired by the old c=
ode rather than moving a lot of "known to be somehow functional" around.
The official Bitcoin client should be some kind of an reference project for=
 other clients and must therefore be extra clean and well documented.
=C2=A0
Hopefully I did not hurt someone's feelings.
=C2=A0
Michael
=C2=A0
=C2=A0

___________________________________________________________
Schon geh=C3=B6rt? WEB.DE hat einen genialen Phishing-Filter in die
Toolbar eingebaut! http://produkte.web.de/go/toolbar