7
\$\begingroup\$

I have many repositories stored in a map. User chooses some of them and then a downloading begins. Repository may be one of 4 types (see code below).

/** 
 * Data class that stores settings of a connection to repository.
 * Lombok's annotations are used to generate some methods.
 */
@Getter
@EqualsAndHashCode
public class Repository {
    public enum Type {
        FILE, FOLDER, GITHUB, BITBUCKET
    }

    private Type type;
    private String username;
    private char[] password;
    /*...*/
}

/**
 * A class that contains common stuff for all repository tools (such as connection to remote repository, download, logging, etc.)
 */
public abstract class AbstractRepositoryTools {

    /**
     * This method is implemented in child classes and actually it does the job.
     */
    protected abstract void doDownloadZip(Repository repo, String branch, Path saveTo);

    public static void downloadZip(Repository repo, String branch, Path saveTo) {
        AbstractRepositoryTools tools = Resolver.getToolsFor(repo);
        tools.doDownloadZip(repo, branch, saveTo);
    }

}

There are package-private classes that implement AbstractRepositoryTools for GitHub, BitBucket, and local repositories (folder or zip file).

/**
 * Resolver of a RepositoryTools implementation (with caching)
 */
class Resolver {

    // cache
    private static volatile BitbucketTools bitbucketTools = null;
    private static volatile GitTools gitTools = null;
    private static volatile LocalFileTools localFileTools = null;
    private static volatile LocalFolderTools localFolderTools = null;

    public static synchronized AbstractRepositoryTools getToolsFor(Repository repo) {
        switch (repo.getType()) {
            case GITHUB:
                if (gitTools == null)
                    gitTools = new GitTools();
                return gitTools;
            case BITBUCKET:
                if (bitbucketTools == null)
                    bitbucketTools = new BitbucketTools();
                return bitbucketTools;
            case FILE:
                if (localFileTools == null)
                    localFileTools = new LocalFileTools();
                return localFileTools;
            case FOLDER:
                if (localFolderTools == null)
                    localFolderTools = new LocalFolderTools();
                return localFolderTools;
            default:
                throw new NotImplementedException("Repository.TYPE = " + repo.getType());
        }
    }
}

Then I can take a repository from the map and download it like:

Repository repo = getRepo();
Path output = Paths.get("C:\\out")
AbstractRepositoryTools.downloadZip(repo, "develop", output);

It works but...

  1. Does this code smell?
  2. If yes, how to improve it?
  3. If no, is there a name for such architecture pattern?
\$\endgroup\$
1
  • \$\begingroup\$ Make Resolver and interface - have Repository implements it and do away with the other class entirely. An enum makes an excellent abstract factory implementation. \$\endgroup\$ Oct 2, 2015 at 14:54

2 Answers 2

11
\$\begingroup\$

A few points about your resolver.

Since you're using an enum to specify the repository's Type and resolve the tools depending on that type, an EnumMap seems to be the simplest solution to resolving the specific tool implementation necessary.

Also caching objects that have virtually no creation cost seems overkill to me. You can just eagerly initialize the objects and then don't even have to care about multithreading problems:

class Resolver {

    private static final Map<Repository.Type, AbstractRepositoryTools> cache =
        new EnumMap<>(Repository.Type.class);

    static {
        cache.put(GITHUB, new GitTools());
        cache.put(FILE, new LocalFileTools());
        cache.put(FOLDER, new LocalFolderTools());
        cache.put(BITBUCKET, new BitbucketTools());
    }

    public static AbstractRepositoryTools getToolsFor(Repository repo) {
        if (cache.containsKey(repo.getType()) {
           return cache.get(repo.getType());
        }
        throw new NotImplementedException("Repository.TYPE = " + repo.getType());
    }
}

Additionally I wouldn't call the class AbstractRepositoryTools, but just RepositoryTools.

You can simplify the resolver call by only giving it the type of the repo (since that's the only thing necessary). Furthermore I would extract the doDownloadZip into a separate interface and then seal the RepositoryTools class by declaring it final

Other than that your code is very clear and simple. I assume the javadoc comments are only explanatory notes for reviewers, and that you excluded the actual javadoc.

And since you asked about the name of the pattern... This is a Facade

\$\endgroup\$
4
  • \$\begingroup\$ NotImplementedException is not standard Java, did you mean UnsupportedOperationException? \$\endgroup\$ Oct 2, 2015 at 11:23
  • \$\begingroup\$ @SimonForsberg taken from question. I assume that OP has a custom implementation in their Project \$\endgroup\$
    – Vogel612
    Oct 2, 2015 at 11:24
  • \$\begingroup\$ It is an exception from Appache Commons that supplements UnsupportedOperationException: import org.apache.commons.lang3.NotImplementedException; \$\endgroup\$ Oct 2, 2015 at 11:24
  • \$\begingroup\$ Whoops, didn't notice that the question already used it. \$\endgroup\$ Oct 2, 2015 at 11:26
2
\$\begingroup\$

We don't have the complete code, so I'm not sure what I am writing applies to your complete problem.

It seems to me that you made things way too complicated.

  • I don't see the need for AbstractRepositoryTools at all. Its methods could be within Repository instead. You would have less classes and the related methods would be together.

  • The Resolver also does not seem useful. As pointed out by Vogel612, a map will suffice.

Minor Details:

  • Repository contains an enum, but it would make more sense to either make Repository an enum with member variables and methods, or make Repository an interface or abstract class with 4 implementations.

  • Instead of being an abstract class AbstractRepositoryTools should be an interface since it implements nothing at all. Since Java 8 interface can have static methods. (It used to drive me nuts that interface could not have static methods.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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