3

Under the context of dependency injection - that is, an interface has mostly one implementation - I took the habit of exposing via the Interface a bunch of fields which are never called by the consumer classes. These fields reflect a high-level implementation strategy; I decided to expose them via my interfaces because I feel it helps to understand how the abstraction works - or let's say what is expected from that abstraction - not in terms of detailed implementation, but in terms of high-level principles and why the object/interface was needed in the first place.

For example, I have an interface IEncrypter which encrypts strings. The idea behind this interface is to be able to choose which encryption algorithm to use. So the implementation class takes an algorithm abstraction IEncryptionAlgo in its constructor, and stores it as a ReadOnly field: ThisEncryptionAlgo As IEncryptionAlgo. Then when I call Encrypt(Message), it calls ThisEncryptionAlgo.Encrypt(Message).

Strictly speaking, the interface does not need to expose ThisEncryptionAlgo, and exposing the Byte() Encrypt(Message As String) function alone is sufficient for the consumer. However, I feel that having the Interface exposing ThisEncryptionAlgo (as a ReadOnly) has some advantages:

  • You help developers to understand the spirit behind the interface, which is useful both when implementing and when consuming.
  • You make debugging easier as you can quickly inspect the property directly from the interface.
  • Error logging and tracing might be easier if you generate a report based on the interface properties.

I believe it is ok because the main purpose of having this as an interface rather than a concrete class is to allow dependency injection and unit testing of the consumers, not to add a true layer of abstraction. Having said that, it also defeats the principle that interfaces should disregard any implementation details.

What is your opinion? Should I remove ThisEncryptionAlgo from my interface?


Full Example

Interface IEncryptionAlgo

    Function Encrypt(Input As Byte()) As Byte()

    Function Decrypt(Input As Byte()) As Byte()

End Interface


Interface ICheckSumAlgo

    Function GetHashSum(Input As Byte()) As Byte()

    ReadOnly Property HashLength As Integer

End Interface


Interface IEncrypter

    ReadOnly Property ThisEncryptionAlgo As IEncryptionAlgo

    ReadOnly Property ThisCheckSumAlgo As ICheckSumAlgo

    Function Encrypt(Message As String) As Byte()

    Function Decrypt(Cypher As Byte()) As String

    Function EncryptWihCheckSum(Message As String) As Byte()

    Function DecryptWithCheckSum(SumAndCypher As Byte()) As String

End Interface


Class Encrypter
    Implements IEncrypter


    Private Sub New()
    End Sub

    Sub New(Algo As IEncryptionAlgo, CheckSum As ICheckSumAlgo)
        Me.ThisEncryptionAlgo = Algo
        Me.ThisCheckSumAlgo = CheckSum
    End Sub


    Public ReadOnly Property ThisEncryptionAlgo As IEncryptionAlgo Implements IEncrypter.ThisEncryptionAlgo

    Public ReadOnly Property ThisCheckSumAlgo As ICheckSumAlgo Implements IEncrypter.ThisCheckSumAlgo


    Public Function Encrypt(Message As String) As Byte() Implements IEncrypter.Encrypt

        Dim MessageBytes As Byte() = Text.Encoding.Unicode.GetBytes(Message)

        Dim Cypher As Byte() = Me.ThisEncryptionAlgo.Encrypt(MessageBytes)
        Return Cypher

    End Function

    Public Function Decrypt(Cypher() As Byte) As String Implements IEncrypter.Decrypt

        Dim MessageBytes As Byte() = Me.ThisEncryptionAlgo.Decrypt(Cypher)

        Dim Message As String = Text.Encoding.Unicode.GetString(MessageBytes)
        Return Message

    End Function


    Public Function EncryptWihCheckSum(Message As String) As Byte() Implements IEncrypter.EncryptWihCheckSum

        Dim Cypher As Byte() = Encrypt(Message)
        Dim CypherSum As Byte() = Me.ThisCheckSumAlgo.GetHashSum(Cypher)

        Dim SumAndCypher As Byte() = CypherSum.Concat(Cypher).ToArray
        Return SumAndCypher

    End Function

    Public Function DecryptWithCheckSum(SumAndCypher() As Byte) As String Implements IEncrypter.DecryptWithCheckSum

        Dim Cypher As Byte() = SumAndCypher.Skip(Me.ThisCheckSumAlgo.HashLength).ToArray
        Dim ExpectedCypherSum As Byte() = SumAndCypher.Take(Me.ThisCheckSumAlgo.HashLength).ToArray
        Dim CurrentCypherSum As Byte() = Me.ThisCheckSumAlgo.GetHashSum(Cypher)

        If Not CurrentCypherSum.SequenceEqual(ExpectedCypherSum) Then Throw New ArgumentException("Check sum failed.", NameOf(SumAndCypher))

        Dim Message As String = Decrypt(Cypher)
        Return Message

    End Function


End Class

Class ServicesFactory
    Implements IServicesFactory

    Function NewEncrypter() As IEncrypter Implements IServicesFactory
        Return New Encrypter(My.AppSettings.GetDefaultAlgo, My.AppSettings.GetDefaultCheckSum)
    End Function

