10

For example, a class usually have class members and methods, eg:

public class Cat{
    private String name;
    private int weight;
    private Image image;

    public void printInfo(){
        System.out.println("Name:"+this.name+",weight:"+this.weight);
    }

    public void draw(){
        //some draw code which uses this.image
    }
}

But after reading about Single responsibility principle and Open closed principle, I prefer separate a class into DTO and helper class with static methods only, eg:

public class CatData{
    public String name;
    public int weight;
    public Image image;
}

public class CatMethods{
    public static void printInfo(Cat cat){
        System.out.println("Name:"+cat.name+",weight:"+cat.weight);
    }

    public static void draw(Cat cat){
        //some draw code which uses cat.image
    }
}

I think it fits Single responsibility principle because now the responsibility of CatData is to keep data only, doesn't care about the methods (also for CatMethods). And it also fits the open closed principle because adding new methods doesn't need to change the CatData class.

My question is, is it a good or an anti-pattern?

  • 15
    Doing anything blindly everywhere because you learned a new concept is always an anti-pattern. – Kayaman Jun 29 '18 at 5:40
  • 4
    What would be the point of classes if it's always better to have data separate from methods that modify them? That sounds like non-object-oriented programming (which certainly has its place). Since this is tagged "object-oriented" I assume you wish to use the tools that OOP supplies? – user1118321 Jun 29 '18 at 5:51
  • 4
    Another question proving that SRP is so badly misunderstood that it should be banned. Keeping data in a bunch of public fields is not a responsibility. – user949300 Jun 29 '18 at 9:47
  • 1
    @user949300, if the class is responsible for those fields, then moving them elsewhere on the app will of course have zero affect. Or to put it another way, you are mistaken: they 100% are a responsibility. – David Arno Jun 29 '18 at 11:29
  • 2
    @DavidArno and R Schmitz: Containing fields does not count as a responsibility for the purposes of SRP. If it did, there could be no OOP, as everything would be a DTO, and then separate classes would contain procedures to work on the data. A single responsibility pertains to a single stakeholder. (Though this seems to change slightly over each iteration of the principal) – user949300 Jun 29 '18 at 12:56
10

You have shown two extremes ("everything private and all (maybe unrelated) methods in one object" vs. "everything public and no method inside the object"). IMHO good OO modeling is none of them, the sweet spot is somewhere in the middle.

One litmus test of what methods or logic belongs into a class, and what belongs outside, is to look at the dependencies the methods will introduce. Methods which don't introduce additional dependencies are fine, as long as they fit well to the abstraction of the given object. Methods which do require additional, external dependencies (like a drawing library or an I/O library) are seldom a good fit. Even if you would make the dependencies vanish by using dependency injection, I still would think twice if placing such methods inside the domain class is really necessary.

So neither you should make every member public, nor do you need to implement methods for every operation on an object inside the class. Here is an alternative suggestion:

public class Cat{
    private String name;
    private int weight;
    private Image image;

    public String getInfo(){
        return "Name:"+this.name+",weight:"+this.weight;
    }
    public Image getImage(){
        return image;
    }
}

Now the Cat object provides enough logic to let surrounding code easily implement printInfo and draw, without exposing all attributes in public. The right place for these two methods is most probably not a god class CatMethods (since printInfo and draw are most probably different concerns, so I think it is very unlikely they belong into the same class).

I can imagine a CatDrawingController which implements draw (and maybe uses dependency injection for getting a Canvas object). I can also imagine another class which implements some console output and uses getInfo (so printInfo may become obsolete in this context). But for making sensible decisions on this, one need to know the context and how the Cat class will actually be used.

That is actually the way how I interpreted Fowler's Anemic Domain Model critics - for generally reusable logic (without external dependencies) the domain classes themselves are a good place, so they should be used for that. But that does not mean to implement any logic there, quite the opposite.

