3

I'm trying out a new code structure where I'm splitting all my giant repositories and factories into loads of smaller classes each with a single responsibility. And on top of this I'm using verbs for class names as I think that it's the best way to describe exactly what each class is meant for.

Each class has only one public method (called "Execute"), but often has private methods and sometimes a constructor with arguments.

Examples:

Before:

class DocumentRepository {
  public List<Document> GetDocuments()
  public void SaveDocument(Document document)
  public Document CopyDocument(int id)
}

After:

class GetDocuments {
  public List<Document> Execute()
}
class SaveDocument {
  public void Execute(Document document)
}
class CopyDocument {
  public Document Execute(int id)
}

One benefit of this structure is that I'm much more willing to split the functionality of the public methods into multiple private methods (much easier to read and manage). Before this would increase the clutter in the repository but now the private methods are contained within their separate classes.

I have always read that classes should have nouns as names but when a class only has one use it seems better to name it by what it does.

Question:

Is this a bad idea (both the separation and the naming)? And if so, what is a better way of creating this separation and avoiding large classes?

Edit: It should be noted that I have come to these (weird) ideas from a Command-Query perspective. So maybe it makes most sense to compare it to that instead of repositories.

svejk
  • 63
  • 4
  • This may be better placed at http://codereview.stackexchange.com. That said, I would say classes are nouns, methods are verbs, but it is all opinion. – Andrew Tavera Mar 04 '17 at 15:44
  • I'm actually toying with the same naming idea in a personal project and googled to see what other people think about it. In my case, it isn't a repository, but what's similar -- a class with a single public method `Invoke`, a number of private methods, and some dependencies. The answers say this naming scheme is weird. But if it helps express a class' purpose the best, maybe don't get trapped by dogma and use it anyway? – Myk Apr 24 '18 at 07:35

3 Answers3

1

IMO, your design is quite weird.

Sure, I understand your aim - to reduce noise in the class by moving private methods to elsewhere, but moving every method into a separate class seems too excessive.

Before, you just need one object to get, save, and copy documents. Now you need 3 in parallel! I guess you can create a wrapper that wraps these three classes,

public class DocumentRepository {
    public readonly GetDocument Get = new GetDocument();
    public readonly SaveDocument Save = new SaveDocument();
    public readonly CopyDocument Copy = new CopyDocument();
}

but still, too convoluted.

If you want less noise, try to organize your class in a better way and make use of #region and #endregion if your IDE supports them. If I were you, I would do this:

public SomeClass {
    // private fields
    ...
    // properties
    ...
    // public methods
    ...
    // private methods
    ...
}

Another approach would be:

public SomeClass {
    // private fields
    ...
    // properties
    ...
    // public method 1
    ...
    #region
    // private methods related/called by public method 1
    ...
    #endregion
    // public method 2
    ...
    #region
    // private methods related/called by public method 2
    ...
    #endregion
    // public method 3
    ...
    #region
    // private methods related/called by public method 3
    ...
    #endregion

    #region
    // other private methods that are not related to a particular public method.
    #endregion
}
Sweeper
  • 213,210
  • 22
  • 193
  • 313
  • The thing is, these methods (or classes in my case), really have nothing to do with each other except handling the same type of object. I have no need to wrap them together, except putting them in the same folder since they operate in the same part of the domain. Each of them are called from a separate Controller Action. – svejk Mar 04 '17 at 17:48
  • This sounds to me like you should create a utility class. If `DocumentRepository` holds no state (no properties or fields), you should just create a static utility class full of these methods, organized in the way I described in the answer. @svejk – Sweeper Mar 04 '17 at 17:52
  • Static would not be a good fit either since they do utilize inheritance (SaveDocument-class is a subclass of SaveEntity-class). And I find it much easier to manage unit testing with instantiatable classes (creating mock-objects of these when necessary). But you are right that they hold no state at all, they are just an executor of a specific task in the application. – svejk Mar 04 '17 at 17:58
1

Though extracting every single method into separate class seems too radical, there is an element of sense in it. For highly loaded systems the Command-Query responsibility segregation (CQRS) principle is being applied. The main idea of it is to strictly separate Queries (idempotent methods, not modifying system state, but only returning data) from Commands (methods that modify system state; in "pure" CQRS they don't even return data - though it is still hard for me to accept this).

Doing so would make your system more stable and easily expandable, as you can have many distributed nodes that do nothing but reading, focusing on performance of a single node that modifies data. However not all systems really need this.

Even if you really need a separate class for single methods, naming it as verb is not a good idea, as it will confuse other developers (staying inside one class it would be hard to determine what you see in code - the other class name or this class' internal method). You don't need to invent your own notation here, just use the following (relatively standard):

class GetDocumentsQuery { }

class SaveDocumentCommand { }

class CopyDocumentCommand { }

More information about CQRS:

  1. https://en.wikipedia.org/wiki/Command%E2%80%93query_separation

  2. http://rob.conery.io/2014/03/04/repositories-and-unitofwork-are-not-a-good-idea/

Aleksei
  • 562
  • 4
  • 11
  • I actually came to these ideas from reading about and exploring the Command-Query ideas. And I guess I'm trying to find my comfort place somewhere in the vicinity of those principles. But I see your point about other developers getting confused by the verb names, even if the only difference then would be to add "Query" or "Command" after the class name. – svejk Mar 04 '17 at 17:52
  • Maybe you mean something very different when you say that in "pure" CQRS commands don't return data. But please notice, in CQRS commands don't have to be one-way. [Here is Greg Young syaing it](https://youtu.be/LDW0QWie21s?t=1672) – Myk Apr 24 '18 at 08:42
0

Is this a bad idea (both the separation and the naming)?

I would say the naming is primarily a matter of taste. Your new design however is simply horrible.

You should group functionally related methods together in a logical way, usually based on the entity it performs actions on. I would say that either your Get, Save, etc. methods belong in a repository patterned class, or in the entity class itself. Never I would turn methods into classes, that just makes no sense at all.

I can understand you want to keep the number of rows per class down. Maybe it is better to use a base class to put common code in, or split of some repetitive actions in a separate class.

Patrick Hofman
  • 153,850
  • 22
  • 249
  • 325