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.

I am using C# and the .net entity framework, and I'm also learning how to better use OOP concepts in my program. My code is all displayed below. I would like to ensure my logic is properly organized before I try to implement the next step in my program. So my goal is twofold:

  1. Ensure my Code is SOLID, OOP, Reusable, etc.
  2. Enable my code to select via a menu, like the one that currently exists in program, which table it uses CRUD functions on.

Because have 2 entities in my entity model, and the code currently uses only the Man entity. But I'm hoping I don't need additional copies of the CRUD functions for alternative entities. I'm hoping that someone in StackExchange knows how to implement CRUD methods that will take any entity I give it.

I think that my code is organized in a MVC fashion. where Program is the controller, UserInterface is the view, and DataAccess is the Model?

Program class

class Program
{
    private enum MenuStage
    {
        MENU_EXIT,
        MENU_CRUD
        //MENU_TableSelect
    }
    static private MenuStage _MyMenuStage;
    static void Main(string[] args)
    {          
        DoUICycle();
        Console.ReadLine();
    }

    // ================================
    // MENU DISPLAY AND PROCESSING
    // ================================
    private static void DoUICycle()
    {   
        string [] sCRUDMenu = new string[] {"Exit", "Create", "Read", "Update", "Delete" };
        string [] sTableMenu = new string[] {"Quit", "Men", "Locations"};
        string[] sMyMenuChoices;
        UserInterface MyUI = new UserInterface();
        int iChoice = -1;

        _MyMenuStage = MenuStage.MENU_CRUD;
        while (_MyMenuStage != MenuStage.MENU_EXIT)
        {
            if (_MyMenuStage == MenuStage.MENU_CRUD)
            {
                sMyMenuChoices = sCRUDMenu;
            }
            else //if (MyStage == MenuStage.MENU_Tables)
            {
                sMyMenuChoices = sTableMenu;
            }
            while (iChoice != 0)
            {
                iChoice = MyUI.GetMenuInput(sMyMenuChoices);
                MenuSwitch(iChoice);
            }
            if (iChoice == 0)
            {
                _MyMenuStage = MenuStage.MENU_EXIT;
            }
        }
    }

