[Kde-hardware-devel] qbluez review

[Àlex Fiestas]

Hi there!

I have been doing some more review on qbluez, as I already told you I think 
you have done (and you are doing) and awesome job.

Do NOT be scared by the length of the email, mostly everything are small 
things to re-factor in order to make the code more testable or easier to read.

First and most important:
We are lacking a lots of autotest, both unit and integration tests. Would be 
nice if from now until the end of GSoC we can port bluedevil and while doing 
it add these missing tests.

To do the tests we have two options:
-We implement a fake bluez
-We use mocks (https://code.google.com/p/googlemock/)

Either of them is good by me.

Some stuff I have found that will be nice to get sorted out:
-There is no api documentation
-ManagerPrivate::init is as complex as it was before, it should be broken down 
into small pieces (replace lambdas into their down objects or methods (lambdas 
are almost impossible to test correctly, and impossible to test using mock)
-ManagerPrivate::load same as init
-InitManagerJobPrivate::initFinished can use some , also remove lambda and all 
a real callback (again so we can unittest it better)
-AdapterPrivate::load replace lambda with a slot (again for testability)
-AgentAdaptor for the whole object, remove lambdas and return early
-DevicePrivate::propertiesChanged maybe split name/alias/class cases into 
their own methods for code clarity?
-LoadDeviceJobPrivate::doStart, replace lamda with slot
-ManagerPrivate::clear split into pieces
-ManagerPrivate::interfacesAdded split into pieces (in the Adapter1 case 
return early as well)
-ManagerPrivate::interfacesRemoved same as added.
-ObexAgentAdaptor::AuthorizePush s/lambdas/slots/
-ObexManagerPrivate::init: Split into pieces, remove lambdas and return early.
-ObexManagerPrivate::load: same as init
-ObexSessionPrivate::init: Remove lambda
-ObexTransferPrivate::init: Split, and remove lambdas
-PendingCall::processReply: split it in pieces, remove lambdas


TLDR:
As you can see I don't like lambdas and the only reason is because they add 
coupling to the code and make the code way harder to test. So please replace 
them.

Also, in some places we need to split methods (I know you inherit those from 
old code, but it is a good time to clean them). These huge init/load methods 
gare die they make qbluez super complex to read.

Finally, you have used "return early" in most places, I'd say use it 
everywhere.

Cheers!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20140804/1f876e1d/attachment.sig>