0
\$\begingroup\$

My main questions: FileWithSave handles its saving itself, but delegates it to a repository object. This allows implementation details to be available without being present in the API when saving (transpose this to a Mail sending API as well, available for send but not public). Is this OK in terms of OOP practices? Are there any maintainence gotchas I'm missing? Does it need reorganization?

Without save:

package com.example.external;

public interface File {
    /**
     * Get the unique identifier.
     *
     * @return unique identifier.
     */
    String getId();

    /**
     * Get the file name.
     *
     * @return file name.
     */
    String getName();

    /**
     * Sets name for the file.
     *
     * @param name file name.
     */
    void setName(String name);

    /**
     * Get the file size.
     *
     * @return file size.
     */
    Long getSize();
}

Without save implementation:

package com.example.internal;

public class FileImpl implements File {
    // I don't want it in my public signature but I want it when I'm saving for some reason :(
    private Something somethingPrivate;
    private final String id;
    private final String name;
    private final Long size;

    public FileImpl(String id, Long size) {
        this.id = id;
        this.size = size;
    }

    public String getId() {
        return id;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public Long getSize() {
        return size;
    }
}

Repository for File:

package com.example.external;

public interface FilingCabinet {
    /**
     * Get a file from the cabinet.
     *
     * @param id unique identifier.
     * @return file.
     */
    File get(String id);

    /**
     * Store a file in the cabinet.
     *
     * @param file.
     */
    void store(File file);
}

Without save client sample code:

FilingCabinet cabinet = room.getFilingCabinet();
File file = cabinet.get("my-unique-id");
file.setName("my-new-name");
cabinet.store(file);

With save:

package com.example.external;

public interface FileWithSave extends File {
    /**
     * Save the changes.
     */
    File store();
}

With save implementation:

package com.example.internal;

public class FileWithSaveImpl implements FileWithSave {
    private Something somethingPrivate;
    private final String id;
    private final String name;
    private final Long size;
    private final PrivateFilingCabinet repo;

    public FileWithSaveImpl(String id, Long size, PrivateFilingCabinet repo) {
        this.id = id;
        this.size = size;
        this.repo = repo;
    }

    @Override
    public String getId() {
        return id;
    }

    @Override
    public String getName() {
        return name;
    }

    @Override
    public void setName(String name) {
        this.name = name;
    }

    @Override
    public Long getSize() {
        return size;
    }

    @Override
    public void store() {
        repo.save(this);
    }
}

Wrapper class:

package com.example.external;

/**
 * Wraps PrivateFilingCabinet and injects PrivateFilingCabinet in FileWithSave object.
 */
public interface Secretary {
    /**
     * Get the file for someone.
     *
     * @param unique identifier.
     * @return file.
     */
    FileWithSave getFile(String id);
}

Repository injected in to FileWithSave and not part of the public API package:

package com.example.internal;

public interface PrivateFilingCabinet {
    /**
     * Get a file from the cabinet.
     *
     * @param id unique identifier.
     * @return file.
     */
    FileWithSaveImpl get(String id);

    /**
     * Store a file in the cabinet.
     *
     * @param file.
     */
    void store(FileWithSaveImpl file);
}

With save client sample code:

FileWithSave file = company.getSecretary().getFile("my-unique-id");
file.setName("my-new-name");
file.store();
\$\endgroup\$
  • \$\begingroup\$ No object should be able to save itself. \$\endgroup\$ – CodesInChaos Feb 19 '15 at 21:18
4
\$\begingroup\$

This is a clear setup:

FilingCabinet cabinet = room.getFilingCabinet();
File file = cabinet.get("my-unique-id");
file.setName("my-new-name");
cabinet.store(file);

You can get files from the cabinet, modify the file, and put the file back into the cabinet for storage. The cabinet is in charge of storing the file.

This is an unclear, confusing setup:

FileWithSave file = company.getSecretary().getFile("my-unique-id");
file.setName("my-new-name");
file.store();

There is a company. The company has a secretary. You can get files from the secretary, modify the file, and the file can store itself.

The second setup makes me wonder about several things:

  • Who's in charge of storing the file?
  • After I do .store(), does the secretary still have the same vision of the file as me?
  • What if I want to store the file somewhere else? For example, at a different company, or in a cabinet.

In the first setup I don't need to ask such questions, as the answers are obvious.

Single responsibility principle

Another warning sign is that the second setup violates the single responsibility principle. The FileWithSave is not just a file anymore, it's also in charge of storage. You might say, that's kind of the point, hence the name file-with-save. Inheritance is not really about giving additional responsibilities to classes. The purpose of inheritance is more about specialization. The best sub-classes do the same things as their parents, just differently. Giving a file the additional responsibility to save itself seems too much. Even if this is not inherently a bad design, you can replace with a better one (the first one), and so you should.

Bad practices

Use interface types instead of implementations. This is especially crucial in interface definitions. So instead of this:

public interface PrivateFilingCabinet {
    FileWithSaveImpl get(String id);
    void store(FileWithSaveImpl file);
}

It should have been:

public interface PrivateFilingCabinet {
    FileWithSave get(String id);
    void store(FileWithSave file);
}
\$\endgroup\$
  • \$\begingroup\$ Very good point about the storing the file elsewhere. I would have to have a method for every "other" intended location. So basically the second option is not very maintainable and "future proof". Thank you this helped me. \$\endgroup\$ – cldfzn Feb 19 '15 at 20:46
3
\$\begingroup\$

Good question. I saw this many times and would like to share my thoughts. It seems to me that approach with save method(or any other business logic method) in domain object is have more cons than pros.

  1. Construction of the object is much heavier. Now you have to provide repo for each object you create. Even if repo won't be needed. Even if it's simple name check, for example. Or just test. You have to get repo somewhere.
  2. Maybe the end user of this object wants not just save, but send it somewhere via email, and via http as a json, or xml. Now you have to provide another three methods. Or you have to know at the beginning what the client want to do with this object and inject correct repo or even repos. Or you logic is split: we save it to the disk or db via save method, but we send it by some services.
  3. It's arguable, but I think it violates single responsibility. Your class becomes data container and some kind of service who has a business logic how to manage this data.
\$\endgroup\$
  • 1
    \$\begingroup\$ Yeah I had heard about the SRP violation before. The object construction being heavier I hadn't even thought about, and is a very good point. If the repo isn't immutable and there is a lot of file objects being created that's a lot of garbage collection for possibly no reason. \$\endgroup\$ – cldfzn Feb 19 '15 at 20:42

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.