InSaNiTy
InSaNiTy

Reputation: 150

Class that stores all essential objects is the right way to go?

The problem is a design problem in Object-Oriented Programming that I'm trying to find or think of the best solution for, And not sure of the solutions I have thought of so far.

The problem:

I wrote a class called ComponentsHub whose idea is to store within it all the main objects that are essential for running the program. Each object is static, final, and public. I use a public access modifier to make it easy to access an object via static import or just to access it statically.

package com.rtransfer.net.components;

import java.io.IOException;
import java.util.logging.FileHandler;
import java.util.logging.Logger;

import com.rtransfer.net.system.StorageManager;

public class ComponentsHub {

    public static final Logger logger;
    
    public static final Server server;
        
    public static final StorageManager storageManager;

    public static final SecurityManager securityManager;
    
    public static final RequestForwarder requestForwarder;
    
    public static final Authenticator authenticator;
    
    public static final Uploader uploader;

    public static final ConnectionHandler connectionHandler;

    public static final Listener listener;
    
    static {
        logger = Logger.getLogger("rtransfer.net");
        
        try {
            FileHandler handler = new FileHandler("logs/logs.txt");
            logger.addHandler(handler);
        } catch (SecurityException | IOException e) {
            e.printStackTrace();
        }       
        server = new Server();
            
        storageManager = new StorageManager();

        securityManager = new SecurityManager();
        
        requestForwarder = new RequestForwarder();
        
        authenticator = new Authenticator();
        
        uploader = new Uploader();

        connectionHandler = new ConnectionHandler();

        listener = new Listener();
    }
}

It's pretty obvious that there are some issues with this code/design (Hope you can tell me some of them). For example, not separating concerns, hard to test, unscalable and more. I do not feel comfortable with this architecture so much, I think I have created some kind of a "God Class".

Solutions I thought of:

I know this is a pretty common type of problem in designing object-oriented systems. I want to solve this so that I do not have to live in nightmares in the further development of my software.

Are the solutions I proposed solves part of the problem? Are there alternatives? I would be happy if you would recommend me suitable design patterns or other techniques that will allow me to solve this problem. How and where should I create objects and allow fairly simple access to them?


Comments and answers outcome:

As suggested in one of the comments, I implemented ServiceLocator, which looks significantly better than the previous idea. Now, I know there are disagreements as to whether it is Anti-pattern or not, and I also understand what its disadvantages are, but without the relevant framework, it would be quite difficult and tiring (as also mentioned in one of the answers) to use traditional dependency injection.

public class InitialContext {
    
    public ServiceComponent lookup(String serviceName) {
        if (serviceName.equalsIgnoreCase(Listener.class.getSimpleName())) {
            return new Listener();
        } else if (serviceName.equalsIgnoreCase(ConnectionHandler.class.getSimpleName())) {
            return new ConnectionHandler();
        } else if (serviceName.equalsIgnoreCase(Uploader.class.getSimpleName())) {
            return new Uploader();
        } else if (serviceName.equalsIgnoreCase(Authenticator.class.getSimpleName())) {
            return new Authenticator();
        } else if (serviceName.equalsIgnoreCase(RequestForwarder.class.getSimpleName())) {
            return new RequestForwarder();
        } else if (serviceName.equalsIgnoreCase(SecurityManager.class.getSimpleName())) {
            return new SecurityManager();
        } else if (serviceName.equalsIgnoreCase(StorageManager.class.getSimpleName())) {
            return new StorageManager();
        } else if (serviceName.equalsIgnoreCase(Server.class.getSimpleName())) {
            return new Server();
        } else {
            return null;
        }
    }
}
public class ServiceCache {

    private Hashtable<String, ServiceComponent> services;
    
    public ServiceCache() {
        services = new Hashtable<>();
    }
    
    public ServiceComponent getService(Class<?> service) {
        return services.get(service.getSimpleName());
    }
    
    public void addService(ServiceComponent newService) {
        services.put(newService.getClass().getSimpleName(), newService);
    }
}
package com.rtransfer.net.components;

import com.rtransfer.utils.InitialContext;
import com.rtransfer.utils.ServiceCache;

public class ServiceLocator {

    private static ServiceCache cache = new ServiceCache();
    
    public static ServiceComponent getService(Class<?> serviceClass) {
        ServiceComponent service = cache.getService(serviceClass);
        
        if (service != null)
            return service;
        InitialContext context = new InitialContext();
        
        service = context.lookup(serviceClass.getSimpleName());
        cache.addService(service);
        
        return service;
    }
}

So finally I've decided to use dependency injection.

Upvotes: 0

Views: 137

Answers (3)

Ben Seidel
Ben Seidel

Reputation: 494

Depending on your circumstances you may or may not have an issue on your hand, and that maybe issue can be summarised by the umbrella term of "Singleton instances".

