Unnecessary (?) ReadOnly fields in Interfaces

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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.
... 
}

Answer 4

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.