Hacker News Reader

Comment by positivecomment | Hacker News Reader

> as much as you can into static helper functions and most of the rest into dumb private stateless functions

In our work we use C# and it is very hard, even next to impossible to make a static class pass a code review - given it's not for extension methods (which I hate... why not be explicit about the first parameter and stop acting as a part of the class </rant>). They just tell us to use IoC and move to the next point. I honestly don't know why. Our IoC library can treat a dependency as static or singleton, but those are also discouraged. Once I had a static class named GraphRequestHelpers* and the reviewer got really negative, FSM knows why. She told me that we need IoC to make everything testable and "Helper" in the name is a code-smell. Sounds cargo-culting to me but I have only 6 years of experience so who I am to know.

* Now we have RequestExtensions and everything is apparently perfect.

View on HN

There is some cargo culting there but it's mostly correct.

Helper is a code smell as it's a sign of "we don't know what the responsibility of this is or what to call it so we'll just chuck a load of shit in this file and call it a helper". The methods in should belong to something and live on that class, not in an external class.

RequestExtensions is more shit than the original solution. Extension methods are even worse! Shoot the reviewer.

> Shoot the reviewer

Duly noted! Although I'll try talking to her first, I'm sure there's more behind the decision :)

One of the methods that was inside takes a request, extracts the body and returns the parsed graph from the body. It's used by many controllers from many projects. I don't know where to put such a thing, hence the request extension.

Always ask for the reason before slating it :)

Usually that's a single responsibility class:

   interface IGraphParser {
       Graph Parse(Request request);
   }

Inject that into the caller via the container then you can mock the thing that calls it and just return a static Graph object, which you can't do with a simple extension method (which is why it sucks).

This is a matter of taste not fact. In functional languages the style is compositional with static functions everywhere. It works well. The keeping data and methods together thing is one approach. Sometimes it's great. Sometimes unnecessary.

For example would you argue against string formatting helpers? Or would they need to be written to an interface and added to myriad DI bucket lists?

It's not that simple and it's not a fact. I'm an advanced user of functional languages as well and have written an entire scheme implementation before. I only semi-agree. That's slightly disingenuous representation of functional languages which have more than a few pitfalls. They certainly aren't the silver bullet and they really do not scale to the same height and complexity of the problem domain as the OO languages do due to the nature of the abstraction you describe. Nothing is particularly explicit. I'd rather take the compromises of OO over the maintenance problems of a functional language.

String formats are data so they would be stored as constants so that they are interned. They can be stored in a const class which is a static class with no methods i.e.:

   sealed class StringFormats {
       public const DateFormatX = @"...";
   }

Also string formats for example tend to be owned by the respective objects so you can add overloads to the object to provide certain arbitrary representations. If the translation between an object and the string representation is complex, then you're really serializing it so that should be an abstracted concern.

To be clear I'm not saying functional is always better. I'm saying there are other possibilities and dogma is bad.

The Haskell community has its dogmas too. And it's fair share of "let's do this simpler" blog posts.

As an aside Haskell has many equivalents of dependency injection and sugars to help and you can "inject all the things" there too.

My point is to think of the problem you are trying to solve, rather than ticking off the SOLID / Martin Fowler etc. tick boxes.

Understanding OO patterns, SOLID etc is a good thing but being prepared to "break the rules" is good IMO too.

Breaking the rules is good when appropriate. Problem is those rules are pretty amazingly good. I went through a weird phase of denial and ended up back where I started before I applied the aforementioned rules.

Every exception I have shot myself.

Extension methods are useful for only one reason: they trigger code completion for browsing what this object can do. Static methods suffer from FP code completion problems (you can’t complete easily in the first arg of a function/procedure).

I think I am not mistaken in saying extension methods, like lambda functions, were invented primarily for the use case of Linq. Even if they weren't, that's how Linq is implemented, so extension methods serve more than that "one purpose" if you don't insist on writing C# in the style of C# 2.0.

