Trying to clearly seperate my logic in my program

Question

My problem is I need to know

• is my code in my CRUD functions structured well and effectively? I plan to implement CRUD for Locations entity too. hopefully without redundant code.
• you see how one CRUD function in the program class calls the other CRUD function in the DataAccess Class? I"m trying to set it up so that i can control which entity gets edited or read in the control class, and access it in the dataAccess class. Is this bad logic?

Info on classes

• Program - I'm trying to put all controlling logic here
• DataAccess - I'm trying to put my Data-accessing logic here, away from everything else.
• UserInterface - I'm trying to put my Interface logic here, away from everything else. Such as the way information gets displayed to the user, the way info gets received from user.
• InputValidation - I want to put all logic that validates user input here, ranging from Type validation to input range validation.
• ServiceManager - class that has a method that checks to see if SQL server is running, before doing LINQ calls to DataBase.
• DbResult - a repository wrapper class containing a string message, and a boolean. Used to report failure/success results with a message like "success!" or whatever exception.ToString() prints. I am thinking this class should be removed?
• Man - auto-generated class by entity framework.
• Location - auto-generated class by entity framework. I plan to implement this class in my CRUD functions as soon as I can get working clean logic for the CRUD functions that handle the Man class. And hopefully Man & Location can share the same CRUD functions.

Notes

• I'd like to enable CRUD functions to handle more than one entity class, without copying and pasting 4 CRUD functions for each new entity.
• I'm considering removing DbResult, but not sure how to without keeping Data-Accessing logic separate from User Interface logic. My thought was to pass whatever message needed to be shown to the user, to the Interface class for it to be displayed.

Program Class

class Program
{
    static private UserInterface _MyUI; 
    static void Main(string[] args)
    {
        DbResult resultSQLRunning;
        ServiceManager mySM = new ServiceManager();
        _MyUI = new UserInterface();

        resultSQLRunning = mySM.SQLRunning();

        _MyUI.DisplayMessage(resultSQLRunning.Message);

        if (resultSQLRunning.bSuccess)
        {
            using (TestDatabaseEntities myDB = new TestDatabaseEntities())
            {
                DataAccess myDA = new DataAccess(myDB);
                DbResult myResult = myDA.DatabaseExists();
                _MyUI.DisplayMessage(myResult.Message);
            }
            do
            {
                DecideFromMenuChoice(_MyUI.GetUserAction());
            } while (_MyUI._MyMenuStage != UserInterface.MenuStage.MENU_EXIT);
        }
        Console.ReadLine(); // press any key to exit, basically.
    }

