Implementing DAO with SOLID

Question

I might not be keeping with Single Responsibility with my DAO. I also implement a custom observable through update. It makes sense because when something changes in the data, then it makes sense for the observers to be updated. Is there way for me separate them but still able to update when I update the data structures? Also is there way to clean my code for readability? I'm trying to do defense coding on each method.

DAO

public class RAMTradeDAO
        implements
            GenericDAO<Trade, UUID, Boolean>,
            Update<Observer<Set<UUID>, Map<UUID, Trade>, ControllerType>>
{
    private static Map<UUID, Trade>    tradeList;
    private static Set<UUID>           requests;
    private static ArrayList<Observer>  observers   = new ArrayList<Observer>();
    private String                     availability;

    public RAMTradeDAO()
        {
            tradeList = new ConcurrentHashMap<UUID, Trade>();
            requests = Collections.synchronizedSet(new TreeSet<UUID>());
        }

    @Override
    public boolean update(Trade trade, Boolean active) throws NotFoundException
        {
            if (trade == null || trade.getTradeId() == null
                    || !tradeList.containsKey(trade.getTradeId())) throw new NotFoundException(
                    "RAMTradeDAO update NULL");

            tradeList.put(trade.getTradeId(), trade);

            if (!active)
                {
                    requests.add(trade.getTradeId());
                }

            return active;

        }

    @Override
    public boolean delete(Trade trade, Boolean active) throws NotFoundException
        {
            if (trade == null || trade.getTradeId() == null
                    || !tradeList.containsKey(trade.getTradeId())) throw new NotFoundException(
                    "RAMTradeDAO delete NULL");

            if (!active)
                {
                    if (requests.contains(trade.getTradeId()))
                        {
                            LOG.SYSTEM.info("Trade " + trade.getTradeId()
                                    + " was deleted from request");
                            requests.remove(trade.getTradeId());
                        }
                    else
                        {
                            throw new NotFoundException(
                                    "RAMTradeDAO delete no request found");
                        }
                }
            else
                {
                    LOG.SYSTEM.info("Trade " + trade.getTradeId()
                            + " was deleted");
                    tradeList.remove(trade.getTradeId());
                    requests.remove(trade.getTradeId());
                }
            return true;
        }

    @Override
    public Trade findByKey(UUID tradeID, Boolean active)
            throws NotFoundException
        {
            if (tradeID == null || !tradeList.containsKey(tradeID)) throw new NotFoundException(
                    "RAMTradeDAO findByKey NULL");

            if (!active)
                {
                    if (!requests.contains(tradeID)) throw new NotFoundException(
                            "RAMTradeDAO findByKey request");

                    return tradeList.get(tradeID);
                }

            return tradeList.get(tradeID);
        }

    @Override
    public Trade findByValue(Trade trade, Boolean active)
            throws NotFoundException
        {
            if (trade == null || trade.getTradeId() == null
                    || !tradeList.containsKey(trade.getTradeId())) throw new NotFoundException(
                    "RAMTradeDAO findByValue NULL");

            if (!active)
                {
                    if (!(requests.contains(trade.getTradeId()))) throw new NotFoundException(
                            "RAMTradeDAO findByValue request");

                    return tradeList.get(trade.getTradeId());
                }

            return tradeList.get(trade.getTradeId());
        }

    @Override
    public boolean insert(Trade trade, Boolean active) throws NotFoundException
        {
            if (trade == null || trade.getTradeId() == null) throw new NotFoundException(
                    "RAMTradeDAO Insert");

            tradeList.put(trade.getTradeId(), trade);

            if (trade.getStatus().equals(TradeStatus.REQUEST))
                {
                    requests.add(trade.getTradeId());
                }

            LOG.USER.info("Trade_ID: " + trade.getTradeId() + " Request_type: "
                    + trade.getStatus());

            notifyObservers();

            return true;
        }

    @Override
    public void notifyObservers() throws NotFoundException
        {
            if (observers == null || tradeList == null) throw new NotFoundException(
                    "notifyObservers NULL");

            LOG.CONSOLE.debug("RAMTradeDAO notifyObservers");
            for (Observer ob : observers)
                {
                    LOG.CONSOLE
                            .debug("Notifying to all the subscribers that theres been update");
                    ob.update(requests, requests, tradeList);
                }

        }

    @Override
    public void registerObserver(Observer observer) throws NotFoundException
        {
            if (observers == null || observer == null) throw new NotFoundException(
                    "registerObserver NULL");

            LOG.CONSOLE.debug("RegisterObserver");

            observers.add(observer);
            notifyObservers();
        }

    @Override
    public void removeObserver(Observer observer) throws NotFoundException
        {
            if (observers == null || observer == null
                    || !observers.contains(observer)) throw new NotFoundException(
                    "registerObserver NULL");

            observers.remove(observer);
        }

    /**
     * @return the tradeList
     * @throws InsertionException
     * @throws NotFoundException
     */
    public static Map<UUID, Trade> getTradeList() throws NotFoundException
        {
            if (tradeList == null) throw new NotFoundException(
                    "RAMTradeDAO getTradeList");

            return tradeList;
        }

    /**
     * @return the buyRequests
     * @throws InsertionException
     * @throws NotFoundException
     */
    public static Set<UUID> getRequests() throws NotFoundException
        {
            if (requests == null) throw new NotFoundException(
                    "RAMTradeDAO getRequests");

            return requests;
        }
}

