Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In my C# program, I have a class defined as follows:

public class DbResult
{
    public bool bSuccess { get; private set; }
    public String Message { get; private set; }

    private DbResult(bool success, string message)
    {
        this.bSuccess = success;
        this.Message = message;
    }

    public static DbResult Failed(string message)
    {
        return new DbResult(false, message);
    }

    public static DbResult Success(string message)
    {
        return new DbResult(true, message);
    }
}

This is used in my DataAccess class, which has several methods whose return type is DbResult. They use LINQ and the Entity Framework. UPDATE Here is the entire DataAccess as of right now:

public class DataAccess
{
    private readonly TestDatabaseEntities _MyDBEntities;

    public DataAccess(TestDatabaseEntities entities)
    {
        _MyDBEntities = entities;
    }


    public DbResult DatabaseExists()
    {
        DbResult MyResult;

        if (_MyDBEntities.Database.Exists())
            MyResult = DbResult.Success("Database Found");
        else
            MyResult = DbResult.Failed("Database Not Found");

        return MyResult;
    }

    // ============================
    // CRUD FUNCTIONS for MAN TABLE
    // ============================

    public DbResult Create(Man M)
    {
        DbResult DbRes;

        try
        {
            _MyDBEntities.Men.Add(new Man { ManID = M.ManID, Name = M.Name });
            DbRes = DbResult.Success("Record created");
        }
        catch (Exception e)
        {
            DbRes = DbResult.Failed(e.ToString());   
        }            
        return DbRes;
    }

    public DbResult Update(IQueryable<Man> myQuery, Man man)
    {
        DbResult DbRes;

        try
        {
            foreach (Man M in myQuery)
            {
                M.Name = man.Name;
            }
            DbRes = DbResult.Success("Record updated");
        }
        catch (Exception e)
        {
            DbRes = DbResult.Failed(e.ToString());   
        }

        return DbRes;
    }

    public DbResult Delete(IQueryable myQuery)
    {
        DbResult DbRes;

        try
        {
            foreach (Man M in myQuery)
            {
                _MyDBEntities.Men.Remove(M);
            }
            DbRes = DbResult.Success("Record deleted");
        }
        catch (Exception e)
        {
            DbRes = DbResult.Failed(e.ToString());                
        }

        return DbRes;
    }

    public DbResult Read(IQueryable myQuery, out string[,] Records)
    {
        DbResult DbRes;
        Records = null;
        try
        {
            List<Man> men = myQuery.OfType<Man>().ToList();
            Records = new string[men.Count, 2];
            for (int i = 0; i < men.Count; i++)
            {
                Records[i, 0] = men[i].ManID.ToString();
                Records[i, 1] = men[i].Name;
            }
            DbRes = DbResult.Success("Read Success");
        }
        catch (Exception e)
        {
            DbRes = DbResult.Failed(e.ToString());
        }

        return DbRes;
    }

    // ============================
    // SAVECHANGES FUNCTION
    // ============================
    public DbResult SaveChanges()
    {
        DbResult DbRes;
        try
        {
            _MyDBEntities.SaveChanges();
            DbRes = DbResult.Success("Saved successfully");
        }
        catch (Exception e)
        {
            DbRes = DbResult.Failed(e.ToString());
        }

        return DbRes;
    }
}

I also have a class called ServiceManager, which checks to see if the SQL Server (SQLEXPRESS) service is running on the local machine.

public class ServiceManager
{
    public DbResult SQLRunning()
    {
        DbResult MyResult;
        ServiceController sc = new ServiceController("SQL Server (SQLEXPRESS)");

        if (sc.Status == ServiceControllerStatus.Running)
            MyResult = DbResult.Success("SQL Server is running");
        else
            MyResult = DbResult.Failed("SQL Server is NOT running.");

        return MyResult;
    }
}

As you may have noticed in the code directly above, I use the DbResult class as a return type to carry the status message and bool back to the main program. I think this does not make sense since DbResult is a class created for returning results from trying to perform operations on the Database, not the SQL Server checkup. I could create a new class called ServiceResult, which does the exact same thing as DbResult, but that would be duplicating code. see example below:

public class ServiceResult
{
    public bool bRunning { get; private set; }
    public String Message { get; private set; }

    private ServiceResult(bool bRunning, string message)
    {
        this.bRunning = success;
        this.Message = message;
    }

    public static ServiceResult Running(string message)
    {
        return new ServiceResult(false, message);
    }

    public static ServiceResult Stopped(string message)
    {
        return new ServiceResult(true, message);
    }
}

