Code review comment for lp:~phablet-team/media-hub/add-logger
Revision history for this message
| Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote | # |
review:
Needs Fixing
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
I have some concerns with this MP:
* I do not really like moving to the printf way of doing things. Why should be drop type safeness? It would be better to use the boost logger more "natively" with "<<" operators, changing the wrapper class to something similar to what Qt does (qDebug() << message, etc.). Even more, using printf way in the end we need to resort to using stringstream in several places where it was not needed before.
* I still see cout and cerr being used in many places. But please do not change that yet until we have an agreement on how the class Logger should be used.
* I do not think we should introduce functions in the Utils namespace that we do not use. Specially taking into account that I would prefer to avoid using the only one that seems to be used, Sprintf :)
* Line sizes in new files are > 100 in many cases
There are also a couple of inline comments.