End Class


Class ConsummerClass

    Private ReadOnly Property MainFactory As IServicesFactory

    Private Sub New
    End Sub

    Sub New(MainFactory as IServicesFactory)
        Me.MainFactory=MainFactory
    End Sub

    Sub Main()

        Dim MyMessage As String = InputBox("Write something")

        Dim Encrypter As IEncrypter = Me.MainFactory.NewEncrypter
        Dim EncryptedMessage As Byte() = Encrypter.Encrypt(MyMessage)

        WebClient.SendPost(Convert.ToBase64String(EncryptedMessage))

    End Sub

End Class

As one can see, it is "by design" that IEncrypter holds a field which refers to the algo object to use. If one wants to use a different algo, they may implement IEncryptionAlgo and inject it via the ServicesFactory. Under such context, ThisEncryptionAlgo is not needed by the consumer class, but having it exposed via the IEncrypter interface ensures any implementation of the later fits the overall architecture. At least, that is what I intuitively feel, but I'd like to challenge this.

16
  • My question might as well apply to concrete classes: Should I make ThisEncryptionAlgo private?
    – Ama
    Jan 30 '20 at 16:42
  • 1
    You make multiple claims about interfaces which are very dubious, such as "an interface has mostly one implementation" or "having that interface is to allow dependency injection and unit testing, not to add a true layer of abstraction". You conflate interfaces and dependency injection. An interface is an abstraction, and you shouldn't create one if you don't need the abstraction! The entire point is to decouple the client of your abstraction from its actual implementation. Jan 30 '20 at 16:55
  • There are different levels of abstraction. In my example, you may distinguish the encryption algo, from who chose which algo to use, and from how it was offered to choose an algo. The consummer does not care which algo was choosen, but exposing which algo was choosen indicates the fields exposed by that same object are tight to this choice. If that makes sense?
    – Ama
    Jan 30 '20 at 17:27
  • @Ama Please create an MCVE to make it more clear what you mean.
    – xtratic
    Jan 30 '20 at 17:45
  • 1
    @xtratic This isn't SE, this is softwareengineering; we don't require MCVEs here, this exchange is for whiteboard designing things.
    – Andy
    Jan 30 '20 at 23:21
3

I like that you are thinking about knowledge management, which to me is very important, and I'm not completely against small compromises in architectural elegance if they improve the productivity of your development team.

That being said, I believe your readonly fields connote something very different about the class.

Let's examine the two options.

Interface IEncrypter

    Function Encrypt(Message As String) As Byte()

    Function Decrypt(Cypher As Byte()) As String

    Function EncryptWihCheckSum(Message As String) As Byte()

    Function DecryptWithCheckSum(SumAndCypher As Byte()) As String

End Interface

The above is a black box that is capable of encrypting and decrypting strings. If that is all your application needs, that is all you need to specify in the interface that is being injected.

Interface IEncrypter

    ReadOnly Property ThisEncryptionAlgo As IEncryptionAlgo

    ReadOnly Property ThisCheckSumAlgo As ICheckSumAlgo

    Function Encrypt(Message As String) As Byte()

    Function Decrypt(Cypher As Byte()) As String

    Function EncryptWihCheckSum(Message As String) As Byte()

    Function DecryptWithCheckSum(SumAndCypher As Byte()) As String

End Interface

The above is not just an encryptor and decryptor. It is a helper class and a container for an algorithm. This is a very different animal. Even though it provides all the abilities of your encryptor, its role is no longer to hide the implementation from the caller but to assist with calling yet another interface.

The latter locks you into more design decisions. For example, it precludes a null encryption scheme (which would have no algorithm) and it precludes encryption schemes that may utilize more than one algorithm (e.g. RSA + AES, which you might use for longer messages that require public key cryptography).

If you just want an encryptor and not a helper, but you want to expose a hint about the algorithm for debugging or logging purposes, you might consider exposing the hint as a string instead.

Interface IEncrypter

    ReadOnly Property EncryptionMethod As String

    Function Encrypt(Message As String) As Byte()

    Function Decrypt(Cypher As Byte()) As String

    Function EncryptWihCheckSum(Message As String) As Byte()

    Function DecryptWithCheckSum(SumAndCypher As Byte()) As String

End Interface

That way you aren't locked into using exactly one algorithm object, and you still have a way to meet your debugging and knowledge management needs.

1
  • Your suggestion to expose the "knowledge" fields via a string is a compromise I really like. However, as I commented somewhere else, nothing stops you from implementing RSA+AES together, or to use a null encryption: just use RSA+AES to implement together IEncryptionAlgo and you're good.
    – Ama
    Jan 31 '20 at 11:37
8

Yes remove it.

You prevent someone implementing an encryptor without implementing an Algo effectively tightly coupling your classes.

You don't need an interface to do unit testing or dependency injection. DI and unit testing require abstraction.

Addendum for full code

