Move IFDEF's to specialized units

[mdbs99]

In this unit we have the code below:

uses
  Classes, SysUtils,
  {$IFDEF FPC}
    md5,
  {$ELSE}
    IdGlobal, IdHash, IdHashMessageDigest,
  {$ENDIF}

Let's move all $IFDEF from units that is part of API to another specialized units, ie, each unit that can be used by final developers should not have $IFDEF.

I'm thinking of something like that:

1. In interface session there are no IFDEF's
2. In implementation session could be includes files (*.inc) to each compiler or 3rd libraries

implementation

uses
  Classes, SysUtils,
  {$IFDEF lazarus}
    {$include james.fpc.md5.inc}
  {$ELSE}
    {$include james.delphi.md5.inc}
  {$ENDIF}

So, each implementation could be isolated.

[mdbs99]

@nunopicado what do you think?

[nunopicado]

Unfortunately, with IFDEF's, we tend to need them in several moments inside a single unit, sometimes even inside a single method we need to use them more than once.
For instance, in the example shown, conditional directives are needed in the uses clause and in the method body.
Using an inc file like you propose will require one or two inc files for every time a conditional directive is needed (one if there is no ELSE, two if there is an ELSE). In that unit, as is, it would mean 5 inc files, just for that method alone to compile.

I agree conditional directive affect negatively the readability of the code, but I'm not quite sure those inc files would be a solution, rather a bigger problem in the maintainability of the code.

I once had the same problem, and my solution was to have a proxy unit (meaning only one additional file per unit) where the conditional directives would be centralized as functions the main unit would consume without conditional directives. I'm not sure this approach would fit this project concept, though...

[mdbs99]

In that unit, as is, it would mean 5 inc files, just for that method alone to compile.

Sorry but I didn't understand why that.
I'm proposing to have just 3 files (1 unit and 2 inc) to each problem that involves IFDEF's.

[nunopicado]

hmm maybe it was I who didn't understand your idea at first.
To clarify, you propose that whenever a conditional directive is required, all the implementation is moved to a specialized inc file for the platform it represents, is that so?

That would indeed only require 3 files, but wouldn't that also mean duplicated code (for all the code which has the same implementation in both FPC and Delphi) for every unit that requires conditional compilation no matter how small the change is?

[mdbs99]

That would indeed only require 3 files, but wouldn't that also mean duplicated code (for all the code which has the same implementation in both FPC and Delphi) for every unit that requires conditional compilation no matter how small the change is?

Yes and no. It will depend of the implementation, for example, if a classe has so many methods the code will be duplicated to each plataform. But then this code is "wrong", we should decompose it in little parts to minimaze the duplication — only the signature will be duplicate, not the implementation in these cases.

Here an example. Look this unit here and tell me what do you think about this idea.

[nunopicado]

I actually don't like that unit very much. I can't tell why, but I think clauses should be grouped by kind, and the idea of having multiple const clauses throughout the same section of the unit feels a little strange.

That being said, I don't think it is a problem with the number of methods.
In the example given, there's only one method. And for that one method we needed two groups of conditional compilation directives, one for the uses section and one for the method itself.
If you put that into one inc file, you would need to put in both the implementation uses clause and the full class implementation, do you agree?

Now, imagine the following. You have a class with 3 methods and 2 constructors. Still well within reasonable limits, right?
One of those methods requires conditional compilation. What would happen then?
Duplicate code from both constructors and the other two methods in order to have one method conditional compiled?
Remember, 3 methods that belong together in the same class. We can't just break the class and split its implementation, because they do belong together. The only problem has to do with the syntax used for the compiler to understand, not the cohesion of the class or the conceptualization of the object.

What would we do then?

Of course we could put the other methods bellow the {$include} directive, so they wouldn't be in the inc file, thus would not be duplicated. But that feels like spaghetti code. Having a class implemented partially on a file and the rest on another feels itchy.

Besides, if the uses clause sits inside the inc file, we'll have another problem, also with duplicated code.
You have units that are common two both platforms, but will be duplicated in the inc file, so when one has to me refactored, the other might have to be as well.
To avoid this you could keep the common units in the interface section uses clause, and keep the specific ones in the implementation section, inside the inc file. But I don't think the interface section uses clause should be abused, for the sake of minimizing dependencies.

[mdbs99]

You're right... but I didn't think to code this way.
Let's restart and get the code from the first example. Now, after your refactoring, it looks like this:

uses
  Classes, SysUtils,
  {$IFDEF FPC}
    md5,
  {$ELSE}
    hash,
  {$ENDIF}

Then, we have more conditionals:

function TMD5Stream.GetStream: IDataStream;
begin
  Result := TDataStream.New(
    {$IFDEF FPC}
      MD5Print(
        MD5String(
          FOrigin.AsString
        )
      )
    {$ELSE}
      THashMD5.GetHashString(
        FOrigin.AsString
      )
    {$ENDIF}
  );
end;

At a first place, should not exist functions to calculate MD5 inside this object.
We can reuse this algorithm in other place, right?

