DEV Community: jamietwells

Enumerable types and interfaces, which type do you prefer to return?

jamietwells — Tue, 19 Nov 2019 23:39:05 +0000

A discussion I've been involved with recently has been an updated guideline around the return types of interfaces. It was decided that IEnumerable was to not be used unless it was required that results be yielded rather than available immediately. There was to be a new rule that whenever you saw an IEnumerable you change it to ICollection to make it clear the results are immediately available.

I disagree with this change for many reasons.

First, on principle. It seems like a change to protect against an implementation where the developer doesn't realise an enumerable might be lazy and the generator might be in an invalid state when the consumer requests the results (perhaps it has an IDisposable dependency). This has never seemed like a convincing argument to me. We shouldn't limit design choices so that developers don't have to learn the language, they should understand the language they're writing in.

Also, it has the effect that developers tend to call .ToList() or .ToArray() at the end of every method simply to satisfy the ICollection. The caller might only be interested in .Any() or .Take(some) or doing further filtering. The creation and allocation of the intermediate collection is then wasted time.

ICollection has methods like Add and Remove, meaning what was previously a beautiful, simple, immutable return type is now a bloated mutable mess of an interface. Worse is if someone actually does start using the .Add and .Remove methods but the implementation behind the interface returns an array (which for some reason that I've never quite understood implements the ICollection interface) the calling code will start having runtime exceptions.

I wouldn't be so averse to using IReadOnlyCollection but unfortunately this idea was met with strong resistance as that type wasn't something other devs were familiar with and ICollection was already used in some places in the code, where as IReadOnlyCollection was not. The other issue brought up with IReadOnlyCollection was that it wasn't a sub-type of ICollection, it was on the same level.

I'd always assumed it was a subtype but apparently not, it was added much later and so would have been a breaking change to make ICollection implement IReadOnlyCollection. Regardless, it is sort of a subtype because all the members declared on IReadOnlyCollection are declared on ICollection and more so that argument again I didn't find convincing. I also don't really understand why it had to be a subtype of ICollection to be used instead, surely the added immutability would be reason enough.

I've always been of the opinion that these sorts of decisions should be made on a case by case basis. There isn't always a clear cut answer for the correct return type for a collection. I would usually stick to simpler interfaces (IEnumerable or IReadOnlyCollection) but I'd be hesitant to make a blanket statement around which to use in general. What do you think? How do you decide on return types? Perhaps you agree with my colleagues and can put their arguments to me so that I can be on the same page and feel more comfortable with the new guidelines.

Dynamic types in strongly typed languages - always wrong?

jamietwells — Fri, 18 Oct 2019 20:45:10 +0000

Recently I had a discussion with some colleagues about an endpoint I had created. I was asked by the product team to add the functionality to export some data in a specific format. We had endpoints that already did similar things and they all worked something like this:

Post some data to an endpoint
Write a row in a "file" table and remember the ID.
Raise a message with the data to a queue to start generating the document
Return the file ID to the front end so it can poll and API for the state of the file (and download when ready)

There were several of these endpoints already. The API didn't use the payload for anything, it simply put it on a queue but all these endpoints were the same code copied and pasted over and over. The only difference: the DTO classes for the payload they accepted.

Seeing this I decided I had to do something! I made a new endpoint that would become the standard export endpoint going forward.

I was using C# so forgive me if you're unfamiliar, I will try and make it easy enough to follow. Here is the endpoint I wrote:

[Route("/export/{type}/{documentName}/{format}")]
public Guid ExportDocument([FromBody]JObject payload,
    string type, string documentName, string format)
{
    var fileId = _fileRepo.NewFile(documentName, format);
    var message = new 
    {
        DocumentName = documentName,
        Format = format,
        Payload = payload,
        FileId: fileId
    };

    var queue = GetQueueFromType(type);
    queue.PublishMessage(ToJson(message));

    return fileId;
}

^{(Not the real code, but close enough to get the idea)}

In that example, the JObject is a C# class for representing some arbitrary JSON object, and that comes from the body of the HTTP request.

My thinking was: the next time someone needed to export a file in a different format they could use this endpoint and the only thing they would need to do is add their queue URL to mapping file (a simple JSON file). They could subscribe their new worker to the queue and messages would start making their way there.

I was fairly pleased with the design but it proved controversial when it came to making the pull request. The sticking point: the JObject.

Colleagues said things like:

This is C# not JavaScript, we should be have strongly typed objects. JObject is a code smell.

What if someone posts a payload that's missing data the worker needs to process the message? You should validate it on the API.

The API should serve as the documentation for what is a valid request. How will people know what to post if there isn't a DTO?

Why are you trying to avoid creating new endpoints? We're not charged per endpoint!

I argued and still maintain this is the correct design for several reasons:

No code duplication. You could argue that duplication could be minimal for the solution with multiple endpoints if they all called back into a generic method but I still feel like the multiple endpoints themselves are a form of duplication; it's just more obviously duplicated when the endpoints are various versions of the same method copied and pasted, as was the case here.
No DTOs required. Who wants yet more useless classes in an already large codebase? I'm not fond of DTOs in C# anyway - I dislike the fact that the seem to find themselves spreading out everywhere unless they are kept on a short leash and I particularly dislike that they have to have public get and set methods for every property.
Reduced coupling. If the worker that's creating the documents changes and requires a new format for the payload the API doesn't need to change. The frontend needs to pass the new format and the worker needs to be able to handle the new format but the API will just pass along the payload as before.

I also just don't think a strongly typed endpoint has much of an advantage. It's not going to catch a type mismatch at compile time, it's a web API, the JavaScript can send anything. It just causes a runtime error which is exactly the same as the runtime error the worker would experience if the payload it received was incorrect. Perhaps it acts as a sort of documentation, but I feel like you could achieve similar documentation just by writing a comment above the endpoint saying: Check the worker to find out what shape of payload is expected for each document type.

I managed to get the pull request in, but I feel like everyone else would still prefer if I got rid of it and went back to the old method. What do you think? Was it really a bad design? Are there disadvantages I've not mentioned? Would you have done something different? I feel like I just can't see the other side of the debate here so please help me understand in the comments.

I need to stop saying "Unit Tests"

jamietwells — Wed, 02 Oct 2019 18:50:48 +0000

In my head there are two types of automated tests worth (in 99% of cases) talking about and I've usually called them:

Unit Tests
Integration Tests

Recently I've realised that using those names comes with so much baggage that I need a better name for them. When I'm talking about unit tests I mean any test that starts at some public entry point of the code, goes all the way through and ends at some boundary with an external system. This is either a mocked dependency or is the response of the function/method called initially. This response or call to an external system then has some expected properties asserted to be true. The unit test says nothing about how many functions/methods are called, how many classes might be involved - nothing. The only restriction is that you don't go into some external system such as a database, a Web API, a filesystem etc.

Is this not what everyone else thinks a unit test is?

An integration test, then, is a test to ensure your system can connect and integrate with the external systems we mocked out in the unit tests. We're not testing the external systems, only that we integrate successfully with them. If they're under your control you test them using unit tests inside that system.

Maybe I have the wrong idea and what I describe aren't unit tests and integration tests. Maybe there's a different name I should be using? I ask because I recently discussed unit tests with several people and got responses similar to:

Oh, I don't bother with unit tests, they're a waste of time. I don't like testing one method/function/class at a time

Unit testing? No, I prefer to test in the way the user will interact with the system

Both of these objections seem completely crazy to me. Why on Earth would I write a test that does one method at a time? And why would I test something in a way the user wouldn't use it?

One person I was talking to was actually complaining about how, in C#, interfaces couldn't have static functions defined on them (put aside how that would actually work in practise; the only thing I can think is making the class generic for that interface type or something) and this was a problem due to a "rule" their company had that said every class should be mocked out for a unit test except the class under test! This meant that any "Helper" class or "Calculation" class had to be mocked out, which meant it had to adhere to an interface, and therefore had to have instance methods even if it was more suited to being a static class. It also meant that internal classes (and private classes) were not able to be used, because you wouldn't be able to test them and would therefore have holes in your testing.

This is I do not understand.

Why do companies make rules like this? The tests shouldn't drive the architecture. If the class you're testing does everything in one big method or calls 50 other internal classes it shouldn't change your tests! Test that class. Whatever it's up to, test it; and test it in the way a user would use your system. If a user shouldn't use some class because it is for code only inside your library then make it internal and don't test it directly. Please, don't make everything public just for the tests.

So that's my rant. If people think I mean tests testing every method in the code individually when I say unit test then I need a better name for it. I'm open to suggestions. Spec test? BDD test? Behaviour test? All these seem to mean different things to different people.

Is this the meaning you ascribe to "unit test"? Do you have a better name for what I describe? Does your company have an rules similar to the the rule I described above? Do you agree with the rule above? Let me know - I really want to know if I'm just missing something obvious or if we do need a better name for these things free of the baggage.