mdbs99 / james

James is a collection of object-oriented Pascal primitives for Lazarus and Delphi
MIT License
53 stars 21 forks source link

Avoid uses clause in interface section #45

Closed nunopicado closed 7 years ago

nunopicado commented 7 years ago

There are some calls in uses clauses of interface section that could be moved to the implementation section, allowing better structure. Do you agree?

mdbs99 commented 7 years ago

@nunopicado why? For me is easier to see all references in just one section per unit. Besides that, using implementation section is easiest to have circular dependency, which is an anti-pattern at least in Object Pascal.

nunopicado commented 7 years ago

Think of the keywords involved: Interface and Implementation. Like when using interfaces vs classes, interfaces should be a contract and classes should be the implementation of that contract.

Whenever you use the interface section uses clause, you are adding dependencies to the interface. It's unfortunately impossible to completely eradicate dependencies, but as you know we should do our best to keep them as low as possible and away from the contract. This means we should do our best to put all unit dependencies in the implementation section. The interface section uses clause should have the bare minimum to allow the contract to work, if possible, none at all.

By minimizing the units you use in the interface section, you will also force yourself not to use these dependencies on the object contract. Thus, it is in fact a way to reduce the risk of dependencies on the project.

It is also a matter of concept. Reducing the scope of every item tends to be a good idea. The interface section uses clause has a broader scope, applied to the entire unit instead of only the implementation section.

Circular dependency should, of course, be avoided. They should not exist at all, and I can't think of any good reason to use them.

mdbs99 commented 7 years ago

@nunopicado good words, but I still not seeing the benefits. These dependencies still exist, don't matter if they are into interface or implementation section.

Please, could you give some practical example?

nunopicado commented 7 years ago

Code readability: If you keep the interface uses clause to a minimum, you can know just by looking at it what are the contract dependencies. You can literally change the whole implementation section without change any of its contract. Look at issue #42 for instance. To refactor it according to its comment section, you'll have to change its contract. Even if nothing else changed, conceptually that would already be bad.

Enforcing less dependencies: If you declare a global variable, it does not mean automatically you'll have problems with that. If you use a GoTo statement, it doesn't necessarily mean you'll have problems. Unfortunately, when we use a global variable, when we use a GoTo, when we declare public fields or methods, things tend to go south, and sooner rather than later we (or someone else) will be using those resources in a bad way, losing control of the code. Using units in the interface section means you can use those units resources in the contract of the unit. That doesn't mean you will, you can have enough self control not to do it, mas you nevertheless can. And if it can be done, sooner or later someone will. If you put in your interface section only what that interface section needs, and put in the implementation section what it's required by that implementation, you'll have better control over what goes where, and what should or should not be done.

mdbs99 commented 7 years ago

Sorry, but I disagree.

First of all, I believe that we always have to "keep the interface uses clause to a minimum" so this is not a good argument to convince me. :)

I believe that having all units at one place is better for code readability. Besides that, it is better to keep control of the indentifiers. As you know, the compiler — fpc or delphi — compiles in a pre-determinated sequence to read units. Example:

unit MyUnit;
uses
  A, B, C;

When the compiler get into in MyUnit, it read C unit, then B and at last, A. Because that we should use this pattern or sequence:

uses
  // fpc/delphi units,
  // 3rd units,
  // my open source units,
  // units of the project
;

Why? Well, take a look on this explanation here. Did you see the problems about TColor?

Put all units into the interface prevents circular dependency.

nunopicado commented 7 years ago

The TColor example is actually a good one on why the implementation section uses clause is important. Let's review:

{$mode delphi}
unit UnitUsingColors;
interface
uses 
  Graphics;

procedure ShowColor(const Color: TColor);

implementation

uses 
  GoogleMapsEngine;

procedure ShowColor(const Color: TColor);
begin
  Writeln(ColorToString(Color));
end;

end.