Neither the service factory or the consuming code use the extra read only properties, They only require an:

IEncryptor
    byte[] Encrypt(string)

Now some people would argue that you should make interfaces as small as possible, splitting up the decryption etc. I wouldn't go that far, but its a matter of taste.

However, it sounds like the only thing that you expect to call the read only fields is your unit tests to check that the servicefactory returns correctly built objects.

There are lots of ways you can do this without exposing the value as a property, and I would argue it doesn't really help with the tests at all, since I could implement an Encryptor which exposes a different algorithm to the one it actually uses.

This is better achieved by simply encrypting a string and comparing the result to a known values encrypted with the expected algorithm.

9
  • The main purpose of the class is to allow for choosing an algo (kind of a strategy pattern I guess?), not to implement an algo. Which is why in a sense exposing the choosen algo does not feel incredibly awful to me. (yet the consumer does not care which algo was chosen). How would you unit test the consumer of this interface, if it was not an interface?
    – Ama
    Jan 30 '20 at 17:03
  • if its readonly and a construction parameter how does the class allow you to choose it
    – Ewan
    Jan 30 '20 at 17:05
  • «The main purpose of the class is to allow for choosing an algo» — Looks like an XY problem to me. Why can't you just pass an algo class or instance? If you just unify an interface to encrypt bytes, just expose .Encrypt().
    – 9000
    Jan 30 '20 at 17:06
  • A factory creates the Encrypter object and passes the algo through the constructor. Then the instance of the Encrypter is passed to any consumer class. This is just an example; another one could be a SendToServerService object which takes a URL in the constructor, and exposes various methods such as SendSomething, SendSomethingElse, etc. Should ISendToServer expose a ReadOnly DestinationUrl.
    – Ama
    Jan 30 '20 at 17:15
  • 1
    @Ama Is there a good reason to have IEncrypter be an interface? If the answer is not really, then you could just turn the IEncrypter interface into a concrete class that takes an IEncryptionAlgo and an ICheckSumAlgo as dependencies, turning the whole thing into a Strategy pattern - this would also communicate your intent clearly. Jan 30 '20 at 21:27
5

I'm used to think that an interface only defines methods and constants. We can, of course, suppose that for "read-only fields" your interface are methods that return constant values.

The point of an interface if to define, well, an interface, but allow any reasonable implementation.

If a reasonable implementation would need full access to an encryption algorithm anyway, then exposing GetEncryptionAlgo() (or passing it as a parameter to an interface method) is ok.

If ThisEncryptionAlgo.Encrypt is strictly sufficient for the purposes of your interface, then exposing the rest is not a good idea, in my eys.

What you call the "spirit" seems to be your hunch of a possible implementation. This limits possible implementations. E.g. if you have a completely different encryption library then you are used to, it's easy t expose via the interface that only needs .Encrypt, and hard, or impossible, in your interface.

Ironically, it becomes harder to mock the encryption provider for testing of a class implementing that interface: you need to mock more, and have an easier time to mock it incorrectly.

In general, I'd suggest to expose only the strict minimum of methods in an interface. The simpler it is, the lower the chance of implementing it incorrectly.

If you want to expose more, consider an extended interface that augments your minimal interface with specific implementation conveniences:

interface Stream<T> {  // Minimal.
  T Read();
  Boolean HasMore();
}

interface SeekableStream<T> : Stream<T> {
  long GetPosition();
  void Seek(long);
}

class File<T> : SeekableStream<T> { // Still acceptable anywhere a Stream goes.
... 
}
1
  • By "spirit", I guess I meant architecture: I am in a case where exposing the indeed constant field is not adding constraints to the implementation because it is expected to be done that way. If an algorithm which does not implement IAlgo is required, then it should implement IAlgo, not IEncrypter.
    – Ama
    Jan 30 '20 at 17:35
3

I'd be very cautious about adding such "extra" methods to your interface.

An interface should be very clear about its responsibility.

Is IEncrypter truly responsible to tell you about an IEncryptionAlgo or ICheckSumAlgo? I think it should only Encrypt and Decrypt. By adding extra methods you are imposing more requirements/constraints on the implementations.

What about if you wanted a non-encrypting IEncrypter that just lets the bytes pass through, what would you return for ThisEncryptionAlgo? You might return null or a blank IEncryptionAlgo but that might cause an NPE in the caller or just push your problems farther down the line.

You shouldn't have exposed it in the first place, the details of the Encrypter should not be exposed. Once an interface shares something, users of that interface could make a connection to it and such connections require maintenance and are hard to break.

If you're sharing the IEncryptionAlgo and ICheckSumAlgo so that you can log about them then, rather than returning those implementation details, you could instead just make the GetEncrypterInfo As String a part of your interface and any implementer could fulfill that much more easily.

1
  • 1
    Your comment on logging is interesting. Regarding the non-encrypting IEncrypter, I would actually implement an IEncryptionAlgo which would implement the Enrypt/Decrypt methods by returning the bytes as they are.
    – Ama
    Jan 30 '20 at 22: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.