current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I am wondering if in particular the function called AssignDBStatusMessage and TryDataBaseAction are unnecessary. It seems to me that the logic is more cluttered if i do away with those functions. Please provide me with your ideas and thoughts.

If you have other thoughts please let me know.

man is an entity class generated by entity framework. TestDataBaseEntities is the DBcontext item.

public class DataAccess
{
    // ============================
    // CRUD FUNCTIONS for MAN TABLE
    // ============================
    public bool CreateMan(TestDatabaseEntities dbEntities, out string StatusMessage, Man M)
    {
        string ErrorMessage;
        bool bSuccessful;

        bSuccessful = TryDataBaseAction(dbEntities, out ErrorMessage,
            () =>
            {
                dbEntities.Men.Add(new Man { ManID = M.ManID, Name = M.Name });
            });
        StatusMessage = AssignDBStatusMessage("Records created successfully", ErrorMessage, bSuccessful);

        return bSuccessful;
    }

    public bool UpdateMan(TestDatabaseEntities dbEntities, IQueryable<Man> query, out string StatusMessage, Man man)
    {
        string ErrorMessage;
        bool bSuccessful;

        bSuccessful = TryDataBaseAction(dbEntities, out ErrorMessage,
            () =>
            {
                foreach (Man M in query)
                {
                    M.Name = man.Name;
                }
            });
        StatusMessage = AssignDBStatusMessage("Records updated successfully", ErrorMessage, bSuccessful);

        return bSuccessful;

    }

    public bool DeleteMan(TestDatabaseEntities dbEntities, IQueryable myQuery, out string StatusMessage)
    {
        string ErrorMessage;
        bool bSuccessful;

        bSuccessful = TryDataBaseAction(dbEntities, out ErrorMessage,
            () =>
            {
                foreach (Man M in myQuery)
                {
                    dbEntities.Men.Remove(M);
                }
            });

        StatusMessage = AssignDBStatusMessage("Records deleted successfully", ErrorMessage, bSuccessful);

        return bSuccessful;
    }

    public bool ReadMan(TestDatabaseEntities dbEntities, IQueryable myQuery, out string StatusMessage, out string[,] Records)
    {
        string ErrorMessage;
        bool bSuccessful;
        string[,] TheseRecords = null;

        // hands an Action() to TryDataBase, as indicated by lambda expression in 3rd arguement.
        bSuccessful = TryDataBaseAction(dbEntities, out ErrorMessage,
        () =>
        {
            List<Man> men = myQuery.OfType<Man>().ToList();
            TheseRecords = new string[men.Count, 2];

            for (int i = 0; i < men.Count; i++)
            {
                TheseRecords[i, 0] = men[i].ManID.ToString();
                TheseRecords[i, 1] = men[i].Name;
            }
        });

        Records = TheseRecords;

        StatusMessage = AssignDBStatusMessage("Records read successfully", ErrorMessage, bSuccessful);


        return bSuccessful;
    }

    // ============================
    // SAVECHANGES FUNCTION
    // ============================
    public bool SaveChanges(TestDatabaseEntities dbEntities, out string StatusMessage)
    {
        bool bSuccessful;
        string ErrorMessage;
        bSuccessful = TryDataBaseAction(dbEntities, out ErrorMessage, () => dbEntities.SaveChanges());

        StatusMessage = AssignDBStatusMessage("Save changes Sucessful", ErrorMessage, bSuccessful);

        return bSuccessful;
    }
    // ============================
    // Helper functions?
    // ============================
    public bool TryDataBaseAction(TestDatabaseEntities MyDBEntities, out string ErrorMessage, Action MyDBAction)
    {
        UserInterface MyUI = new UserInterface();
        try
        {
            MyDBAction();
            ErrorMessage = "No Errors";
            return true;
        }
        catch (Exception e)
        {
            ErrorMessage = e.ToString();
            return false;
        }
    }

    private string AssignDBStatusMessage(string SuccessMsg, string FailureMsg, bool bSuccessful)
    {
        if (bSuccessful)
            return SuccessMsg;
        else
            return FailureMsg;
    }
}
share|improve this question

1 Answer 1

up vote 2 down vote accepted

Not huge thoughts but a couple of things.

  1. I would probably consider making the entities object a class level parameter since it seems to be used in every method.

  2. I wouldn't put the success message in this class as that seems more caller dependant. Hence I would let the caller determine what success message they want on a true result. I would only supply a failed message to at least inform the user what happened.

  3. Is this sort of a repository pattern... not sure. But I might toy with the idea of changing DataAccess to ManRepository and getting rid of man in the function names so it's just Create, Update and Read.

i.e.

public class ManRespository
{
   private readonly TestDatabaseEntities _dbEntities;
   public ManRepository(TestDatabaseEntities dbEntities)
   {
      _dbEntities = dbEntities;
   }

   // Your Create, Update, Read methods here
   public DbResult Create(Man man) 
   {
      // etc
   }
}
  1. In light of 2, I might consider not returning bool at all but rather a result class.

Something like this perhaps:

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

   private DbResult(boolean success, string message)
   {
      Success = success;
      Message = message;
   }

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

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

Then an example of the one of your methods might now be simplified to:

public DbResult Create(Man M)
{
    return TryDataBaseAction(
        () =>
        {
            _dbEntities.Men.Add(new Man { ManID = M.ManID, Name = M.Name });
        });
}

public DbResult TryDataBaseAction(Action MyDBAction)
{
    string errorMessage = string.Empty;

    try
    {
            // What was MyUI supposed to do here?  I would think mixing UI
            // dependencies at this level was not a good idea unless it was through
            // an abstraction such as an Interface or Abstract class of some sort?
        UserInterface MyUI = new UserInterface();

        MyDBAction();       

        return DbResult.Success(string.Empty);
    }
    catch (Exception e)
    {
        errorMessage = e.ToString();
    }

    return DbResult.Failed(errorMessage);
}

Note: AssignDBStatusMessage actually became redundant in this approach and TryDataBaseAction is seeming a bit weak now??

share|improve this answer
    
MyUI was something I forgot to remove. In the original code, i had MyUI call its method to display output to user cause it seemed like going through less hoops and hurdles than passing it back to the main program. –  Matt Rohde Nov 13 '12 at 21:59
    
On what occurances are you expecting the method to fail then? I think it would be best to do away with the TryDataBaseAction() action altogether and put that stuff into the individual Creates, Updates as I assume they fail for different reasons?? –  dreza Nov 13 '12 at 22:04
    
Well, sometimes it's simply that the SQL server isn't up and running, in which case the CRUD methods would fail then. But yes, they'd fail for different reasons otherwise, Update and Delete could fail if the targeted Record ID wasn't found, Read? i don't know why it would. Create? possibly if the new record's ID already existed. (I still need to make a fix for that). I am trying to plan so that I can can possibly use the CRUD functions on an alternate entity Locations, which is also a table in my entity model. I was just trying to get a good foundation going for the man entity first. –  Matt Rohde Nov 14 '12 at 0:15
    
@ArmorCode I quite like the repository pattern. Perhaps this might be an option to explore for yourself? –  dreza Nov 14 '12 at 0:41
1  
My thoughts were that why do you need to supply a success message from this layer at all. If it succeeded that's great. If the caller wants to display a message then let them put one together.... Just my thoughts anyway. –  dreza Nov 14 '12 at 22:29

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.