[cjsuk]

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.

[quickthrower2]

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?

[cjsuk]

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.

[quickthrower2]

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.

[cjsuk]

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.

[positivecomment]

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

[cjsuk]

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