    static private void DecideFromMenuChoice(int choice)
    {
        if (_MyUI._MyMenuStage == UserInterface.MenuStage.MENU_CRUD)
        {
            switch (choice)
            {
                case 1: DoCreate(); break;
                case 2: DoRead(); break;
                case 3: DoUpdate(); break;
                case 4: DoDelete(); break;
                default:
                    break;
            }
        }
        /* else if (_MyUI._MyMenuStage == UserInterface.MenuStage.MENU_TABLE)
        {
            switch (choice)
            {
                // not sure what to do here, somehow select a table to be used in CRUD functions? 
                // do i need to call more CRUD functions for each new table I add?
            }
        }*/
    }
    // ============================
    // CRUD FUNCTIONS
    // ============================
    static private void DoCreate()
    {
    int myID;
        bool isValidID;
        var dbEntities = new TestDatabaseEntities();
        string newName;

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

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

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

        CreationResult = MyDA.Create(new Man() {ManID = myID, Name = newName });
        _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()
    {
        /* The code in this DoUpdate function is "simplified" and "cleaner" 
         * than the commented block below, I'm told. is it?*/
        int myID = _MyUI.GetInput<int>("Enter ID to update: ", int.Parse);
        string newName = _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 = newName;

                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");
        }
    } 
    /*{
        int myID;
        var dbEntities = new TestDatabaseEntities();
        string newName = "";
        DataAccess MyDA = new DataAccess(dbEntities);
        DbResult updateResult, saveResult;

        myID = _MyUI.GetInput<int>("Enter ID to update: ", int.Parse);
        newName = _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 isValidInput;
        var dbEntities = new TestDatabaseEntities();
        DataAccess myDA = new DataAccess(dbEntities);
        DbResult deleteResult, saveResult;

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

        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);
    }
}

DataAccess Class

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 dbResult;

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

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

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

        return dbResult;
    }

    public DbResult Delete(IQueryable myQuery)
    {
        DbResult dbResult;

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

        return dbResult;
    }

    public DbResult Read(IQueryable myQuery, out string[,] records)
    {
        DbResult dbResult;
        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;
            }
            dbResult = DbResult.Success("Read Success");
        }
        catch (Exception e)
        {
            dbResult = DbResult.Failed(e.ToString());
        }

        return dbResult;
    }

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

        return dbResult;
    }
}

UserInterface Class

public class UserInterface
{
    public enum MenuStage
    {
        MENU_EXIT,
        MENU_CRUD,
        MENU_TableSelect
    }
    public MenuStage _MyMenuStage{ get; private set; }

    public UserInterface()
    {
        _MyMenuStage = MenuStage.MENU_CRUD;
    }
    /// <summary>
    /// Prompts user for input with given message, and converts input to type T
    /// </summary>
    /// <typeparam name="T">Value type to convert to, and return</typeparam>
    /// <param name="message">Message to be printed to console</param>
    /// <param name="transform">The type conversion function to use on user's input</param>
    /// <returns>Type T</returns>
    public T GetInput<T>(string message, Converter<string, T> transform)
    {
        DisplayPrompt(message);
        return transform(Console.ReadLine());
    }
    /// <summary>
    /// Asks the user for valid input
    /// </summary>
    /// <typeparam name="T">The type of result to return as out parameter</typeparam>
    /// <param name="message">The message to prompt the user with</param>
    /// <param name="errorMessage">The message to Display to user with if input is invalid</param>
    /// <param name="typeValidator">The TryParse function to use to test the input.</param>
    /// <returns>True if input is valid as per function given to TypeValidator, Result as type T</returns>
    public bool GetValidInput<T>(string message, InputValidation.TryParse<T> typeValidator, out T result, int upper = -1, int lower = -1)
    {
        InputValidation myInputValidator = new InputValidation();
        bool isValid = false;
        bool shouldTestRange = (upper != -1 && lower != -1);
        string input;

        input = GetInput(message, x => (x.Trim()));

        isValid = myInputValidator.ValidateInputType(input, typeValidator, out result);

        if (!isValid)
        {
            DisplayMessage("Error, invalid input type entered, type required is " + typeof(T).ToString());
            return false;
        }

        if (shouldTestRange)
            isValid = myInputValidator.ValidateInputRange(ref result, lower, upper);

        if (!isValid)
        {
            DisplayMessage("Error, input is out of range.  Range: " + lower.ToString() + " - " + upper.ToString());
            return false;
        }

        return isValid;
    }

    public void DisplayMessage(string message)
    {
        Console.WriteLine(message);
    }
    public void DisplayPrompt(string message)
    {
        Console.Write(message);
    }
    public void DisplayRecords(string[,] message)
    {
        if (message == null)
            return;

        DisplayDivider();
        for (int i = 0; i < message.GetLength(0); i++)
        {
            for (int j = 0; j < message.GetLength(1); j++)
            {
                Console.Write(message[i, j] + " ");
            }
            Console.Write("\n"); 
        }
        DisplayDivider();
    }
    public void DisplayMenuOptions(string[] items)
    { 
        byte index = 0;
        DisplayDivider('~');
        Console.WriteLine("Select an action from menu");
        foreach (string s in items)
        {
            Console.WriteLine(index++ + ") " + s);
        }
        DisplayDivider('~');
    }
    public void DisplayDivider(char myChar = '|')
    {
        String myDivider = new String(myChar, 30);

        DisplayMessage(myDivider);
    }

    public int GetUserAction()
    {
        string[] menuOptions;
        int choice = -1;

        //TODO: implement logic that Chooses which MenuStage we're at.

        menuOptions = DecideMenuOptions();
        DisplayMenuOptions(menuOptions);

        choice = GetInputFromRange(0, menuOptions.Length - 1);

        if (choice == 0)
        {
            _MyMenuStage = MenuStage.MENU_EXIT;
        }

        return choice;
    }

    private int GetInputFromRange(int lowerBound, int upperBound)
    {
        int choice;
        bool isValid = false;

        do
        {
            isValid = (GetValidInput<int>("Enter choice> ", int.TryParse, out choice, lower: lowerBound, upper: upperBound));
        } while (!isValid);
        return choice;
    }

    /// <summary>
    /// Determines which menu options need to be displayed
    /// </summary>
    /// <returns>Menu options that need to be displayed</returns>
    private string[] DecideMenuOptions()
    {
        string[] menuCRUD = new string[] { "Exit", "Create", "Read", "Update", "Delete" };
        string[] menuTables = new string[] { "Quit", "Men", "Locations" };
        string[] myMenuChoices = null;

        if (_MyMenuStage == MenuStage.MENU_CRUD)
        {
            myMenuChoices = menuCRUD;
        }
        else if (_MyMenuStage == MenuStage.MENU_TableSelect)
        {
            myMenuChoices = menuTables;
        }

        return myMenuChoices;
    }
}

InputValidation Class

public class InputValidation//<T> where T: class
{
    /// <summary>
    /// Delegate that matches the signature of TryParse, method defined for all primitives.
    /// </summary>
    /// <typeparam name="T">Output type of This Delegate</typeparam>
    /// <param name="input">input for this Delegate to translate to type T</param>
    /// <param name="output">The translated variable to return via out parameter</param>
    /// <returns>Whether the Parse was successful or not, and output as output</returns>
    public delegate bool TryParse<T>(string sInput, out T output);
    public bool ValidateInputType<T>(string sInput, TryParse<T> TypeValidator, out T result)
    {
        return TypeValidator(sInput, out result);
    }
    public bool ValidateInputRange<T>(ref T result, int lower, int upper)
    {
        // How can I use relational operators like > < = on T? without forcing T result to an int?
        return isValidRange(int.Parse(result.ToString()), lower, upper);
    }
    public bool isValidRange(int item, int Lower, int Upper)
    {
        return (Lower <= item && item <= Upper);
    }
}

ServiceManager Class

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;
    }
}

DbResult (wrapper class)

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);
    }
}

Man Class

public partial class Man
{
    public Man()
    {
        this.Locations = new HashSet<Location>();
    }

    public int ManID { get; set; }
    public string Name { get; set; }

    public virtual ICollection<Location> Locations { get; set; }
}

Location Class

public partial class Location
{
    public Location()
    {
        this.Men = new HashSet<Man>();
    }

    public int PlaceID { get; set; }
    public string Place { get; set; }

    public virtual ICollection<Man> Men { get; set; }
}

UPDATE:

11/20/12 9:01 CST: Improved seperation of logic in UserInterface class and Program Class. At least I think so. 11/21/12 6:46 CST: Improved UserInterface class slightly.

Answer 1

Since it's to show off to an employer I'll look at it in that context.

One style issue I have is that you are mixing naming conventions. You are using both hungarian notation and CamelCase. I would suggest to get rid of the hungarian notation (sParam, bIsTrue, ...).

Regarding the overall structure of your code:

• The Program-class:

  • it has too much logic (too many responsibilities). I would refactor the logic out of there into a mediator-kind of class. The Program-class should only start something.
  • IMHO: the interpretation of the user input (choice 1, 2, etc) should be done by the UI class (actually if you go MVC the controller, but it looks like overkill). The UI class should expose events or delegates where the program (or better a mediator) can plugin the necessary actions.

• The DataAccess-class:

  • The DataAccess class has the role of a repository. Why not call it that. It'll show you know the repository pattern. However on second thought it looks like it has the role of a helper class?
  • Use IEnumerable<string> and yield return instead of a string array in the read method
  • No disposing of the TestDatabaseEntities? It is missing from the CRUD methods on your program class. Usually the TestDatabaseEntities has a lifetime equal to the repository, a repository encapsulates the `TestDatabaseEntities and exposes the disposing by implementing IDisposable.

• Also in your create method why do this:

  _MyDBEntities.Men.Add(new Man { ManID = M.ManID, Name = M.Name });

and not directly:

_MyDBEntities.Men.Add(M);

I hope that this is in the direction of the kind of advice you were hoping for?

I'll see if I can whip up an example later that explains better what I am trying to tell.

Answer 2

This is hard, because you're writing an application which is specifically a pure CRUD app, so the distinction between the business logic and the database logic isn't as clear as it usually is. Most applications don't let you just create a new _____, they do that for you behind the scenes as you manipulate the UI. Additionally, I'm not an expert - I've done a lot of experimenting with EF, but I have yet to use it in anything resembling a production context.

That being said, I think almost everything in your DataAccess class needs to go. See these three posts for examples of working with POCO data objects, which looks kindof like what you're trying to do.

Your DataAccess class should probably only expose a Get method for each data type, and an overall SubmitChanges method that saves everything. Get will determine whether the objects exists, and return either a new object (as generated by context.CreateObject<>()) or the object loaded from the database. Then you can manipulate that object as you choose, and SubmitChanges will save all changes to all objects back to the database. You might need to implement a Delete for each object as well, but you can probably avoid that somehow.

From my understanding, proper n-tier architecture means that nothing outside of the data layer should have any idea how CRUD operations work - to everything else, it's just a sequence of either "Load" or "Load, Mutate, Save".

Does this help?