You see how the above class is very similar to the DbResult class and works relatively the same way? This is how I might make a class for Services, but is there any way I can make a class that does both DbResults and ServiceResults without becoming more ambiguous like the following class example below? or, is it ok to be ambiguous like the result I put below?

public class Result
{
    public bool bYesNo { get; private set; }
    public String Message { get; private set; }

    private Result(bool YesNo, string message)
    {
        this.bYesNo = YesNo;
        this.Message = message;
    }

    public static Result No(string message)
    {
        return new Result(false, message);
    }

    public static Result Yes(string message)
    {
        return new Result(true, message);
    }
}

UPDATED

The methods that use my DataAccess class looks like this

    static private void DoCreate()
    {
        int myID;
        bool bIsValidID;
        var dbEntities = new TestDatabaseEntities();
        string sNewName;

        DataAccess MyDA = new DataAccess(dbEntities);
        DbResult CreationResult, SaveResult;

        do
        {
            bIsValidID = _MyUI.GetValidInput<int>("Enter ID: ", int.TryParse, out myID);
        }
        while (!bIsValidID);

        sNewName = _MyUI.GetInput<string>("Enter Name:", x => x.Trim());

        CreationResult = MyDA.Create(new Man() {ManID = myID, Name = sNewName });
        _MyUI.DisplayMessage(CreationResult.Message);

        if (!CreationResult.bSuccess)
            return;

        SaveResult = MyDA.SaveChanges();
        _MyUI.DisplayMessage(SaveResult.Message);
    }
    static private void DoRead()
    {
        var dbEntities = new TestDatabaseEntities();
        DataAccess MyDA = new DataAccess(dbEntities);
        string [,] Records;
        DbResult ReadResult;

        var query = from person in dbEntities.Men
                    where true
                    select person;
        ReadResult = MyDA.Read(query, out Records);
        if (ReadResult.bSuccess)
        {
            _MyUI.DisplayRecords(Records);
        }
        if (!ReadResult.bSuccess)
            _MyUI.DisplayMessage(ReadResult.Message);
    }
    static private void DoUpdate()
    {
        int myID;
        var dbEntities = new TestDatabaseEntities();
        string sNewName = "";
        DataAccess MyDA = new DataAccess(dbEntities);
        DbResult UpdateResult, SaveResult;

        myID = _MyUI.GetInput<int>("Enter ID to update: ", int.Parse);
        sNewName = _MyUI.GetInput<string>("Enter new name: ", x => x.Trim());

        var query =
            from person in dbEntities.Men
            where person.ManID == myID
            select person;

        UpdateResult = MyDA.Update(query, new Man() { ManID = myID, Name = sNewName });
        _MyUI.DisplayMessage(UpdateResult.Message);

        if (!UpdateResult.bSuccess)
            return;

        SaveResult = MyDA.SaveChanges();
        _MyUI.DisplayMessage(SaveResult.Message);
    }
    static private void DoDelete() 
    {
        int myID;
        bool bValidInput;
        var dbEntities = new TestDatabaseEntities();
        DataAccess MyDA = new DataAccess(dbEntities);
        DbResult DeleteResult, SaveResult;

        do
        {
            bValidInput = _MyUI.GetValidInput<int>("Enter ID to delete: ", int.TryParse, out myID);
        } while (!bValidInput);

        var Query =
            from person in dbEntities.Men
            where person.ManID == myID
            select person;

        DeleteResult = MyDA.Delete(Query);
        _MyUI.DisplayMessage(DeleteResult.Message);

        if (!DeleteResult.bSuccess)
            return;

        SaveResult = MyDA.SaveChanges();
        _MyUI.DisplayMessage(SaveResult.Message);
    }
share|improve this question
3  
Just a comment on your code, using Hungarian Notation is not a standard naming convenction, especially on public properties. stackoverflow.com/questions/309205/… –  Jeff Vanzella Nov 19 '12 at 17:35
add comment

1 Answer

up vote 2 down vote accepted

Can I try to convince you that you don't actually need such a Result class at first place?

How do you actually use the string Message value of this class? Do you show it to user? If yes, then what would you do when you need to make it multilingual? What I'm trying to say is that verbal interpretation of "success" or "failure" should actually be done on UI level, and Result class is not a UI. Also there is usually a single "success" case. You may say that you might want to differentiate several failure cases, but those are best described by a nice technology called exceptions.

