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...
- Does this code smell?
- If yes, how to improve it?
- If no, is there a name for such architecture pattern?
2 Answers 2
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
-
\$\begingroup\$ NotImplementedException is not standard Java, did you mean
UnsupportedOperationException
? \$\endgroup\$Simon Forsberg– Simon Forsberg2015年10月02日 11:23:15 +00:00Commented Oct 2, 2015 at 11:23 -
\$\begingroup\$ @SimonForsberg taken from question. I assume that OP has a custom implementation in their Project \$\endgroup\$Vogel612– Vogel6122015年10月02日 11:24:29 +00:00Commented Oct 2, 2015 at 11:24
-
\$\begingroup\$ It is an exception from Appache Commons that supplements
UnsupportedOperationException
:import org.apache.commons.lang3.NotImplementedException;
\$\endgroup\$naXa stands with Ukraine– naXa stands with Ukraine2015年10月02日 11:24:47 +00:00Commented Oct 2, 2015 at 11:24 -
\$\begingroup\$ Whoops, didn't notice that the question already used it. \$\endgroup\$Simon Forsberg– Simon Forsberg2015年10月02日 11:26:56 +00:00Commented Oct 2, 2015 at 11:26
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 withinRepository
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 makeRepository
an enum with member variables and methods, or makeRepository
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.)
Explore related questions
See similar questions with these tags.
Resolver
andinterface
- haveRepository implements
it and do away with the otherclass
entirely. Anenum
makes an excellent abstract factory implementation. \$\endgroup\$