[Scummvm-devel] GSoC patch for trunk

[Ender]

> Hey everyone, I created a patch some days ago to merge my work into trunk.
> I decided to take the conversation here to give it more exposure from other
> devs :)

Disclaimer: I'm reading this e-mail at home on my dialup, and dare not
checkout your branch ATM... So all my comments are based purely on this
e-mail and please forgive any stupidity ;)

>>* The "backend/factories" dir is badly named, IMO, as "factories" could
>> be
>>for anything, but you use it for *FS node* factories exclusively.
>> However,
>>instead of renaming this, I propose that you simply merge all of its
>>contents into the existing "backends/fs" dir. So the files in
>>"backends/factories/posix" would end up in "backends/fs/posix".
>>
> Actually, the factory files used to reside in the same directories as the
> FSNode implementations but I moved them out on purpose. I'm working on a
> BaseFile class which will use the same factories, so I decided that having
> a factory for file objects in the fsnode directory would be misleading.
> Also, if any other backend (say, the plugins one) switches to factory-based
> instantiation, the factories folder would be the ideal place to put a new
> kind of factory into.
>
> For me, it's a matter of clarity. You can check out my branch to see the
> new "file" folder and how the factories seem better in their own folder
there.

Given the international nature of ScummVMs developers, I'm of the opinion
that a more generic name like 'fs' or 'file' would be preferred if all
code relates to FS access. Whilst the idea of a class factory is in theory
a generic concept, I've really only encountered the term frequently in
code either COM/MFC/Win32 (or Java) originating/derived code - although
maybe that's just me!

>* Same file: any good reason to introduce a whole FilesystemFactoryMaker
>>class, just for a single static method in it? This just adds code and
>> fake OO boiler plating, w/o giving any advantage (or does it?). Why not
use
>> use a global function, like in the good old C days? :-)
>>
> This is another place where I chose a separate file for clarity. I can add
> the static method to the abstract-fs-factory.h file if you'd prefer it
> that way.

Clarify == Good. On the other hand, back 'in the day' when we began OO'ing
ScummVM heavily I took a pretty strong stance against 'adding a new file
for files sake'. Eg, in my opinion readability can be impacted if code is
fragmented / split out into too many files. This philosophy isn't one of
the stated design guidelines of ScummVM, but has pretty much carried on
unofficially. Unless there is a substantial amount of code, or the code
implements sufficient unique functionality that it cannot be placed in a
existing file reasonably, I recommend keeping the number of files to a
minimum.

Also, the general existing guideline is to keep such small static members
in the header. I'd point to a few examples of this, but I'm having trouble
with ViewCVS and our DOxygen stuff seems broken at the moment ;)



.. And thats pretty much the only two issues I dare comment on until I get
a chance to actually look at the code!

Cheers,

 Ender