HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Managing documents with manager class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managingclasswithmanagerdocuments

Problem

So I have a program that visualizes data from files (csv,txt,etc.). These files are saved to database as individual tables. In my application I have DocumentExplorer which is just a module with a list of available documents(files). You can open and close any document you want, and you can have multiple documents opened at the same time.

This program also uses MEF to separate modules and exposes API module for users to create plugins. What I'm working on is some kind of manager to manage all opened documents in current session. Basically I need something that will be responsible for closing, opening, switching documents. I have 2 ideas how to deal with it, but I'm not sure whether they are any good or not.
This is what I came up with so far:

  1. DocumentManager passed by IoC container with interface exposed inside API module.



public class DocumentManager : IDocumentManager
    {
        private List ActiveDocuments { get; set; }

        public void Create(IDocument document)
        {
            if(!ActiveDocuments.Contains(document))
                ActiveDocuments.Add(document);
        }

        public void Close(IDocument document)
        {
            if (ActiveDocuments.Contains(document))
                ActiveDocuments.Remove(document);
        }

        public IDocument Get(Expression> predicate)
        {
            return ActiveDocuments.AsQueryable().FirstOrDefault(predicate);
        }
    }


  1. DocumentManager as static class kept inside API module, so others can access it. (without IDocumentManager interface, obviously)



What are your thoughts? Are these good approaches? Do you know any good tricks with this kind of problem?

Solution

Manager - Repository or Factory or both?

What is a Manager? It's usually a class that does too much. In this case it tries to be a repository and a factory a the same time. This should be separated.

Why would we do this? To make the usage more obvious and to be able to add functionality to either class without affecting or breaking the others and of course to test each module separately.

DocumentRepository

When implementing the repository consider switching from the List to the HashSet. This would improve not only the performance (which is probably neglectable here - with a dozen of documents no one will notice the change) but it would greaty simplify your code and both methods would be single calls:

public bool Add(IDocument document)
{
    return ActiveDocuments.Add(document);
}

public bool Remove(IDocument document)
{
    remove ActiveDocuments.Remove(document);
}


What about the naming? You call your methods Create and Close but I don't see anything but adding and removing documents to/from a collection. I suggest naming them simply Add and Remove respectively.

If you store the documents in a database then you may add other methods to it. However even better would be to implement two repositories. The first one only knows how to talk to the database and if you need to cache the documents then the decorator repository can do the rest.

Example:

interface IDocumentRepository
{
    IDocument Get(int id);
    bool Add(IDocument document);    
}

class DocumentRepository : IDocumentRepository
{
    public IDocument Get(int id) 
    {
        // get from database
    }
    bool Add(IDocument document)
    {
        // add to database
    }
}

class CachedDocumentRepository : IDocumentRepository
{
    private readonly DocumentRepository _documentRepository;

    private readonly HashSet _cache = new HashSet();

    public CachedDocumentRepository(DocumentRepository documentRepository)
    {
        _documentRepository = documentRepository;
    }

    public IDocument Get(int id) 
    {
        // check the cache and get a document from there or
        // get it from _documentRepository.Get(id);
        // and add to the cache
    }

    bool Add(IDocument document)
    {
        // add to the cache and
        // add to database
    }
}


IEnumerable

With your current implemetation you could remove the Get method and further simplify the repository by implementing the IEnumerable interface for this class.

DocumentFactory

This class should only know how to create documents. It can have methods for creating many different types but nothing else.

Code Snippets

public bool Add(IDocument document)
{
    return ActiveDocuments.Add(document);
}

public bool Remove(IDocument document)
{
    remove ActiveDocuments.Remove(document);
}
interface IDocumentRepository
{
    IDocument Get(int id);
    bool Add(IDocument document);    
}

class DocumentRepository : IDocumentRepository
{
    public IDocument Get(int id) 
    {
        // get from database
    }
    bool Add(IDocument document)
    {
        // add to database
    }
}

class CachedDocumentRepository : IDocumentRepository
{
    private readonly DocumentRepository _documentRepository;

    private readonly HashSet<IDocument> _cache = new HashSet<IDocument>();

    public CachedDocumentRepository(DocumentRepository documentRepository)
    {
        _documentRepository = documentRepository;
    }

    public IDocument Get(int id) 
    {
        // check the cache and get a document from there or
        // get it from _documentRepository.Get(id);
        // and add to the cache
    }

    bool Add(IDocument document)
    {
        // add to the cache and
        // add to database
    }
}

Context

StackExchange Code Review Q#147532, answer score: 2

Revisions (0)

No revisions yet.