But this algorithm or better, this MD5 object, has different implementations depending at the compiler.

What I'm proposing is that:

1. We may have two inc files [1]: james.crypto.md5.fpc.inc and james.crypto.md5.delphi.inc
2. Both files will implement the same class, ie, TMD5Hash (for example)
3. Same class name, but different implementations

So, in the main james.crypto.md5.clss unit, into implementation section, we will code:

  {$IFDEF fpc}
    {$include james.crypto.md5.fpc.inc}
  {$ELSE}
    {$include james.crypto.md5.delphi.inc}
  {$ENDIF}

So we can use TMD5Hash class without IFDEF.

What do you think?

---

[1] I improved the namespaces

[mdbs99]

The use of ".inc" instead ".pas" is just to users don't use these files directly... but maybe is better write a real unit and allow users make their choices.
The other "problem" is that james.crypto.md5.fpc.pas, as an unit, looks like a james.crypto.md5.pas specialization, but it isn't.

My main point here is: If you have different implementation to the same concept, we should create others classes, instead of using IFDEF's in just one class.

[nunopicado]

So in the main unit (james.crypto.md5) we would call the object THashMD5, that would sit inside the inc file. Looks better then, but...
I guess my problem with this resides in the use of inc files.
This would force you to have an uses clause inside the inc file, and that's something I'm not confortable with.

What if THashMD5, being an object on it's own, would sit not on an inc file, but rather on a full blown unit? Say, james.crypto.md5.delphi.pas and james.crypto.md5.fpc.pas.

james.crypto.md5 would have only one compiler directive, in its uses clause, and nothing else. It would call the required functions from the respective unit, when needed.
Being in a unit would also mean that the compiler wouldn't have to constantly recompile the object every time james.crypto.md5 is changed.
What do you think? Any advantage of using inc files instead that I'm not seeing?

Actually, this is more or less what I was referring to when I told you up in this thread about the proxy functions.

[nunopicado]

Hadn't read your last comment...
That's it, .pas looks to me like a better option, even if it means users can use it directly.

[mdbs99]

Ok. Let's try this way.
I will do the first refactoring in that unit as soon as possible.

[mdbs99]

@nunopicado please, take a look at these commits above. Try to run into Delphi and tell me what do you think about.
If everything is working and you're satisfied with this solution, please close the issue for me. Thanks.

[nunopicado]

Yes, I think is a much better option than the previous inc file based solution.
Just one question, though:

type
  TMD5Hash =
    {$IFDEF fpc}
      James.Crypto.MD5.FPC.TMD5Hash;
    {$ELSE}
      James.Crypto.MD5.Delphi.TMD5Hash;
    {$ENDIF}

This isn't required for a successful compilation. Since the idea was avoid proliferation of compiler directives, could you please explain the choice to include this? Is this for readability or some other reason I'm not quite following?

[mdbs99]

This isn't required for a successful compilation.

Why not? The TMD5Hash class is an alias for another one. We need to use IFDEF to tell compiler which class it should get.
Maybe we can take both directives (on the uses and declaration type) and put in a new inc file to include just once in MD5 unit.

[nunopicado]

Not necessary, I don't have anything against the alias.
I just asked because in this particular case (and that was the intention), both the Delphi and FPC classes have the same name, so there's no need for an alias. So if you just take out the alias, since any of the compilers will see only the unit meant to be seen, it will work just the same.

[mdbs99]

...both the Delphi and FPC classes have the same name, so there's no need for an alias.

Just to organize and make the developer's life easier — me too.

If this class will be only used in james.crypto.md5.clss I agree with you, is not necessary an alias.
But if I would like to use this class in another place? Should I think about IFDEF's in that place? No, I shouldn't.

So, the goal when I created this alias is to maintain all MD5 classes together in one place for developers do not need to think about compilers and IFDEF's.

Do you see now?

[nunopicado]

So it was about readability... 👍
I thought so, but wanted to be sure.

[mdbs99]

Not only about readability, but about encapsulation: We are encapsulating the cross compiler problem into the classes.
I do not want that Delphi developers need to search what units is for Delphi or not.

[nunopicado]

But they wouldn't even without the alias.
When any developer uses james.crypto.md5.clss.pas, they will use the TMD5Stream without concerns on what unit is doing what inside that object. It doensn't matter if it's a Delphi or FPC developer, they will use the same unit.
It will only be helpful if you want those users to access directly TMD5Hash (without thinking about what unit really holds that class. Is that your idea?
If so, it makes sense, I didn't think you wanted TMD5Hash available outside TMD5Stream.

[mdbs99]

It will only be helpful if you want those users to access directly TMD5Hash (without thinking about what unit really holds that class. Is that your idea?
If so, it makes sense, I didn't think you wanted TMD5Hash available outside TMD5Stream.

Exactly that.
We need to use TMD5Hash outside. In fact, we already do that in MD5 Tests, take a look here.

[nunopicado]

I did. I didn't get the idea before, but it's all clear now.
I like the solution, way better than the previous one with inc files.