So, what do we have beside Message? A boolean flag. That's what we probably want to return in case if we're interested whether something was successful or not... but wait. What usually happens when something goes wrong? Exception. And it would probably be more useful to let the business code process this exception, as the same exception may be treated differently in different situations.

Now let's go back to your code. You ServiceManager class doesn't have a state, so let's make it static:

public static class ServiceManager
{
    public static bool IsSqlServerRunning()
    {
        //TODO: What if service is not installed?
        ServiceController sc = new ServiceController("SQL Server (SQLEXPRESS)");
        return (sc.Status == ServiceControllerStatus.Running);
    }
}

Now it's much cleaner, and the caller of this method can decide what to output in case when SQL Server is not running...

About DB wrappers - I would suggest to avoid them at all. They represent abstractions over abstraction (Entity framework). By wrapping adds/deletes/selects in separate methods you loose lots of benefits provided by ORM frameworks like transactional nature of the DB interaction, without getting pretty much any benefit. It's actually much easier and cleaner to explicitly define context boundaries (in case of Web processing boundaries are already defined) and directly use the ORM to manipulate with the data.

Update based on updated question. There are several issues in the code that uses DataAccess class:

  • All methods are static. It's not an issue for a small codebase and console app like yours, but don't overuse them.
  • The following code is actually exactly the same as just dbEntities.Men

    from person in dbEntities.Men
    where true
    select person
    
  • naming conventions (local variables should start with lower case
  • don't convert Man objects into 2-dimensional array...

I'll show you how to simplify the code based on DoUpdate method, it includes most of the tasks done in other methods. No DataAccess class usage, cleaner code, better exception management.

static private void DoUpdate()
{
    int myID = _MyUI.GetInput<int>("Enter ID to update: ", int.Parse);
    string sNewName = _MyUI.GetInput<string>("Enter new name: ", x => x.Trim());

    try
    { 
        using (var dbEntities = new TestDatabaseEntities())
        {
            //TODO: If ManID is unique then we can use FirstOrDefault here.
            var allMatchingMen =
                from person in dbEntities.Men
                where person.ManID == myID
                select person;

            foreach(var man in allMatchingMen)
                man.Name = sNewName;

            dbEntities.SaveChanges();
            _MyUI.DisplayMessage("Record(s) updated");
        }
    }
    catch (OptimisticConcurrencyException ex)
    {
        //TODO: add auto-retry logic
        _MyUI.DisplayMessage("Someone updated the record, let's retry");
    }
    catch (Exception ex)
    {
        _MyUI.DisplayMessage("Something went wrong, could not update");
    }
}
share|improve this answer
    
I disagree on the avoiding DB wrappers. What happens when it is decided that the data storage should be changed from SQL to, say Mongo, or Solr. You have tightly bound your application to SQL. By having a properly designed data access layer using interfaces, you just have to create the class that performs the needed actions on the data store and plug it into the application. –  Jeff Vanzella Nov 19 '12 at 17:29
    
The business code would still "know" about the underlying ORM, at least in order to optimize the DB usage, otherwise you'll quickly get a system that doesn't use the benefits of the specific DB platform. Also, switching from SQL to Mongo won't be that easy anyway, e.g. because of lack of ACID. There is a good article why you shouldn't wrap ORM into repository from one of the core contributors of NHibernate: ayende.com/blog/4784/… –  almaz Nov 19 '12 at 18:09
    
I understand why I shouldn't use the wrapper class now, however, I don't understand how what I was doing with the wrapper classes is considered "wrapping ORM" into a class. I just see a boolean, and a message, with some methods to regulate how they are assigned values. On a note of lesser importance, the exception gets passed to the DbResult.Failure method from the data Access layer, where the exception occurs. and then the message gets displayed in the UI class. –  Matt Rohde Nov 19 '12 at 18:55
    
By "wrapping ORM" I meant your DataAccess class that hides the work with ORM classes. Concerning "exception gets passed to the DbResult.Failure method" - that's exactly what I wouldn't suggest to do... By passing a string (probably exception.ToString()) instead of rich exception object you loose a lot of information. For certain exceptions later you may want to implement automatic retries, for others you may want to show a customised notification rather than stack trace ;) –  almaz Nov 19 '12 at 20:50
    
@almaz I've updated my question to include the methods in my main class(which runs the menu to display to user) which use my DataAccess Class. After what you've told me, I sense there must be something wrong with creating dbEntities in those methods and passing it to the dataAccess class. Would i be correct in guessing that a lot of that code should be transferred to the DataAccess Class? –  Matt Rohde Nov 20 '12 at 4:03
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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