[Kde-hardware-devel] Screen Brightness

[Thomas Gillespie]

> Random comments over code quality, not functionality
> 
> device == QString() -> bad, you should use isNull() or isEmpty() depending of 
> what you want
> 
> foreach(QString device, control.keys() -> bad, you should use foreach(const 
> QString &device to avoid unnecessary QString constructions. The same applies 
> to your other foreach constructions
> 
> int getBrightness(QString device = QString())
> I don't know much about Solid naming schemes, but in KDE/Qt in general with 
> omit get for getters, so it should be 
> int brightness(QString device = QString())
> BAD, it should be 
> int brightness(const QString &device = QString())
> to avoid unnecessary QString construction calls
> 
> The same applies for the setter
> bool setBrightness(int brightness, QString device = QString())
> should be 
> bool setBrightness(int brightness, const QString &device = QString())
> 
> There's qRound, you don't need your own round function.
> 
> Albert

Hi,

Thanks for the tips, I think I've got them all, here's version 2.

Thanks

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: brightness.diff
Type: text/x-patch
Size: 15998 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20080420/2bd08a66/attachment-0001.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20080420/2bd08a66/attachment-0001.pgp