OOP between projects

So, I wanted a separate project to act as the data layer for my website project.

Classes in the website should reference the data layer after getting input from the user and should get back information regarding the result of whatever actions are performed and then communicate those results to the user.

The following is an example of how I've got it set out. This example deletes a user record from the database:

// Website/Admin/Users.aspx.cs
protected void btnDelete_Click(object sender, EventArgs e)
{
    User us = new User(Convert.ToInt32(Session["ActionUserID"]));

    if (us.Delete())
    {
        lblResult.Text = "User record was successfully deleted.";
    }
    else
    {
        lblResult.Text = us.errmsg;
    }
}

// Website/App_Code/User.cs
//
// For simplicity sake, I've removed code that isn't needed in this example
public int userid { get; protected set; }
public string errmsg { get; protected set; }

public User(int UserID)
{
    userid = UserID;
}

public bool Delete()
{
    OrtundUser ou = new OrtundUser(userid);
    if (ou.Delete())
    {
        return true;
    }
    else
    {
        errmsg = ou.errmsg;
        return false;
    }
}

// DataProject/OrtundUser.cs
public int userid { get; protected set; }
public string errmsg { get; protected set; }

OrtundData od = new OrtundData(); // instantiate the DbContext

public OrtundUser(int UserID)
{
    userid = UserID;
}

public bool Delete()
{
    try
    {
        var user = (from u in od.users
                    where u.userid == userid
                    select u).FirstOrDefault();

        if (user != null)
        {
            od.Remove(user);
            od.SaveChanges();

            return true;
        }
    }
    catch (Exception ex)
    {
        errmsg = ex.InnerException;
        return false;
    }
}

I feel like there's a lot of repetition of code here. I know this will work as intended, but I don't know if it's as efficient as it could be. (for example, the KISS principl

A couple of general points:

You shouldn't be using properties to set an error message like that. The error message is associated with the action you attempted on the object- in this case the delete- not the object itself. After the error has been handled, it doesn't make any sense for the property to continue holding that error message. It also potentially adds unnecessary concurrency woes.

You could fix that by wrapping up the bool you're returning and the message into a class, like DbActionResult. Or you could make the message an out parameter. But this still isn't a great solution because you have a leaking of concerns between your layers. Why should the data layer directly dictate what's being displayed in the UI like that? Better options would be either:

• Return some kind of status code from the data layer, and have the UI display a message based on that code. If you don't need any more information in the UI than just whether or not it passed, a bool would be sufficient. If you want to log the exact exception details for debugging, that can be done with some kind of logging framework

OR

• Don't catch the exception at all. Or if you do catch it to do some kind of handling/logging, rethrow it. That way you can access it directly in the UI layer if necessary, but the UI decides how to handle turning that exception into a message to display to the user, rather than the data layer doing it.

Also, this may have just been because it was an example, but OrtundUser is a bad name for a class. A User is a User, it shouldn't be a UserWhichIsPersistedInAParticularWay. That violates the Single Responsibility Principle, as it now has to take responsibility not only for what it's actually supposed to do- represent a user- but also do at least some management of its own persistence. You also have a similar problem with your actual User class, though it's not reflected in the name. By putting the Delete method on the user, it now has to be concerned not only with what it actually is, but with how to persist itself

This is related to the actual meat of the question. Your first step should be to separate your User class out into User and UserRepository like so:

public class User
{
    public int userid { get; set; }
}

public class UserRepository
{
    public bool Delete(User user)
    {
        //Delete logic goes here
    }
}

That's not a full example, but it'll be refined later. Hopefully you can see how this has now separated out different responsibilities into their own classes.

From your mention of DbContext in a comment, I assume you're using Entity Framework. EF actually already provides you with a class that will serve the purpose UserRepository here, which is DbSet. You should be able to get that from your OrtundData. That class will handle operations like Delete for you. So in fact the only class from the above example that you actually need is the User class.

(As a side note, this is an implementation of what's called the 'repository pattern'. If you look this up, you'll see it's often recommended that even when using Entity Framework, you add your own repository class on top- UserRepository in the above example. I wouldn't worry about that at the moment, it adds another layer of abstraction which can be valuable in some cases, but don't do it without understanding why. It's more important to get the basics in place.)

So finally coming to whether User and OrtundUser are redundant. Yes, they are. Once you remove the persistence logic from both of them as described above, you're left with nothing but a property bag which is identical for both of them. You should delete OrtundUser, and push User down into your data layer.

In fact, in large solution you could push futher- have a "Core" project referenced by both the UI and Data projects which contains your domain objects like User. Data would then reference Core, and UI would reference both Data and Core. In your case, actually structuring your solution like this is likely to be unnecessary, but it's a good way to think about it conceptually.

So, finally, with all of the above applied your code might look something like this (again this is omitting some details like construction, just leaving you with the key bits):

// Website/Admin/Users.aspx.cs
private DbSet _userRepository;
private SomeSortOfLogger _logger;

protected void btnDelete_Click(object sender, EventArgs e)
{
    User us = new User(Convert.ToInt32(Session["ActionUserID"]));

    try
    {
       _userRepository.Delete(us);
       lblResult.Text = "User record was successfully deleted.";
    }
    catch(Exception e)
    {
        _logger.Log(e);
        lblResult.Text = "There was an error, see log for details";
    }
}

// DataProject/User.cs
public int userid { get; protected set; }

public User(int UserID)
{
    userid = UserID;
}

There's likely still scope for improvement- e.g. adding a better way of doing exception/error h

StackExchange Code Review Q#47350, answer score: 4