5
\$\begingroup\$

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 principle would argue the existence of App_Code/User.cs at all ... why not just reference the DataProject directly?)

If anyone can provide any suggestions for me to improve this (particularly with a look at removing redundant code) - possibly explaining any concepts that I may seem to be misunderstanding - I'd really appreciate it.

\$\endgroup\$
4
\$\begingroup\$

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<User>. 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<User> _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 handling and logging, dependency injection. But those would be beyond the scope of this question.

\$\endgroup\$
4
\$\begingroup\$

The most important thing that needs to be fixed that I see:

User us = new User(Convert.ToInt32(Session["ActionUserID"]));

Using Session State for Request Data

Consider this example scenario:

Suppose the user views a User userOne in one page then another User userTwo on another page, say two compare them against each other. Then he decides that the two Users are duplicates and userOne is the superfluous one. So he goes back to the page displaying userOne and click "Delete" button. But because the page displaying userTwo was loaded later it overwritten the Session["ActionUserID"] property with the ID of userTwo. Therefore userTwo is deleted even though the user clicked the "Delete" on userOne's page.

Using session and application state should be avoided as much as possible:

  • The state associated with a page goes in the ViewState or hidden fields etc.

  • The state associated with a session goes in the Session.

  • The state associated with the whole application goes in the HttpContext.Application.

Using Magic Constants as Keys

You are getting Session["ActionUserID"] above, i.e. ... = ... Session["ActionUserID"] ... There must be somewhere else that you must be setting it, i.e. something like Session["ActionUserID"] = user.ID. Suppose you decided to rename the key from "ActionUserID" to "UserID", but instead renamed one occurence to "UserID" and the other to "UserId". You have a serious bug, but your application happily builds.

All string keys should be defined as constants:

private const string UserIdKey = "ActionUserID";

Now if you type UserIDKey instead of UserIdKey you have a build error right away. If you decide to change the key value, just change the constant value and it will work (for all new sessions that is. You should make sure that you do not need to change keys often.)

For other points see @BenAaronson's answer

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.