Should my service access layer communicate with my data layer?

Question

I have separated my application into two pieces. The first piece is my core functionality project. The other piece is the UI of the application. The core functionality should communicate with web services and interact with my SQLite databases.

In the core application I have four layers. These layers are:

• Business Layer
• Data Access Layer
• Data Layer
• Service Access Layer

I shall describe what each layer is in my application and where it is used for.

Business Layer

This layer holds my business models. It also holds some managers. These managers are important because they are communicating with the UI of the platform specific application. An example of such a manager is listed below:

public class AlbumManager
{
    static AlbumManager ()
    {
    }

    public static Album GetAlbum(int id) {
        return DAL.AlbumRepository.GetAlbum (id);
    }

    public static IList<Album> GetAlbums (int venue_id)
    {
        return new List<Album> (DAL.AlbumRepository.GetAlbums (venue_id));
    }

    public static int SaveAlbums(IList<Album> albums) {
        int count = 0;
        foreach(var album in albums){
            if (DAL.AlbumRepository.SaveAlbum (album) != 0)
                count += 1;
        }
        return count;
    }

    public static int SaveAlbum (Album item)
    {
        return DAL.AlbumRepository.SaveAlbum(item);
    }

    public static int DeleteAlbum(int id)
    {
        return DAL.AlbumRepository.DeleteAlbum(id);
    }
}

All it does is providing methods that are communicating with my repositories.

Data Access Layer

This layer is holding my repositories. One such repository looks like this:

public class AlbumRepository
{

    DL.AppDatabase db = null;
    protected static AlbumRepository me;

    static AlbumRepository() {
        me = new AlbumRepository();
    }

    protected AlbumRepository ()
    {
        db = new PhotoWapp.DL.AppDatabase (Constants.DatabaseFilePath);
    }

    public static Album GetAlbum(int id)
    {
        return me.db.GetItem<Album>(id);
    }

    public static IEnumerable<Album> GetAlbums (int venue_id)
    {
        return me.db.GetItems<Album>();
    }

    public static int SaveAlbum (Album item)
    {
        return me.db.SaveItem<Album>(item);
    }

    public static int DeleteAlbum(int id)
    {
        return me.db.DeleteItem<Album>(id);
    }
}

I am just listing this class because you get an overview about how my application is behaving.

Data Layer

The data layer is communicating with the SQLite database. The most important class looks like this:

public class AppDatabase : SQLiteConnection
{
    static object locker = new object ();

    /// <summary>
    /// Initializes a new instance of the <see cref="Tasky.DL.TaskDatabase"/> TaskDatabase. 
    /// if the database doesn't exist, it will create the database and all the tables.
    /// </summary>
    /// <param name='path'>
    /// Path.
    /// </param>
    public AppDatabase (string path) : base (path)
    {
        // create the tables
        CreateTable<Album> ();
    }

    public IEnumerable<T> GetItems<T> () where T : BL.Contracts.IBusinessEntity, new ()
    {
        lock (locker) {
            return (from i in Table<T> () select i).ToList ();
        }
    }

    public T GetItem<T> (int id) where T : BL.Contracts.IBusinessEntity, new ()
    {
        lock (locker) {
            return Table<T>().FirstOrDefault(x => x.ID == id);
            // Following throws NotSupportedException - thanks aliegeni
            //return (from i in Table<T> ()
            //        where i.ID == id
            //        select i).FirstOrDefault ();
        }
    }

    public int SaveItem<T> (T item) where T : BL.Contracts.IBusinessEntity
    {
        lock (locker) {
            if (item.ID != 0) {
                InsertOrReplace (item);
                // Update (item);
                return item.ID;
            } else {
                return Insert (item);
            }
        }
    }

    public int DeleteItem<T>(int id) where T : BL.Contracts.IBusinessEntity, new ()
    {
        lock (locker) {
            #if NETFX_CORE
            return Delete(new T() { ID = id });
            #else
            return Delete<T> (new T () { ID = id });
            #endif
        }
    }
}

It has interaction with the database itself and nothing else.

Service Access Layer

And I have a service layer which should interact with a web service later on, but I have not yet written that webservice, so I am demonstrating this in-memory service.

public class MemoryAlbumService : Interfaces.AlbumService
{

    public MemoryAlbumService ()
    {

    }

    #region AlbumService implementation

    public void GetAlbum (int id)
    {

        var album = new Album ();

        album.ID              = id;
        album.Title               = string.Format("Title {0}", id);
        album.Description      = string.Format("Beschrijving {0}", id);
        album.Start_Of_Album = DateTime.Now.AddDays(-1);
        album.End_Of_Album      = DateTime.Now.AddDays(5);
        album.Venue_Id          = 1;
        album.Thumbnail_Url  = "http://testurl";

        BL.Managers.AlbumManager.SaveAlbum (album);

        OnFetchCompleted(EventArgs.Empty);

    }

    public virtual void OnFetchCompleted(EventArgs e) {
        EventHandler handler = OnCompleted;
        if(handler != null) {
            handler (this, e);
        }
    }

    public event EventHandler OnCompleted;
}

Per my understanding, this service layer should only return data. And it should not do anything else. Is this right? Because I wanted to do the following from my services:

1. Get data
2. Raise an event when the data has been fetched
3. Then ask the manager for the updated data from the platform specific application.

I know my service isn't returning anything right now, but I just want to know if this method is valid how I am approaching this.

Does anybody has any valuable tips about solving this or any tips do to it in a separate way?

PS:

Somebody responded to my Stack Overflow post with the following answer:

Just retrieve data from the web service, nothing else. You can then create a facade which uses both the web service and the local service to keeps things in sync.

Answer 1

This looks pretty over-engineered, I'd tend to agree with the comment to your SO post.

The most pressing issue I'm seeing with your code is this one:

• Why isn't AlbumManager a static class if it only exposes static members?
• ...and why do you need all these static members in the first place?

...the same applies to AlbumRepository. I think you're abusing static here, the C# paradigm is oop; static classes (/members) live at the type level - in OOP you want your members to live at the object/instance level.

I'm not a multithreading guru, but I would believe this is potentially problematic:

static object locker = new object();

The locker being static means every instance of the AppDatabase class is using the same locker object, hence there'd be no guarantee that two or more threads can't enter the locked code, ...making the lock blocks (and the locker itself) pretty much useless.

---

Keep it simple: code your business logic against abstractions - your logic needs something that gives it an IEnumerable<Album>? Then have your business logic use some IAlbumService that exposes an IEnumerable<Album> GetAlbums() method.

You want your DAL in a separate assembly? Good idea. Create another assembly, and implement the BLL interfaces in that class library - yes, this means the DAL references the BLL; this is called inversion of control. The BLL has no idea about any other assembly, and it doesn't care who is implementing these interfaces, nor how they're implemented.

Whether you want to call it a service or a repository, at the end of the day all that matters is that the BLL code has a way of getting its data. You don't need this lasagna in-between! Data Layer, Data Access Layer and Service Access Layer.... ... are you not missing a Service Layer? How about a DAL assembly with DAL.Services and DAL.Repositories namespaces instead? You're making your life harder than it needs to be!

Keep it simple, thank yourself later ;)

---

Side note, this: protected static AlbumRepository me; should be PascalCase as a protected member, and it makes a very confusing Singleton (and may I add, WTF protected static??). I don't see a reason for a repository to be implemented as a Singleton, and as a general rule of thumb I'd steer away from Singleton. And me is a bad name for something most Singleton implementations call instance.