Re: WIP patch to reduce rewrite_filename duplication

[Bruno Haible]

Hi Collin,

Object-oriented programming is not easy, and it comes with constant
hesitations and deliberations.

> Less repeated code to maintain.

Yes, this is one of the goals to keep in mind.

The other most relevant questions are:

  - Can the purpose of a class be explained in simple terms?
  - Is a class properly decoupled from the code that uses the class?

  - Are we using the common design patterns (such as getters/setters,
    class-private caching, etc.)?

Here you already have noticed a warning sign:
> The setters also take two arguments which feels a bit unorthodox to me.

Another, bigger warning sign is that you can no longer easily explain
what the GLFileTable is.
Before, it was a data structure that holds file lists during an operation.
After, it is a helper class that holds file lists and also meddles with
file name rewriting as required by the caller. (That's the best description
I can come up with!)

The two warning signs together indicate that, in this case, you are
trying to move too much logic from the GLImport and GLTestDir classes into
GLFileTable.

Don't get me wrong: Moving application code to helper classes can be OK
in other cases. But when done so, it should decouple the class and the
caller. Which is not being done here: The fact that the caller has a
'cache' and a 'config' variable had no impact on the GLFileTable class,
and after the patch it would.

> I haven't pushed this patch yet, but it goes in the correct direction,
> in my opinion.

IMO, the creation of a rewrite_file_name method (singular! you mentioned
a couple of days ago that the ability to rewrite a single file name is
one of the motivations for this refactoring) in a central place goes in
the correct direction. However, moving knowledge about 'cache' and 'config'
into GLFileTable does not.

> > 'tests=lib/' should not occur in the scope of mode == 'copy-file'.
> > But this string is an indicator inside gnulib-tool; users of gnulib-tool
> > should not be allowed to pass it.
> 
> Does it make more sense to check for a filename starting with
> 'tests=lib/' and error out or is it safe to assume the input is clean?

Neither of the two proprosals smells well. Why not add a boolean
'also_tests_lib' parameter to the rewrite_file_name method, that tells
whether to replace 'tests=lib/' or not? This way, the refactoring does
not introduce a functional difference.

> Ideally I would like to use a module-level function or @staticmethod
> there.

Module-level functions are OK. @staticmethod raises questions; in particular,
whether the method belongs there, where you are attempting to put it.

> Any opinions on these ideas? If not, I will probably sleep on it and
> push an improved patch.

In a case like this, it's better if you post the patch for review, rather
than push it too quickly. Trust me: OO takes a long time to learn.

       Bruno