4

I was modifying a class whose job is to load other classes from the same directory. Basically, everytime a new class is added you need to add a new CommandClass; statement to this class for it to be loaded.

I thought that it's strange and violates "Closed for modification" principle, so instead of adding another line to existing 70 of them, I changed it to autoload all these classes.

After a while two people argued that with autoloading those classes, they can't find where these classes are used, i.e. they can't grep for the class names to locate files that are using them.

Considering that others have already been doing these for more than seventy times, I reverted my change (and add another one of those lines). However, I'm still not sure, does that argument holds any real weight? What better way is there to satisfy their need to locate code?

2
  • Can't the program tell you this information? – coredump Jun 24 '16 at 11:44
  • 1
    @coredump even if it can, that's not their point. They want to be able to e.g. git grep ClassName and see what files would show up. – imel96 Jun 25 '16 at 2:03
8

In general, whenever using dynamic loading or accessing techniques, you will have to deal with the problem that your typical search, refactoring or cross-referencing tools cannot easily determine any more which code is used where, what to refactor or from where a piece of code is called.

So whenever you consider to use such a technique, be aware that this is a trade-off - on the pro side, your code gets more DRY, or it will conform to the OCP, and so becomes more easily extendable. The latter is especially helpful when you create a library or framework where the user of that lib is not able to change or extend the lib source code arbitrarily. On the con side, the whole mechanism becomes less obvious. If you use these techniques too often, there is a certain risk the code might become hard to understand or convoluted. Therefore, you will have to make a decision on a per-case base, which approach will serve you better in total.

In your specific case, it sounds there might be a little bit of superstition of your colleagues involved - do they really require to find the part of the code which holds the "load" class by using grep and search for a specific class name? If, for example, one name of the classes will change, your autoloader won't have to be changed, so no need to grep for it. And 70 repetitions of similar code is surely a code smell - when in the future you get a requirement which causes the same kind of change to every of those 70 lines, then an autoloader will show its real value.

On the other hand, changing the loader everytime a new class is added does not seem to be much of a problem in your current situation, because you have full control of the source code, and there is hopefully not really much code repeated. I guess when you have to add a class, it cannot easily be forgotten to extend the loader (your system would probably not be able to run the code of the new class). And when one of your team wants to keep some old versions of the classes in the directory, but does not want those to be loaded (for example, for testing purposes), a "whitelist" of classes to be loaded can have some benefits, too.

In the end, you have to consider these two sides and make a decision for your case.

2
  • "do they really require to find the part of code ..." Yes, in code review, one put "I want to be able to search..." So, I made the decision (to revert the change) but only for diplomatic reasons. I agree with the whitelist as I also thought about using config file, but that would be another can of worms. – imel96 Jun 25 '16 at 2:23
  • @imel96: as long as it is only one new CommandClass per new class, as long as the "loader" is not maintained from someone with different access rights to the source code as the command classes, and as long as you do not have a requirement which makes you change those 70 new CommandClass lines in a similar manner, I don't see why using the autoloader will bring you any real benefit. The OCP is not an end in itself, the idea is that it solves you some problems. If you do not have any of those problem to be solved, there is no need to apply it. – Doc Brown Jun 25 '16 at 6:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.