Note also the example above leaves still room for making a decision about (im)mutability. If the Cat class will not expose any setters, and Image is immutable itself, this design will allow to make Cat immutable (which the DTO approach won't). But if you think immutability is not required or not helpful for your case, you can also go into that direction.

  • Trigger happy downvoters, this gets boring. If you have some critics, please let me know. I will be happy to clarify any misunderstanding, if you have some. – Doc Brown Jun 29 '18 at 20:32
  • While I generally agree with your answer...I feel getInfo() might be misleading for someone asking this question. It subtly mixes presentation responsibilities as printInfo() does, defeating the purpose of DrawingController class – Adriano Repetti Jun 30 '18 at 10:03
  • @AdrianoRepetti I don't see that it's defeating the purpose of DrawingController. I do agree that getInfo() and printInfo() are horrible names. Consider toString() and printTo(Stream stream) – candied_orange Jun 30 '18 at 10:18
  • @candid it's not (just) about the name but about what it does. That code is only about a presentation detail and we effectively just abstracted away the output not the processed content and its logic. – Adriano Repetti Jun 30 '18 at 10:34
  • @AdrianoRepetti: honestly, this is just a contrived example, to fit to the original question and demonstrate my point. In a real system, there will be surrounding context which will allow to make decisions about better methods and names, of course. – Doc Brown Jun 30 '18 at 11:31
6

Late answer but I can't resist.

Is X most classes into Y good or an anti-pattern?

In most cases, most rules, applied without thinking, will mostly go horribly wrong (including this one).

Let me tell you a story about the birth of an object amid the chaos of some down right, quick and dirty, procedural code that happened, not by design, but out of desperation.

My intern and I are pair programming to quickly create some throw away code to scrape a webpage. We have absolutely no reason to expect this code will live long, so we're just banging out something that works. We grab the whole page as a string and chop out the stuff we need in the most amazingly brittle way you could imagine. Don't judge. It works.

Now while doing this I created some static methods to do the chopping. My intern created a DTO class that was very much like your CatData.

When I first looked at the DTO it bugged me. The years of damage Java has done to my brain made me recoil at the public fields. But we were working in C#. C# has no need of premature getters and setters to preserve your right to make the data immutable, or encapsulated later. Without changing the interface you can add them whenever you like. Maybe just so you can set a breakpoint. All without telling your clients a thing about it. Yea C#. Boo Java.

So I held my tongue. I watched as he used my static methods to initialize this thing before using it. We had about 14 of them. It was, ugly, but we had no reason to care.

Then we needed it in other places. We found ourselves wanting to copy and paste the code. 14 lines of initialization being flung around. It was starting to get painful. He hesitated and asked me for ideas.

Reluctantly I asked, "would you consider an object?"

He looked back at his DTO and screwed up his face in confusion. "It is an object".

"I mean a real object"

"Huh?"

"Let me show you something. You decide if it's useful"

I chose a new name and quickly whipped up something that looked a little like this:

public class Cat{
    CatData(string catPage) {
        this.catPage = catPage
    }
    private readonly string catPage;

    public string name() { return chop("name prefix", "name suffix"); }
    public string weight() { return chop("weight prefix", "weight suffix"); }
    public string image() { return chop("image prefix", "image suffix"); }

    private string chop(string prefix, string suffix) {
        int start = catPage.indexOf(prefix) + prefix.Length;
        int end = catPage.indexOf(suffix);
        int length = end - start;
        return catPage.Substring(start, length);
    }
}

This did nothing the static methods weren't already doing. But now I'd sucked the 14 static methods into a class where they could be alone with the data they worked on.

I didn't force my intern to use it. I just offered it and let him decide if he wanted to stick with the static methods. I went home thinking he'd probably stick to what he already had working. The next day I found he was using it in a bunch of places. It decluttered the rest of the code which was still ugly and procedural but this bit of complexity was now hidden from us behind an object. It was a little better.

Now sure every time you access this it's doing a fair bit of work. A DTO is a nice fast cached value. I worried about that but realized I could add the caching if we ever needed without touching any of the using code. So I'm not going to bother until we care.

Am I saying you should always stick to OO objects over DTO's? No. DTO's shine when you need to cross a boundary that keeps you from moving methods. DTO's have their place.

But so do OO objects. Learn how to use both tools. Learn what each costs. Learn to let the problem, the situation, and the intern decide. Dogma is not your friend here.


Since my answer is already ridiculously long let me disabuse you of some misconceptions with a review of your code.

For example, a class usually have class members and methods, eg:

public class Cat{
    private String name;
    private int weight;
    private Image image;

    public void printInfo(){
        System.out.println("Name:"+this.name+",weight:"+this.weight);
    }

    public void draw(){
        //some draw code which uses this.image
    }
}

Where's your constructor? This isn't showing me enough to know if it's useful.

But after reading about Single responsibility principle and Open closed principle, I prefer separate a class into DTO and helper class with static methods only, eg:

public class CatData{
    public String name;
    public int weight;
    public Image image;
}

public class CatMethods{
    public static void printInfo(Cat cat){
        System.out.println("Name:"+cat.name+",weight:"+cat.weight);
    }

    public static void draw(Cat cat){
        //some draw code which uses cat.image
    }
}

I think it fits Single responsibility principle because now the responsibility of CatData is to keep data only, doesn't care about the methods (also for CatMethods).

You can do many silly things in the name of the Single Responsibility Principle. I could argue that Cat Strings and Cat ints should be separated. That drawing methods and Images must all have their own class. That your running program is a single responsibility so you should only have one class. :P

To me, the best way to follow the Single Responsibility Principle is to find a good abstraction that lets you put complexity in a box so you can hide it. If you can give it a good name that keeps people from being surprised by what they find when they look inside you've followed it fairly well. Expecting it to dictate more decisions then that is asking for trouble. Honestly, both of your code listings do that so I don't see why SRP matters here.

And it also fits the open closed principle because adding new methods doesn't need to change the CatData class.

Well no. The open close principle isn't about adding new methods. It's about being able to change the implementation of old methods and having to edit nothing. Nothing that uses you and not your old methods. Instead you write some new code somewhere else. Some form of polymorphism will do that nicely. Don't see that here.

My question is, is it a good or an anti-pattern?

Well hell how should I know? Look, doing it either way has benefits and costs. When you separate code from data you can change either without having to recompile the other. Maybe that is critically important to you. Maybe it just makes your code pointlessly complicated.

If it makes you feel better you aren't that far from something Martin Fowler calls a parameter object. You don't have to only take primitives into your object.

What I would like you to do is develop a sense for how to do your separation, or not, in either coding style. Because believe it or not you're not being forced to choose a style. You just have to live with your choice.

2

You have stumbled across a topic of debate that has been creating argument amongst developers for well over a decade. In 2003, Martin Fowler coined the phrase "Anaemic Domain Model" (ADM) to describe this separation of data and functionality. He - and others that agree with him - argue that "Rich Domain Models" (mixing data and functionality) is "proper OO", and that the ADM approach is a non-OO anti-pattern.

There have always been those that dismiss this argument and that side of the argument has grown louder and bolder in recent years with the adoption by more developers of functional development techniques. That approach actively encourages the separation of data and function concerns. The data should be immutable as much as possible, so the encapsulation of mutable state becomes a non-concern. There is no benefit to attaching functions directly to that data in such situations. Whether is then "not OO" or not is of absolutely no interest to such folk.

Regardless of which side of the fence you sit on (I sit very firmly on the "Martin Fowler is talking a load of old tosh" side BTW), your use of static methods for printInfo and draw is near-universally frowned upon. Static methods are hard to mock when writing unit tests. So if they have side effects (such as printing or drawing to some screen or other device), they shouldn't be static, or should have the output location passed in as a parameter.

So you could have an interface:

public interface CatMethods {
    void printInfo(Cat cat);
    void draw(Cat cat);
}

And an implementation that gets injected into the rest of your system at runtime (with other implementations being used for testing):

internal class CatMethodsForScreen implements CatMethods {
    public void printInfo(Cat cat) {
        System.out.println("Name:"+cat.name+",weight:"+cat.weight);
    }

    public void draw(Cat cat) {
        //some draw code which uses cat.image
    }
}

Or add extra parameters to remove the side-effects from those methods:

public static class CatMethods {
    public static void printInfo(Cat cat, OutputHandler output) {
        output.println("Name:"+cat.name+",weight:"+cat.weight);
    }

    public static void draw(Cat cat, Canvas canvas) {
        //some draw code which uses cat.image and draws it on canvas
    }
}
  • But why would you try to shoehorn an OO language like Java into a functional language, instead of going with a functional language from the start? Is it some kind of "I hate Java, but everyone uses it so I have to, but at least I'm going to write it in my own way". Unless you're claiming that the OO paradigm is obsolete and outdated in some way. I subscribe to the "If you want X, you know where to get it" school of thought. – Kayaman Jun 29 '18 at 7:02
  • @Kayaman, "I subscribe to the "If you want X, you know where to get it" school of thought". I don't. I find that attitude as both short-sighted and slightly offensive. I don't use Java, but I use C# and I ignore some of the "real OO" features. I don't care for inheritance, stateful objects and the like and I write lots of static methods and sealed (final) classes. The tooling and community size around C# is second to none. For F#, it's way way poorer. So I subscribe to the "If you want X, ask for it to be added to your current tooling" school of thought. – David Arno Jun 29 '18 at 7:07
  • 1
    I'd say ignoring aspects of a language is a lot different from trying to mix and match features from other languages. I don't try to write "real OO" either, as trying to just follow rules (or principles) in an inexact science like programming doesn't work. Of course you need to know the rules so you can break them. Tacking on features blindly can be bad for the development of the language. No offense, but IMHO a part of the FP scene seems to feel "FP is the greatest thing since sliced bread, why won't people understand it". I'd never say (or think) OO or Java is "the best". – Kayaman Jun 29 '18 at 8:13
  • 1
    @Kayaman, but what is "trying to mix and match features from other languages" supposed to mean? Are you arguing that functional features shouldn't be added to java, because "Java is an OO language"? If so, that is a short-sighted argument in my view. Let the language evolve with the wants of developers. Forcing them to switch to another language instead is the wrong approach, I feel. Sure some "FP advocates" go over the top about FP being the best thing ever. And some "OO advocates" go over the top about OO having "won the battle because it's clearly superior". Ignore them both. – David Arno Jun 29 '18 at 8:56
  • 1
    I'm not being anti-FP. I use it in Java where it's useful. But if a programmer says "I want this" it's like a client saying "I want this". Programmers aren't language designers and clients aren't programmers. I upvoted your answer since you say that OP's "solution" is frowned upon. The comment in the question is more succinct though, i.e. he's reinvented procedural programming in an OO language. The general idea does have a point though, as you showed. A non-anemic domain model doesn't mean data and behavior are exclusive, and outside renderers or presenters would be forbidden. – Kayaman Jun 29 '18 at 9:17
0

DTOs -- Data Transport Objects

are useful for just that. If you are going to shunt data between programs or systems than DTOs are desirable as they provide a manageable sub-set of an object which is concerned only with data structure and formatting. The advantage being that you do not have to synchronise updates to complex methods over several systems (as long as the underlying data does not change).

The whole point of OO is to bind data and the code acting on that data closely together. Separating a logical Object into separate classes is usually a bad idea.

-1

Many design patterns and principles conflict, and ultimately only your judgement as a good programmer will help you decide which patterns should apply for a specific problem.

In my opinion, the Do The Simplest Thing That Could Possibly Work principle should win by default unless you have a strong reason to make the design more complicated. So in your specific example, I would opt for the first design, which is a more traditional object-oriented approach with data and functions together in the same class.

Here are a couple questions you could ask yourself about the application:

  • Will I ever need to provide an alternate way of rendering a Cat image? If so, will inheritance not be a convenient way to do that?
  • Does my Cat class need to inherit from another class or satisfy an interface?

Answering yes to any of these might lead you to a more complex design with separate DTO and functional classes.

-1

Is it a good pattern?

You're probably going to get different answers here because people follow different schools of thought. So before I even begin: This answer is according to object-oriented programming as it is described by Robert C. Martin in the book "Clean Code".


"True" OOP

As far as I'm aware, there's 2 interpretations of OOP. For most people, it boils down to "an object is a class".

However, I found the other school of thought more helpful: "An object is something that does something". If you're working with classes, each class would be one of two things: a "data-holder" or an object. Data-holders hold data (duh), objects do stuff. That does not mean that a data-holder can't have methods! Think of a list: A list doesn't really do anything, but it has methods, to enforce the way it holds the data.

Data holders

You Cat is a prime example of a data holder. Sure, outside of your program a cat is something that does something, but inside your program, a Cat is a String name, an int weight and an Image image.

That data holders don't do anything also doesn't mean that they don't contain business logic! If your Cat class has a constructor requiring name, weight and image, you've successfully encapsulated the business rule that every cat has those. If you can change the weight after a Cat class has been created, that's another business rule encapsulated. These are very basic things, but that just means that's it's very important to not get them wrong - and this way, you ensure that it's impossible to get that wrong.

Objects

Objects do something. If you want Clean* objects, we can change that to "Objects do one thing".

Printing the cat info is the job of an object. For example, you could call this object "CatInfoLogger", with a public method Log(Cat). That is all we need to know from the outside: This object logs a cat's info and to do so, it needs a Cat.

On the inside, this object would have references to whatever it needs to fulfill its single responsibility of logging cats. In this case, that's only a reference to System for System.out.println, but in most cases, it will be private references to other objects. If the logic for formatting the output before printing it becomes too complex, CatInfoLogger can just get a reference to a new object CatInfoFormatter. If later on, every log needs to also be written to a file, you can add a CatToFileLogger object which does that.

Data holders vs objects - summary

Now, why would you do all that? Because now changing a class changes only one thing (the way a cat is logged in the example). If you're changing an object, you're only changing the way a certain action is done; if you change a data holder, you only change what data is held and/or in which way it is held.

Changing data might mean the way something is done has to change, too, but that change won't happen until you make it happen yourself by changing the responsible object.

Overkill?

This solution might seem a little overkill. Let me be frank: It is. If your whole program is only as big as the example, don't do any of the above - just choose either of your proposed ways with a coin toss. There is no danger of confusing other code if there is no other code.

However, any "serious" (= more than a script or 2) software is probably big enough that it would profit from a structure like this.


Answer

Is separating most classes into DTO and helper classes [...] a good or an anti-pattern?

Turning "most classes" (without any further qualification) into DTOs/data holders, is not an anti pattern, because it's actually not any pattern - it's more or less random.

Keeping data and objects apart however is seen as a good way to keep your code Clean*. Sometimes you will only need a flat, "anemic" DTO and that's it. Other times, you need methods to enforce the way the data is held. Just don't put your program's functionality with the data.


*That's "Clean" with a capital C like the book title, because - remember the first paragraph - this is about one school of thought, while there are also others.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.