Answer 1

Exceptions

You're using exceptions wrong. You're throwing exceptions of type NotFoundException for things like passing null in, broken internal state, just not finding results...

First, the exceptions should be identifyable by CLASS. That is, NotFoundException should only be returned if a requested object was not found. Java 7 has Objects.requireNonNull (java.util.Objects), you can use that for null variables. For internal state failures, throw IllegalStateException instead. Lastly, you might want to consider returning null if you didn't find something (but that's your call).

ConcurrentHashMap

The default amount of shards that the HashMap will be split in is 16. Do you intend to have 16 threads? Maybe you were looking for Collections.synchronizedMap instead? Note that if you do intend to have multiple threads, you can get null from your find functions: They call the function, it's in the map, another thread comes by, calls remove, removes it from the map, and then you try to retrieve it in the first thread. null.

Naming Collection Variables

@Override
public boolean update(Trade trade, Boolean active) throws NotFoundException
    {
        if (trade == null || trade.getTradeId() == null
                || !tradeList.containsKey(trade.getTradeId())) throw new NotFoundException(
                "RAMTradeDAO update NULL");

        tradeList.put(trade.getTradeId(), trade);

Wait, what? If the thing you're gonna update is not in a List then report an error... else add it?! But then you'll get duplicates! Oh wait, it's a Map! Name it something like tradeMap, that should prevent confusion like this.

Multiple returns

@Override
public Trade findByKey(UUID tradeID, Boolean active)
        throws NotFoundException
    {
        if (tradeID == null || !tradeList.containsKey(tradeID)) throw new NotFoundException(
                "RAMTradeDAO findByKey NULL");

        if (!active)
            {
                if (!requests.contains(tradeID)) throw new NotFoundException(
                        "RAMTradeDAO findByKey request");

                return tradeList.get(tradeID);
            }

        return tradeList.get(tradeID);
    }

It's not bad to have multiple returns. However, here you have needlessly complicated the look of the code by mixing various types of braces, putting in a double return... lemme rewrite it like so:

@Override
public Trade findByKey(UUID tradeID, Boolean active)
        throws NotFoundException
    {
        if (tradeID == null || !tradeList.containsKey(tradeID)) throw new NotFoundException("RAMTradeDAO findByKey NULL");

        if (!active && !requests.contains(tradeID))
            {
                throw new NotFoundException("RAMTradeDAO findByKey request");
            }

        return tradeList.get(tradeID);
    }

This also applies to findByValue.

Code formatting

private static Map<UUID, Trade>    tradeList;
private static Set<UUID>           requests;
private static ArrayList<Observer>  observers   = new ArrayList<Observer>();
private String                     availability;

Coding style is a bit of a sensitive point amongst programmers... but there's some flaws with this style of lining up variable names:

• If you make one a private static final VeryLongClassNameWithSomeOtherAdjectivesAddedToIt then you have to add more spaces to all your variables.
• It's easy to mess this formatting up, like you did. It now looks ugly and is no longer doing what it should do (make things more readable)
• This approach tends to exclude adding javadoc to your variables. Because that would require inserting comments in between your variables.

Answer 2

Dodgy initialization

Setting static fields in the constructor is dodgy:

private static Set<UUID>           requests;

public RAMTradeDAO()
    {
        tradeList = new ConcurrentHashMap<UUID, Trade>();
        requests = Collections.synchronizedSet(new TreeSet<UUID>());
    }

If you create another instance of the DAO, it will recreate requests. Is that really what you want?

And in the getRequests method you're checking requests != null, which makes no sense, it always exists.

Excessive null checks

Why do you have so many null checks?

This is often a sign of poor class design and poor validation at the time of construction. Take for example your Trade class. You're checking if trade.getTradeId() != null several times. So you can have a Trade object with tradeId = null? Does it makes sense? I think it shouldn't. If it doesn't make sense, then the Trade class shouldn't allow creating such invalid instances. And then you can omit the null checks for this value.

At other times your null checks are simply pointless, for example observers != null, when the variable is declared as:

private static ArrayList<Observer>  observers   = new ArrayList<Observer>();

So observers is never null, you don't need to check it.

Another thing, you should have declared this as a List instead of an ArrayList:

private static List<Observer>  observers   = new ArrayList<Observer>();