From kde-hardware-devel  Sun Aug 03 22:59:25 2014
From: =?ISO-8859-1?Q?=C0lex?= Fiestas <afiestas () kde ! org>
Date: Sun, 03 Aug 2014 22:59:25 +0000
To: kde-hardware-devel
Subject: [Kde-hardware-devel] qbluez review
Message-Id: <1996310.QRKI4C5fJF () minibad>
X-MARC-Message: https://marc.info/?l=kde-hardware-devel&m=140710678203199
MIME-Version: 1
Content-Type: multipart/mixed; boundary="--===============5225603221292631055=="


--===============5225603221292631055==
Content-Type: multipart/signed; boundary="nextPart1444948.0ET8c9VoSC"; micalg="pgp-sha1"; protocol="application/pgp-signature"


--nextPart1444948.0ET8c9VoSC
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="us-ascii"

Hi there!

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

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

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

To do the tests we have two options:
=2DWe implement a fake bluez
=2DWe 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:
=2DThere is no api documentation
=2DManagerPrivate::init is as complex as it was before, it should be brok=
en down=20
into small pieces (replace lambdas into their down objects or methods (=
lambdas=20
are almost impossible to test correctly, and impossible to test using m=
ock)
=2DManagerPrivate::load same as init
=2DInitManagerJobPrivate::initFinished can use some , also remove lambda =
and all=20
a real callback (again so we can unittest it better)
=2DAdapterPrivate::load replace lambda with a slot (again for testability=
)
=2DAgentAdaptor for the whole object, remove lambdas and return early
=2DDevicePrivate::propertiesChanged maybe split name/alias/class cases in=
to=20
their own methods for code clarity?
=2DLoadDeviceJobPrivate::doStart, replace lamda with slot
=2DManagerPrivate::clear split into pieces
=2DManagerPrivate::interfacesAdded split into pieces (in the Adapter1 cas=
e=20
return early as well)
=2DManagerPrivate::interfacesRemoved same as added.
=2DObexAgentAdaptor::AuthorizePush s/lambdas/slots/
=2DObexManagerPrivate::init: Split into pieces, remove lambdas and return=
 early.
=2DObexManagerPrivate::load: same as init
=2DObexSessionPrivate::init: Remove lambda
=2DObexTransferPrivate::init: Split, and remove lambdas
=2DPendingCall::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=20
coupling to the code and make the code way harder to test. So please re=
place=20
them.

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

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

everywhere.

Cheers!
--nextPart1444948.0ET8c9VoSC
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABAgAGBQJT3r7SAAoJEGCsZ2zsJAWe2WsP/0B6BoFJlk3lgdKY6oWSvJbQ
+A1gs9GntCF5xmvPvNzw+oA7pR6I6HhSJmd2TVDe/mnJebCAQfg6SSJB3751EyW1
SyfRTaKDzP52P+TFaJWvT72TH9gzSla9UHjqK24sh3aeaplk2uNl3Cc4H13tWGpI
kfRFKn9GBn7vedvhGRRXMFuu3kVEm5EUkiFOOFpYyUQuagLnGSGuPXl/KiJl8jmw
xq7hKStZnlBWIHRrcYCRaZd9CSnDceL5AWejZIYoSSLeBNlb7p6oLHWNXS0GwEJd
j4KEWhUZ8COYe7FTFBP0r993RxrgjOoECO2bfiTcEpe6Hdt0XQz0QlqwB/DuPz+4
rIhtktCUAZKFnKXnWR96JYZSbVQthSk+gOecBJvUl3fxagwxTKYd1jjuNqrxwAQJ
lt3pAbN/cRKZq/vPSfvIE1oZpGW/qyjYJDmeWXqmAzX9vqqwtKLRY/sWn6qiNfXK
W4YMEoHv7OnJKh9WMvVWcvVbrPWL4iyHdlHGy0x5qWyaOxh0ab69fq12YguLpv4i
3xi2dJB8NDS9ncqNI95DgTVme6X6Eu/cnMzGA38obdek4rPD24e6fn+8DItT8q9A
ej7gv+Hr7qeBzvA7codg0oqqUwA0yKJHHjwAxE+HCSK8plesx9gkDlE+aJmmCTGt
ttMzQ1nTGYKfklExfzwD
=GGIv
-----END PGP SIGNATURE-----

--nextPart1444948.0ET8c9VoSC--


--===============5225603221292631055==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Kde-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-devel

--===============5225603221292631055==--