They came out at the same time, I’m sure there was some influence between them (Mads Tergesen would know better). However, all the functionality added in could have been done with static methods, just with more verbose syntax. LINQ query syntax could have been special cases. Anyways, I like what they came up with, it’s very versatile.

It's not clear that would have been much less work

Why hate extension methods? Do you really want to write Enumerable.ToList(Enumerable.Select(Enumerable.Where(someList, e => e.someBool), e => new {a = e.x, b = e.y)) and so on?

That would suck, on the other hand, the extension methods make people create huge chains continuations of which comes from who knows where.

Best solution would have been a pipe operator if you ask me.

Could someone expand IoC for me please?

https://martinfowler.com/bliki/InversionOfControl.html

Do you practise TDD? If you did a lot of this would make more sense to you. TDD is actually quite fun when you get the hang of it (less mental burden as you push all the 'intent' onto the computer).

TDD is completely orthogonal as to whether you write functional code that doesn't have a default receiver for routines, or OO code.

I don't see why TDD requires ruling out static methods and insisting on hiding everything behind an interface. Static methods are straightforward to test, certainly more than a class with multiple dependencies which need to be mocked. Usually the complaint is about coupling when calling static methods but these can be wrapped in a delegate if required.

Simply because you can't mock the static dependency, therefore that method is now dependent on the static class and you don't have any control over it. This is problematic - what if at some point later another developer adds a database call into the static method to do some logging? Now your testing will dirty whatever database you're using, as well as run 10x slower - and yet the test will still pass and everyone will be none the wiser as to what happened.

If you start using a custom delegate solution, then your code is not consistent with everything else that uses DI, making it harder to understand. I can understand interfaces are annoying when navigating code, but the IDE still helps with that even if it is a few more button clicks, and the pros outweigh the cons.

> that method is now dependent on the static class and you don't have any control over it.

I don't see how you have any less control over it than any other code you wrote. If you don't want it to write log statements, then don't do that. Most static methods are small and pure so don't need to write log statements anyway.

> Now your testing will dirty whatever database you're using, as well as run 10x slower.

I've never used a logging framework that didn't allow you to configure where log statements were written, or give you control over the logging threshold for individual classes. However if your method is writing logs then presumably there is a reason, which is just as useful in the tests. If you mock it out then you're testing against different code to the one you will actually run against.

> If you start using a custom delegate solution, then your code is not consistent with everything else that uses DI.

Passing functions as arguments directly is 'DI', just without the need to configure that through an external container. Reducing the amount of interfaces (often with a single implementation) and external configuration makes navigating the code easier.

I think you missed my point, it's not about the logging framework, its about the fact you don't control an external dependency during testing. Unit tests are meant to be reproducible, meaning they are done under controlled conditions.

> Most static methods are small and pure

This is very assuming, tests are a way of being specific about your intent.

> its about the fact you don't control an external dependency during testing

If your code is structured using small static functions, you don't have any dependencies in the first place, just arguments you are passed and transform. You will probably create interfaces for external services you depend on, but you can avoid needing to mock them if you express the transform directly.

> This is very assuming

I'm not assuming anything, since I wrote the static method and I also decided to call it, presumably for the result it calculates. Your argument appears to be that static methods could contain bad code but that applies to all code you depend on.

How is static code different from other noninjected code, like stuff in a method. Taken to the logical conclusion we'll have thousands or classes full of max 2 operations per method.

How many static classes are your methods using? And what is the problem with injecting this stuff at the top of the class instead? If you plan to write tests, you have to control your dependencies, and DI is the simpliest way to do that.

Because these tests become too detached from reality if you inject everything. A silly example to make the point:

IAdder { double Add(double a, double b) }

Test:

car mockAdder = ... // Mock 1+1=2 etc.

um... yes... this is actually what "pure" OO involves. The only reason we don't do this is because it's a nightmare to manage.

> Now your testing will dirty whatever database you're using, as well as run 10x slower

It sounds like a problem is a few layers higher. Why is there a live database in your unit testing environment? Why are working credentials configured? If they're unit test, not integration tests, all db operations should be DId / mocked / whatever. Any call that isn't should fail, not take longer time. Db interaction is for the integration tests.

That's exactly my point, mock your external dependencies. Static calls don't allow you to do that.

So how exactly do you test that your SQL query does the right thing? That you're using the Twitter API correctly?

Testing a database, or an external web service, is an integration test. They can be as simple as:

    void TestCreateUser() {
        var repo = new UsersRepository();
        var mockUser = new User("John", "Smith");
        repo.AddUser(mockUser); // db call
        var addedUser = repo.GetUsers().Single(); // db call
        Assert.StructureIsEqual(mockUser, addedUser);
    }

For the Twitter web service, you might test that you successfully get a response, as you don't have control of what exactly comes back.

In your language of choice:

    static_function(db, other, arguments) { ... }
    test { static_function(fake_db, 1, 2) }

You can even omit the db in the standard case if your language allows default keyword arguments. In almost every language, a method is just a fancy static call that takes extra arguments implicitly. (Closures are poor man's objects, objects are poor man's closures...)

In Java, you can use PowerMock to mock or spy anything, even private static final things. I consider it a smell (though excessive mocking even without powermock is its own smell), but it's immensely valuable to get code you can't change (or fear changing because of its complexity and lack of tests, or simply don't have time to change because the refactoring would take a whole sprint) to have some tests.

You don't need interfaces for everything in order to do DI. Interfaces should be used only for having multiple implementations or to break dependency loops.

Other than that I'm in agreement, static methods generally aren't a good idea. They can all too easily balloon into big chunks of imperative code with inner dependencies (static or not) at 10 indentation levels deep. Non static methods can too, but not as easily, and you have more options for fixes/workarounds in those cases anyway. The only place they really make sense is as part of a set of pure primitive data transforms, and ought to be small.

In C# we have Moq that can mock normal classes, though it requires adding 'virtual' to every method you want to override which is a code smell too. In Java everything being virtual by default I guess it doesn't matter. We like to always keep an interface around as it gets the developer used to working that way and keeps the code consistent. Visual Studio provides a quick shortcut to auto gen the interface too.

“What if ...” doesn’t pass YAGNI.

You mean that the tests will depend on the thing being tested? What a crime!

> what if at some point later another developer adds a database call into the static method to do some logging?

Then you have a developer that does not grasp the idea of functions, and how they can help you improve your code. That's a call for education, not for changing your tests.

The point is that tests are omnipresent, people aren't. I've worked at places where all sorts of dumb code has got through because there is no automation in place to stop it, and everyone else is too busy to do code reviews.

Problem is that the moment you start introducing delegates and crap like that is you're inventing a mechanism to work around your resistance to not using static methods rather than actually solving any problems.

There is no functional difference between a class with static methods and a class without, of which one instance is available to other classes.

Other than the fact that it isolates state, allows mocking and substitution and testing.

I disagree that delegates and higher-order function are 'crap' or in any way more complicated than introducing interfaces that are injected though a centralised container. You could just as easily turn that argument around and say mocking and an overuse of interfaces come from your resistance to using small static methods. In C# Linq is almost entirely based on static methods and delegates and that is not harder to test as a result.

Static methods usually don't rely on any hidden state at all. The example originally given was for a graph operation which could just take the input graph as an argument and return the result. When your code is composed of small independent functions you don't need mocking and substitution at all. In my experience most uses of mocks come from functions that do too much in the first place.

Yeah there is some cargo cult aversion towards statics.

Static methods with no side effect are wonderful, but static state is really bad and static methods which perform IO are horrible because they cannot be mocked in a unittest.

But some people miss this distinction and just say static methods are bad for testing.

C# is the new Java... facepalm

facepalm of enlightenment?

Rifle is the new pistol? You can shoot yourself with both?

Reply on news.ycombinator.com