    static private void MenuSwitch(int selection)
    {
        if (_MyMenuStage == MenuStage.MENU_CRUD)
        {
            switch (selection)
            {
                case 1: DoCreate(); break;
                case 2: DoRead(); break;
                case 3: DoUpdate(); break;
                case 4: DoDelete(); break;
                default:
                    break;
            }
        }
        /* else if (_MyStage == MenuStage.MENU_TableSelect)
        {
            // should i really use enum? or IAmAnEntity interface variable
            switch (selection)
            {
                // 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 bIsValidID;
        var dbEntities = new TestDatabaseEntities();
        string sNewName;

        UserInterface MyUI = new UserInterface();
        DataAccess MyDA = new DataAccess();

        do
        {
            bIsValidID = MyUI.GetValidTypeInput<int>("Enter ID: ", "Error: ID must be an integer", int.TryParse, out myID);
        }
        while (!bIsValidID);

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

        MyDA.CreateMan(dbEntities, new Man() {ManID = myID, Name = sNewName });

        SaveChanges(dbEntities);
    }
    static private void DoRead()
    {
        var dbEntities = new TestDatabaseEntities();
        UserInterface MyUI = new UserInterface();
        DataAccess MyDA = new DataAccess();

        var query = from person in dbEntities.Men
                    where true
                    select person;
        MyDA.ReadMan(dbEntities, query);
    }
    static private void DoUpdate()
    {
        int myID;
        var dbEntities = new TestDatabaseEntities();
        string sNewName = "";
        UserInterface MyUI = new UserInterface();
        DataAccess MyDA = new DataAccess();

        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;

        MyDA.UpdateMan(dbEntities, query, new Man() { ManID = myID, Name = sNewName });

        SaveChanges(dbEntities);
    }
    static private void DoDelete() 
    {
        int myID;
        bool bValidInput;
        var dbEntities = new TestDatabaseEntities();
        UserInterface MyUI = new UserInterface();
        DataAccess MyDA = new DataAccess();

        do
        {
            bValidInput = MyUI.GetValidTypeInput<int>("Enter ID to delete: ", "ID Invalid, please re-enter", int.TryParse, out myID);
        } while (!bValidInput);
        var Query =
            from person in dbEntities.Men
            where person.ManID == myID
            select person;

        MyDA.DeleteMan(dbEntities, Query);

        SaveChanges(dbEntities);
    }
    // ============================
    // SAVECHANGES FUNCTION
    // ============================
    private static void SaveChanges(TestDatabaseEntities dbEntities)
    {
        DataAccess MyDA = new DataAccess();

        MyDA.TryDataBase(dbEntities, "Changes saved successfully", () => dbEntities.SaveChanges());
    }
}

UserInterface Class

public class UserInterface
{
    /// <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 input, out T output);
    /// <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 GetValidTypeInput<T>(string message, string errorMessage, TryParse<T> TypeValidator, out T result, int upper = -1, int lower = -1)
    {
        bool bIsValid = false;
        bool bTestValidRange = (upper != -1 && lower != -1);

        DisplayPrompt(message);

        bIsValid = TypeValidator(Console.ReadLine(), out result);
        if (!bIsValid)
            DisplayDBMessage(errorMessage);

        if (bTestValidRange && bIsValid)
        {
            bIsValid = isValidRange(int.Parse(result.ToString()), lower, upper);
            if (!bIsValid)
                DisplayDBMessage("Input out of valid range");
        }

        return bIsValid;
    }
    public bool isValidRange(int item, int Lower, int Upper)  
    { 
        return (Lower <= item && item <= Upper);
    }
    // ============================
    // DISPLAY FUNCTIONS
    // ============================
    public void DisplayDBMessage(string Msg)
    {
        Console.WriteLine(Msg);
    }
    public void DisplayPrompt(string Msg)

    {
        Console.Write(Msg);
    }
    public void DisplayRecord(string [] Msg)
    {
        if (Msg == null)
            return;

        foreach (string s in Msg)
        {
            Console.Write(s + " ");
        }
        Console.Write("\n"); 
    }
    public void DisplayMenuItems(string[] items)
    { 
        byte ChoiceIndex = 0;
        DisplayDivider('~');
        Console.WriteLine("Select an action from menu");
        foreach (string s in items)
        {
            Console.WriteLine(ChoiceIndex++ + ") " + s);
        }
    }
    public void DisplayDivider(char cCharacter = '|')
    {
        String sDivider = new String(cCharacter, 30);

        DisplayDBMessage(sDivider);
    }
    // ============================
    // MENU PROMPTING FOR USER INPUT
    // ============================
    public int GetMenuInput(string[] sChoices)
    {
        int selection;
        bool bValid = false;           
        UserInterface MyUI = new UserInterface();

        DisplayMenuItems(sChoices);

        do
        {
            bValid = (MyUI.GetValidTypeInput<int>("Enter> ", "Error: Numbers only> ", int.TryParse, out selection, lower: 0, upper: sChoices.Length - 1));
        } while (!bValid);
        return selection;
    }
}

DataAccess Class

public class DataAccess
{
    // ============================
    // CRUD FUNCTIONS for MAN TABLE
    // ============================
    public bool CreateMan(TestDatabaseEntities dbEntities, Man M)
    {
        return TryDataBase(dbEntities, "Man created successfully",
            () =>
            {
                dbEntities.Men.Add(new Man { ManID = M.ManID, Name = M.Name });
            });
    }

    public bool UpdateMan(TestDatabaseEntities dbEntities, IQueryable<Man> query, Man man)
    {
        return TryDataBase(dbEntities, "Man updated successfully",
            () =>
            {
                foreach (Man M in query)
                {
                    M.Name = man.Name;
                }
            });
    }

    public bool DeleteMan(TestDatabaseEntities dbEntities, IQueryable myQuery)
    {
        return TryDataBase(dbEntities, "Man deleted successfully",
            () =>
            {
                foreach (Man M in myQuery)
                {
                    dbEntities.Men.Remove(M);
                }
            });
    }

    public bool ReadMan(TestDatabaseEntities dbEntities, IQueryable myQuery)
    {
        UserInterface MyUI = new UserInterface();
        bool bSuccessful;

        bSuccessful = TryDataBase(dbEntities, "Records read successfully",
            () =>
            {   
                MyUI.DisplayDivider();
                foreach (Man m in myQuery)
                {
                    MyUI.DisplayRecord(new string[] { m.ManID.ToString(), m.Name });
                }
                MyUI.DisplayDivider();
            });


        return bSuccessful;
    }

    // ============================
    // TRY FUNCTION
    // ============================
    public bool TryDataBase(TestDatabaseEntities MyDBEntities, string SuccessMessage,  Action MyDBAction)
    {
        UserInterface MyUI = new UserInterface();
        try
        {
            MyDBAction();
            MyUI.DisplayDBMessage(SuccessMessage);
            return true;
        }
        catch (Exception e)
        {
            MyUI.DisplayDBMessage(e.ToString());
            return false;
        }
    }
}

Man Class (auto-generated)

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 (auto-generated)

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; }
}
share|improve this question
add comment

2 Answers

up vote 1 down vote accepted

A few comments on your code as follows :

  1. Your UI code should not contain a SaveChanges methods as per SRP, it should be in EF itself.

  2. Your UserInterface is actually just a utility method. Encapsulate your validation and prompt logic different places. Although Utility method is not considered as good but it is okay to use in this case although their responsibility should be defined properly.

  3. Your whole architecture is like UI => BL =>EF so EF layer should not interact with UI Code. As TryDataBase methods is showing the errors in prompt. This is not good idea as this concerns to UI not EF or BL. Throw known exception in case of error do not handle it but log it inside BLL or EF.

I would advise you to write test cases even before writing any line of code. By writing unit test properly you can ensure that you followed the SOLID pattern or design pattern. So do this re-factoring part using Unit Test. By EOD eventually you will start following all applicable design pattern.

Create a layer between EF and BLL to improve the code reuse (put them in separate projects, this will help you keep the code in the correct place).

share|improve this answer
1  
What kind of stuff would go in this layer between the EF and BLL. Could you provide an example maybe? –  dreza Nov 9 '12 at 18:28
    
@dreza, I am also confused by that portion of paritosh's explanation. –  Matt Rohde Nov 9 '12 at 20:46
    
I think I skipped the part of Data access layer which is defined above.I am really sorry.. –  paritosh Nov 10 '12 at 6:31
add comment

Your design is very complicated. As stated you should not instantiate a userinterface class in your data calls. Also there are some fundamental problems beyond your design such as magic numbers choice =-1, routes with poor names Menuswitch, etc. I recommend you buy Code Complete by Steve McConnell and read it twice then once each year. As far as your data access goes.. I'd look around for a code template that creates repository classes and a unit of work class to wrap all entity framework calls. You can fid one that will create it all for you based on your model. Good luck.

share|improve this answer
    
two or three routines that simply take a string and then call console.writeline.. this is duplicate code and too much abstraction.. its better in this instance to simply call console.writeline instead of wrapping it in multiple methods –  Buddy James Nov 11 '12 at 14:11
    
renaming MenuSwitch() to MakeDecisionFromMenuSelection() or DecideFromSelection() or DecideFromMenuChoice() be better? –  Matt Rohde Nov 11 '12 at 16:26
    
I agree it's complicated, I"m trying to simplify it, but without repeating any code and keeping single responsibility. –  Matt Rohde Nov 11 '12 at 23:11
add comment

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.