[Qtopia-interest] How to avoid copy and paste, qmailstore.cpp as an example

[Holger Freyther]

Hey,
Lorn asked for patches. So I took the time to go through the code and do 
changes. You have asked for the patches, so take your time and comment on 
them.

A common idiom in the Qtopia code base is to use code cloning by copying one 
implementation to another source file. E.g. last time I counted Qtopia had 
three or four copies of an inefficient yuv->rgb color conversion code. I fell 
vicitim of the code cloning with the QSimInfo (IIRC), and a funny one in the 
ficgta01 relative volume adjusting path (because on the conversion from OSS 
to alsa someone removed the code to get the current volume... *bright idea*).

The main issue with code cloning as used and forced by Qtopia is the issue of 
maintainenance [1]. People buying (into) Qtopia have to maintain a code base 
without any history. They do not know why two copies of the code exist, where 
the similarities are and what the differences are. They can only guess about 
the reasoning of the clones. You force a maintainenance nightmare on vendors 
with these practices.

Q: So how to avoid code cloning?
A: Disable copy and paste in your editor.
A: Before you clone code, think how to abstract it. E.g. this is what makes Qt 
so great, and is the reason people give money to Trolltech. Most of the time 
the abstraction is just right.
A: Not cloning code.

Let us take a look at qmailstore.cpp. This class allows one to get information 
about messages and folders. Conceptually the two have nothing in common, the 
code needed to gather this information from the database is identical though. 
We  will take a look at ::queryMessages and ::queryFolders which are 
currently an almost exact copy (even the qLog in ::queryMessages is saying 
Folder Query).

0) Rule of thumb: If you have code that is more than a page long and is 
exactly the same as another piece of code you do something wrong and have to 
abstract that... 

1.) 0001-qtopiamail-Properly-indent-the-source-code.patch:
	- Make the code readable.

2.) 0002-qtopiamail-Correct-the-copy-and-paste-error-and-up.patch
	- After copy and pasting the ::queryFolders method someone forgot to update 
the qLog. It is still referring to folders, even if you parse messages.
	- Note: Having two error conditions that print the same message is not 
helpful for debugging. add __LINE__ or use a different string...

3.) 0003-qtopiamail-kill-code-cloning-by-sharing-the-query.patch
	- Ah let us start to stop the cloning.
	- The code is the same, the static types are not. templates to rescue
	- So the cloning is gone.
	- Looking at the size of the resulting binary on x86 shows a slight increase. 
Personally I wouldn't care for this one

4.) 0004-qtopiamail-Add-the-WHERE-in-the-buildWhereClause-l.patch
	- I know you bring up pseudo arguments. So to fight the increase in size, I 
changed a bit of the other code to bring it inline with buildOrClause.
	- Ah the template version + this patch creates a smaller binary.
	- Also interesting but already known (I think thiago blogged about it), the
	  usage of QLatin1String is creating more code than using char* and the
	 QString::QString(char*) c'tor.

5.) 0005-qtopiamail-Alternative-answer-to-code-cloning.patch
	- If you don't like the template approach then a #define is better than 
having the same textual copy in the sourcecode in multiple places.
	- The generated code will be identical to the original version but the danger 
of updating one place and forgetting the others is resolved.

6.) 0006-qtopiamail-Annotations-to-qmailstore.cpp.patch
	- Some hints regarding the lack of any Coding Style. (It is not about tabs 
vs. spaces.. it is the complete lack of any style... making it impossible to 
read the code)
	- Comments regarding other places that have copy and pasted code
	- Mostly inspired by my neighbor. I don't know QtSql too well but it looks 
like you can start a transaction in certain methods and leave them without 
finishing the transaction. This looks wrong.


so this is mostly an example of what one could do.... think about it.

	z.


[1] there are code clonings that make sense, the ext4 code is a copy of the 
ext3 but the differences will be bigger than what ext3 and ext4 have in 
common... but this is not the case with the Qtopia code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-qtopiamail-Properly-indent-the-source-code.patch
Type: text/x-diff
Size: 3093 bytes
Desc: not available
Url : http://lists.trolltech.com/pipermail/qtopia-interest/attachments/20080526/44ef2f54/attachment-0006.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-qtopiamail-Correct-the-copy-and-paste-error-and-up.patch
Type: text/x-diff
Size: 1097 bytes
Desc: not available
Url : http://lists.trolltech.com/pipermail/qtopia-interest/attachments/20080526/44ef2f54/attachment-0007.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-qtopiamail-kill-code-cloning-by-sharing-the-query.patch
Type: text/x-diff
Size: 4636 bytes
Desc: not available
Url : http://lists.trolltech.com/pipermail/qtopia-interest/attachments/20080526/44ef2f54/attachment-0008.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-qtopiamail-Add-the-WHERE-in-the-buildWhereClause-l.patch
Type: text/x-diff
Size: 6975 bytes
Desc: not available
Url : http://lists.trolltech.com/pipermail/qtopia-interest/attachments/20080526/44ef2f54/attachment-0009.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-qtopiamail-Alternative-answer-to-code-cloning.patch
Type: text/x-diff
Size: 3388 bytes
Desc: not available
Url : http://lists.trolltech.com/pipermail/qtopia-interest/attachments/20080526/44ef2f54/attachment-0010.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-qtopiamail-Annotations-to-qmailstore.cpp.patch
Type: text/x-diff
Size: 8387 bytes
Desc: not available
Url : http://lists.trolltech.com/pipermail/qtopia-interest/attachments/20080526/44ef2f54/attachment-0011.bin