path: root/fb/ba4bb6ef982406b59511845c794b59aec1242d

Received: from sog-mx-1.v43.ch3.sourceforge.com ([172.29.43.191]
	helo=mx.sourceforge.net)
	by sfs-ml-4.v29.ch3.sourceforge.com with esmtp (Exim 4.76)
	(envelope-from <Michael.Offel@web.de>) id 1Qg2cs-0003wz-35
	for bitcoin-development@lists.sourceforge.net;
	Sun, 10 Jul 2011 22:37:22 +0000
Received: from fmmailgate05.web.de ([217.72.192.243])
	by sog-mx-1.v43.ch3.sourceforge.com with esmtp (Exim 4.76)
	id 1Qg2cr-0006zl-1e for bitcoin-development@lists.sourceforge.net;
	Sun, 10 Jul 2011 22:37:22 +0000
Received: from mwmweb052  ( [172.20.18.61])
	by fmmailgate05.web.de (Postfix) with ESMTP id 44D446536A6B
	for <bitcoin-development@lists.sourceforge.net>;
	Mon, 11 Jul 2011 00:37:15 +0200 (CEST)
Received: from [87.194.33.247]  by  mwmweb052  with HTTP; Mon
	Jul 11 00:37:15 CEST 2011
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
X-Priority: 3
Importance: normal
Sensitivity: Normal
X-Provags-ID: V01U2FsdGVkX18Qv1qUCYkGzbCzkTWu/6PBTI3+cel9daLWfQQlVWvLL7Np1wFpYzwg
	eZ7eJJvp6GRVTzi/yqttLyGy5PeGNkRx
X-Spam-Score: 0.0 (/)
X-Spam-Report: Spam Filtering performed by mx.sourceforge.net.
	See http://spamassassin.org/tag/ for more details.
	0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider
	(michael.offel[at]web.de)
	-0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/,
	no trust [217.72.192.243 listed in list.dnswl.org]
	-0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay
	domain
	0.0 T_TO_NO_BRKTS_FREEMAIL To: misformatted and free email service
X-Headers-End: 1Qg2cr-0006zl-1e
Subject: [Bitcoin-development] overall bitcoin client code quality
X-BeenThere: bitcoin-development@lists.sourceforge.net
X-Mailman-Version: 2.1.9
Precedence: list
List-Id: <bitcoin-development.lists.sourceforge.net>
List-Unsubscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
	<mailto:bitcoin-development-request@lists.sourceforge.net?subject=unsubscribe>
List-Archive: <http://sourceforge.net/mailarchive/forum.php?forum_name=bitcoin-development>
List-Post: <mailto:bitcoin-development@lists.sourceforge.net>
List-Help: <mailto:bitcoin-development-request@lists.sourceforge.net?subject=help>
List-Subscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
	<mailto:bitcoin-development-request@lists.sourceforge.net?subject=subscribe>
X-List-Received-Date: Sun, 10 Jul 2011 22:37:22 -0000

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