Singleton instances get a lot of unwarranted abuse. They are considered bad by just about the entire programming community. But Singletons are a perfectly valid design pattern and from the way you have structured your question, I would recommend that you continue to use them. My motto is, if you want something that can quack and swim and fly but everyone tells you that ducks are bad, who cares what everyone thinks!

Singletons are the embodiment of the things that your application can only have at most one of. Eg, a Main Form or a set of logged in credentials - both of which are singleton instances. You need Singletons in ALL applications.

However, I would suggest that you don't put all your singletons in one basket/class. Apart from violating the Single Responsibility Principal, it will eventually become large and unwieldy as more and more global state of your program gets put into this catch-all. This is generally what happens with Singletons and IMO, is why they are hated so much.

Instead, design your singletons as if they weren't.

  1. Create an interface for each of your Singletons.
    This will allow you to easily replace the implementation of your singleton as needed.
  2. Have setter/getter methods somewhere close to each interface.
    This will allow you to easily replace the way you start and access each singleton, maybe replacing it with a service manager later down the track.
  3. Make sure that each interface is in a separate project to the project that contains the implementations. This will prevent you from unexpectedly creating dependencies on some implementation detail of your singleton.

There are a seemingly wide range of issues raised by the other answers, most of which have absolutely nothing to do with the design choices of how to house your Singleton instances. The best part of starting simple is you can deal with each issue as it arrives.
Having threading issues? Wrap the offending singleton in a lock.
It's only a singleton because it's expensive to build? Change it to a pool of objects.
You want to simulate Dependency Injection? Change your accessor to use a thread-local variable.
You want to have multiple users logged in at once? Change your accessor to look at the user credentials.

Another thing you can do is in the class with you accessor method, create static versions of each of the functions in the interface. Each of these static functions should then just execute through the accessor method, eg

public class Database
{
   public static DatabaseInterface instance() { return instance_; }
   public static boolean connect()  {return instance().connect(); }
}

This way you can remove all the accessor calls from your code.

Always remember, it's the readability of the code that matters most above all else.

Upvotes: 0

SaleemKhair
SaleemKhair

Reputation: 541

As you said It quite a common problem in OOP, and the Idea you have come up with is a good idea but the implementation is poor, for alot of reasons,

What you are trying to do here is the concept of a Context And Dependency Injection. You want a Class that is available to supply instances for you on the go:

SrorageManager storageManager = ComponentsHub.getStorageManager();

But you need to think about some important aspects, from the top of my head:

  1. Mutability
  2. Thread-Safty

And ofcourse Separation Of Concern. Some of those Classes might be subject to multiple threads, some might be Singletons and some might be just Utility classes.

So I propose first; Interfaces

Separation: Define your contracts.


Security Context

public interface SecurityContext {
    public SecurityManager getSecurityManager();
    public Authemticator getAuthenticator();
}

WebContext

public interface WebContext {
    public Uploader getUploader();
    public RequestForwarder getRequestForwarder();
    public Server getServer();
    .
    .
    .
}

And maybe a Data or Persistence Context.


Thread Safe


If you are considering being able to support multi-threading you need to take into consideration a way for control and synchronisation, either using java API Locks or using the syncronized keyword, or maybe have some thread local work isolated, depends on your case.


Mutability


Who can change the state of the contexts? Are contexts created as one-per-application or is it one-per-thread (request) like a database transaction for example.

Do you want to have everything stating from app and never to be changed at runtime for example, maybe a DB credentials changed, maybe you want to implement another Authenticator, those classes should be dynamic, and not static, and a context have the flexibility to give you different implementations.


To summerize I what I think you should be able to do is:

  1. Create the factories/suppliers/instances of the classes in the fields outside the context let's call those dependencies
  2. initialize the context, Using builders and inject the required dependencies new PersistanceContextBuilder.withStorageManager(new FileSystemStorageManager).build();
  3. Figure out a way to provide those context to the rest of the application, preferably Create Services or Controllers that are also separated and depend on different contexts.

Upvotes: 1

Thomas
Thomas

Reputation: 181805

However you structure this singleton object, it remains a singleton (or collection of singletons). It can create unexpected interdependencies between your classes, and makes them difficult to test or reason about in isolation.

The magic phrase to put into a search engine is "dependency injection". That term comes with a lot of baggage and people sometimes associate it with complicated frameworks, but at the core, the principle is really simple: rather than reach out and find its dependencies autonomously, each class should require that the code using the class provides the dependencies.

Not this:

class Dog {
    void bark() {
        SoundManager.emitSound("woof");
    }
}

But this:

class Dog {
    private final SoundManager soundManager;

    public Dog(SoundManager soundManager) {
        this.soundManager = soundManager;
    }

    void bark() {
        this.soundManager.emitSound("woof");
    }
}

And this principle goes up the dependency chain: if a Kennel wants to create a Dog object, it will also need to request a SoundManager from its caller.

You can imagine that it gets tedious to pass all these constructor arguments, especially as your codebase grows. That's where the frameworks are useful.

Upvotes: 7

Related Questions