This will obviously fail, like stated in the article. And the compiler will let you know right away that something have failed, it will even tell you what failed ("Error: Forward declaration not solved "ShowColor(const TGraphicsColor);") And you will be able to fix it right away.

Now imagine the following cases:

Case A:

{$mode delphi}
unit UnitUsingColors;
interface
uses 
  GoogleMapsEngine, Graphics;

procedure ShowColor(const Color: TColor);

implementation

procedure ShowColor(const Color: TColor);
begin
  Writeln(ColorToString(Color));
end;

end.

Yes, this will work. Better right? Not really! Look now:

Case B:

{$mode delphi}
unit UnitUsingColors;
interface
uses 
  Graphics, GoogleMapsEngine;

procedure ShowColor(const Color: TColor);

implementation

procedure ShowColor(const Color: TColor);
begin
  Writeln(ColorToString(Color));
end;

end.

This won't give you a compiler error, or if it does, it will be in the ColorToString function, not where it is really failing.

Of course, we just have to declare the units in the correct order for it to work correctly. And you know what we call having to put statements, or in this case, unit identifiers, in a predefined order... Temporal Coupling.

Still not convinced?

uses
  // fpc/delphi units,
  // 3rd units,
  // my open source units,
  // units of the project
;

Just try the above TColor example using your own rule for ordering the unit names. What order would you have?

uses
  Graphics, // fpc/delphi unit
  GoogleMapsEngine; // Should we consider this a 3rd party unit?

Now go back to the first example: The compiler will give you an error in the header of the problematic method. You'll just go there and insert a Fully Qualified Identifier Name (FQIN):

{$mode delphi}
unit UnitUsingColors;
interface
uses 
  Graphics;

procedure ShowColor(const Color: Graphics.TColor);

implementation

uses 
  GoogleMapsEngine;

procedure ShowColor(const Color: Graphics.TColor);
begin
  Writeln(ColorToString(Color));
end;

end.

Done...
It will work correctly, and without temporal coupling. You can even look at the interface section and know right away what units your method declarations require.

There are those who believe we should always use FQINs in our code. There are also those who think always using FQINs reveal poor design, because it can demonstrate failure in finding good names. I believe code was not developed by one single person, and thus is possible two developers using the same name in different classes is something plausible and not that uncommon to find, and does not reveal poor design, as it is not one single design, but two (or more) working in cooperation.

So I do believe we should use FQINs whenever we foresee a possibility of conflicting names, instead of relying on temporal coupling the unit names in the uses clause.

mdbs99 commented 7 years ago

Yes, this will work. Better right? Not really!

Why not? If your priority is to use Graphics' identifiers, looks good to me.

Of course, we just have to declare the units in the correct order for it to work correctly. And you know what we call having to put statements, or in this case, unit identifiers, in a predefined order... Temporal Coupling.

Still not convinced?

No, I don't. Using or not declarations in implementation section, we still need to use a predefined order or use the full qualified name. The problem continues, but you can have them in two places (interface and implementation) instead of just one.

Just try the above TColor example using your own rule for ordering the unit names. What order would you have?

The same order I've already told you.

Now go back to the first example: The compiler will give you an error in the header of the problematic method. You'll just go there and insert a Fully Qualified Identifier Name (FQIN):

And I can (or need) do the same, using "my" approach. I don't see the difference.

It will work correctly, and without temporal coupling.

There is not temporal coupling if we use fully qualified name in the whole code.

So I do believe we should use FQINs whenever we foresee a possibility of conflicting names, instead of relying on temporal coupling the unit names in the uses clause.

I agree with you but using the implementation section to declarate units does not solve the problem. IMHO, on the contrary, we are increasing the problem because we need to "sincronize" what identifiers we use up (interface) and down (implementation).

nunopicado commented 7 years ago

There is not temporal coupling if we use fully qualified name in the whole code. The problem continues, but you can have them in two places (interface and implementation) instead of just one.

Indeed, the problem continues... The only difference is the compiler will tell you where the problem really is, instead of giving you an error in an unrelated line, or worse, won't even give you an error, which might happen because you happened to place the units in the correct order or because the conflicting classes are related (inheritance). And giving you an error in the correct line will make it easier for you to know where you must use FQINs.

mdbs99 commented 7 years ago

And giving you an error in the correct line will make it easier for you to know where you must use FQINs.

But, again: Using units on implementation section does not prevent such problems.

nunopicado commented 7 years ago

Like I said, it doesn't prevent it, but helps you fix it when they do happen. That looks like a good thing to me.

mdbs99 commented 7 years ago

...it doesn't prevent it, but helps you fix it when they do happen.

But I didn't see that in a practical example.

nunopicado commented 7 years ago

You can, just reproduce the TColor example from the article you posted. You will get the error "Error: Forward declaration not solved "ShowColor(const TGraphicsColor);" on the line ShowColor is declared (interface section).

mdbs99 commented 7 years ago

Suppose we are doing a API for Google Maps. Using the TColor example above, suppose that GoogleMapsEngine unit belongs to us. We coded it, right? The most relevant identifiers for us is the unit that belongs to the project, ie, the API. If we coded a TColor class into this API we want to work with that specifically class, not the Graphics.TColor unit or whatever.

uses
  // fpc/delphi units,
  // 3rd units,
  // my open source units,
  // units of the project
;

But, if we have units declared up and down, we need to think about names collision problems at up and down!

nunopicado commented 7 years ago

If we coded a TColor class into this API we want to work with that specifically class, not the Graphics.TColor unit or whatever.

Why? You may need to access Graphics.TColor. And GoogleMapsEngine unit may not even be yours, it's just two foreign dependencies you have, and need to access different resources from each one. You may need TColor from Graphics and one hypothetical TMap from GoogleMapsEngine. Who knows.

The fact of the matter is, you can't rely on "who knows" and "what if" to do your own code. You need to make sure you use the correct resource from each unit, and for that, either you use FQINs all the time, for everything, always, or... Rely on the compiler.

And the compiler will be much more reliable if you tell it the most you can, the more accurately possible. Which means using the interface uses clause to declare the dependencies needed for the contract, and the implementation uses clause for the dependencies needed for the implementation.

Think of it this way: You don't want ever, never, ever that details of your implementation leak into the interface. When you create your interfaces in one file, you don't put in the units needed by the class that implements that interface. It wouldn't make sense, right? So why would it make sense to have those units in a unit interface? It just doesn't.

Of course, most of the time it won't cause you any problem. I know that, I've been putting all the units in the interface uses clause for years. Most of the time, worked fine, so why bother? The problem was the few times it didn't work fine. Curious enough, I came to this conclusion (that units should go into their respective sections) only after reading your blog about interfaces. Not by something specific you wrote, of course, but by expanding the concept to other areas.

mdbs99 commented 7 years ago

Why? You may need to access Graphics.TColor.

Yes, but by default we want GoogleMapsEngine.TColor.

And GoogleMapsEngine unit may not even be yours, it's just two foreign dependencies you have, and need to access different resources from each one.

According with my example, Google Maps Engine belongs to the hypothetical project.

Which means using the interface uses clause to declare the dependencies needed for the contract, and the implementation uses clause for the dependencies needed for the implementation.

But the problem occurs when the implementation needs to use units declared on interface, which is most common.

When you create your interfaces in one file, you don't put in the units needed by the class that implements that interface. It wouldn't make sense, right? So why would it make sense to have those units in a unit interface? It just doesn't.

Right for the first question. For the second one, my answer is: Because we need those units declared on interface to do the implementation, otherwise I would agree with you.

I would agree with you that some units may be declared on implementation section, such like utilities, functions, inc files, units that depends on plataform... ie, stuff that is cleary related with implementation in details. But, I would say again: Those units that is cleary related with the design or belongs directly to project, should be declared on interface section.

nunopicado commented 7 years ago

I would agree with you that some units may be declared on implementation section, such like utilities, functions, inc files, units that depends on plataform... ie, stuff that is cleary related with implementation in details. But, I would say again: Those units that is cleary related with the design or belongs directly to project, should be declared on interface section.

Aren't the units related with design only the ones needed for the contract?

All others are related to the implementation, so this would takes us to my point exactly: Leave the units required by the contract in the interface section, and all the others move to the implementation section.

mdbs99 commented 7 years ago

Leave the units required by the contract in the interface section

By contract you mean...

I'll give you an example: Look this unit here at this line. We have a dependency to Synapse lib (synacode unit) We don't need it on interface because we don't use any classes from it. We just use a single function here called EncodeBase64. So, this specific unit could be moved to implementation section, right?

Not so fast.

Imagine in the future that we will have a THTTPClient class, which already exists in Synapse, and now we need to use this class on that unit, on the interface. When we put it on the interface and try to use on the implementation, the compiler will give me an error. Why? Because this class already exists in synacode unit, declared before!

If we had continued using it in the interface, we wouln't have an error now.

nunopicado commented 7 years ago

Yes, but that is the compiler doing its job. No harm will ever come of it. He'll warn you it's redeclared, you remove the second declaration, and no bugs were introduced. The interface changed, you code accordingly, and the compiler will check if the code is compilable.

If we were to be thinking about that, we wouldn't create any interfaces, because they might change in the future.

mdbs99 commented 7 years ago

Yes, but that is the compiler doing its job. No harm will ever come of it. He'll warn you it's redeclared, you remove the second declaration, and no bugs were introduced. The interface changed, you code accordingly, and the compiler will check if the code is compilable.

We need to do all these stuff only because we used implementation section... otherwise, no changes would be needed.


@nunopicado We're talking a lot about this and we still disagreeing. I would like to close this issue without any changes because I still do not believe that is good. If in the future we see a practical example that is better to use implementation section, we can return on that issue — or make a new one — and talk about change all units.

Do you agree?

nunopicado commented 7 years ago

Yes, I agree. Let's not waste our time for now on this. We'll get back to it some other time, if needed.

mdbs99 commented 7 years ago

Yes, because we have much work to do. Thank you!