Code review comment for lp:~compiz-team/compiz/compiz.fix_1064791

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

> I am concerned about very long identifiers...
> typedef struct _CCSGNOMEIntegrationGSettingsWrapperFactoryPrivate
> CCSGNOMEIntegrationGSettingsWrapperFactoryPrivate;
> I cannot read that. The words are too long for my feeble human brain to
> recognize symbolically. And I'm sure most humans would agree. It is however
> very important for code to be readable by human beings for the sake of
> maintenance.
>
> I know there's a lot of existing code written that way already. However it was
> landed without review due to limited time before release. So I'd like to say:
> No more new code with unreadably long identifiers please.

Yeah this sucks, but it does accurately describe its role. Its a libcompizconfig gnome-integration gsettings-wrapper factory private-data. That's not really something that can be shortened, and c's lack of namespaces makes it a lot harder to do so. I can perhaps drop the CCS and Integration to just have GNOMEGSettingsWrapperFactoryPrivate or just WrapperFactoryPrivate since its exclusive to this file.

>
> AFAIK, C/C++ should only accept up to 32 characters. And I've never seen
> anything even that long before now.

That would be a god-awful old compiler that did that. Sure you're not typing this on a VT-100 ;-) ?

>
> Secondly, "Bug #1064791: Redundant writes to plugins-with-set-keys" suggests a
> very simple problem that should have a very simple solution. So why "1663
> lines (+1030/-243) 21 files modified" ?

Tests basically, and the fun-stuff that comes with that. I need to instantiate CCSGSettingsBackend independently, and it turned out that this wasn't trivial with the current design because CCSGSettingsBackend creates a lot of objects on construction.

« Back to merge proposal