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!
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Kde-hardware-devel mailing list Kde-hardware-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-hardware-devel