5
\$\begingroup\$

I have a console application that I'm trying to make with minimal redundant code.

I have 4 functions, Create, Read, Update, and Delete.

  • is it possible or practical to combine the four functions into one function that takes different parameters to create, read, update, or delete?
  • is it possible to also adjust that one combined function to use whichever of my two entity tables (Man or Location) I tell it to use?

I also have a helper static class that provides some functions to make some tasks easier.

4 CRUD FUNCTIONS

class Program
{
    #region CRUD functions
    static private void DoCreate()
    {
        int myID;
        var dbEntities = new TestDatabaseEntities();
        string sNewName = "";

        myID = stcHelper.InsistOnValidInput<int>("Enter ID: ", "ID must be Valid: ", int.TryParse);
        sNewName = stcHelper.GetInput<string>("Enter Name", x => x.Trim());

        stcHelper.TryDataBase(dbEntities, () =>
            {
                dbEntities.Men.Add(new Man{ManID = myID, Name = sNewName});
                Console.WriteLine("Insert Sucessful");
            });


        stcHelper.SaveDBChanges(dbEntities);
    }
    static private void DoRead()
    {
        var dbEntities = new TestDatabaseEntities();
        string sDisplayDivider = "|||||||||||||||||||||||||||||||||||||||";
        var query = from person in dbEntities.Men
                    where true
                    select person;


        Console.WriteLine(sDisplayDivider);
        stcHelper.TryDataBase(dbEntities, () =>
            {
                foreach (Man m in query)
                {
                    Console.WriteLine(m.ManID + " " + m.Name);
                }
            });

        Console.WriteLine(sDisplayDivider);
    }
    static private void DoUpdate()
    {
        int myID;
        var dbEntities = new TestDatabaseEntities();
        string sNewName = "";

        // get The Target's ID for the user to update
        myID = stcHelper.GetInput<int>("Enter ID to update: ", int.Parse);
        // get the new name from user
        sNewName = stcHelper.GetInput<string>("Enter new name: ", x => x.Trim());

        // Query the database for the row to be updated.
        var query =
            from person in dbEntities.Men
            where person.ManID == myID
            select person;

        // Execute the query, and change the column values
        stcHelper.TryDataBase(dbEntities, () =>
            {
                foreach (Man M in query)
                {
                    M.Name = sNewName;
                    Console.WriteLine("Update Sucessful!");
                }
            });

        stcHelper.SaveDBChanges(dbEntities);
    }
    static private void DoDelete() 
    {
        int myID;
        //T myID;
        var dbEntities = new TestDatabaseEntities();

        myID = stcHelper.InsistOnValidInput<int>("Enter ID to delete: ", "ID Invalid, please re-enter", int.TryParse);

        // Query the database for the rows to be deleted.
        var Query =
            from details in dbEntities.Men
            where details.ManID == myID
            select details;

        stcHelper.TryDataBase(dbEntities, () =>
            {
                foreach (var detail in Query)
                {
                    dbEntities.Men.Remove(detail);
                    Console.WriteLine("Removal successful.");
                }
            });

        stcHelper.SaveDBChanges(dbEntities);
    }
    #endregion
}

HELPER CLASS

public static class stcHelper
{   
    /// <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>
    static public T GetInput<T>(string message, Converter<string, T> transform)
    {
        Console.Write(message);
        return transform(Console.ReadLine());
    }
    /// <summary>
    /// Repeatedly asks the user for valid input
    /// </summary>
    /// <typeparam name="T">The type of result to return</typeparam>
    /// <param name="message">The initial message to prompt the user with</param>
    /// <param name="errorMessage">The message to prompt user with if input is invalid</param>
    /// <param name="validator">The TryParse function to use to test the input.</param>
    /// <returns>Type T</returns>
    public static T InsistOnValidInput<T>(string message, string errorMessage, TryParse<T> validator)
    {
        Console.Write(message);
        T result;
        while (!validator(Console.ReadLine(), out result))
        {
            Console.Write(errorMessage);
        }
        return result;
    }
    static public void SaveDBChanges(TestDatabaseEntities MyDBEntities)
    {
        // Submit the changes to the database.
        TryDataBase(MyDBEntities,
            () =>
            {
                MyDBEntities.SaveChanges();
                Console.WriteLine("Save Changes Successful.");
            });
    }
    static public void TryDataBase(TestDatabaseEntities MyDBEntities, Action MyAction)
    {
        try
        {
            MyAction();
        }
        catch (Exception e)
        {
            Console.WriteLine("Database interaction failed: " + e.ToString());
            return;
        }
    }
}

Man and Location. Auto-generated

by the entity framework

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

Man and Location, NON-Auto Generated

public partial class Location : IAmAnEntity
{
    public void PromptNewName()
    {
        Place = stcHelper.GetInput<string>("Give Location Name: ", (x) => x.Trim());
    }

    public void PromptNewID()
    {
        PlaceID = stcHelper.InsistOnValidInput<int>("Give New ID: ", "ID must be integer: ", int.TryParse);
    }

    public void CreateMe()
    {
        PromptNewID();
        PromptNewName();
    }

    public void ReadMe()
    {
        throw new NotImplementedException();
    }

    public void UpdateMe()
    {
        throw new NotImplementedException();
    }

    public void DeleteMe()
    {
        throw new NotImplementedException();
    }
}
public partial class Man : IAmAnEntity
{
    public void PromptNewName()
    {
        Name = stcHelper.GetInput<string>("Enter Name for new person", i => i.Trim());
    }

    public void PromptNewID()
    {
        ManID = stcHelper.InsistOnValidInput<int>("Enter ID for new person:", "ID must be integer: ", int.TryParse);
    }

    public void CreateMe()
    {
        PromptNewID();
        PromptNewName();
    }


    public void ReadMe()
    {
        throw new NotImplementedException();
    }

    public void UpdateMe()
    {
        throw new NotImplementedException();
    }

    public void DeleteMe()
    {
        throw new NotImplementedException();
    }
}

interface IAmAnEntity

public interface IAmAnEntity
{
    void PromptNewName();
    void PromptNewID();
    void CreateMe();
    void ReadMe();
    void UpdateMe();
    void DeleteMe();
}
\$\endgroup\$

1 Answer 1

7
\$\begingroup\$

First off, thank you for the formatting and white space.

To answer your question, having the CRUD operations into one is not possible. Somewhere you have to have different logic to do the operations, which is the standard way of doing thing

Looking through your code, I see a few issues. Having your data access, U.I. and entities in one class violates the Single Responsibility Principle all over the place. Basically a class/method should do one thing, and do it well.

Your entities, data access and U.I. should be separated out into their own classes, maybe even their own libraries. When I read any class with the word 'helper' in it, I cringe, especially when its static. There is usually a better to write that code, and still follow SOLID.

I would change it something more like this:

class DataAccess
{
     public Man CreateMan(string name, ...)
     {
          // Logic required to create a man
     }

     public bool UpdateMan(Man man)
     {
          // Logic required to update
     }

     public bool DeleteMan(Man man)
     {
          // Logic required to delete
     }

     public Man GetMan(int id)
     {
          // Logic required to retrieve
     }
}

public class UserInterface
{
     public T GetInput<T>(string prompt)
     {

     }

     public T InsistOnValidInput<T>(string prompt, TryParse<T> validator)
     {
     }
}

public static class Program
{
     private DataAccess _dataAccess;
     private UserInterface _userInterface;

     public int Main()
     {
          _dataAccess = new DataAccess();
          _userInterface = new UserInterface();

          var input = string.Empty;
          while(input != "quit")
          {
              input = _userInterface.GetInput<string>("Enter operation:");
              ProcessInput(input);
          }
     }

     public void ProcessInput(string input)
     {
          switch(input)
          {
               case "new Person":
                   CreatePerson();
                   break;
               case "delete person":
                   DeletePerson()
                   break;
               // other operations
           }                          
     }

     private CreatePerson()
     {
         var firstName = _userInterface .InsistOnValidInput<string>("Enter first name: ", i => i.Trim());
         var age = _userInterface .InsistOnValidInput<int>("Enter age: ", int.TryParse);

         var person = _dataAcess.CreatePerson(firstName, age, ...);
     }

     // More operations
}

This code will decouple your logic, so in the future if you wish to change how the data is stored, you just change the DataAccess code. If you want to make it a windows app, you replace the UserInterface class. In fact, I would create a interfaces and inject concrete classes where they are required. But this is a discussion for another time.

A couple more comments:

  • Lose the regions. Regions just add unnecessary and confusing code. If you need to wrap code in a region, there is a good chance that code can be pulled out into its own class.
  • Most of the comments add nothing to the code. // Execute the query, and change the column values is fairly obvious to anybody that can read that code. Comments should be used sparingly to justify why you did something, explain the reasoning behind what looks like complex code, or to clarify something. i.e. // Bug in X library so this has to be done first
    • Most of the places you've created static methods, static is being used incorrectly. You have basically create a procedural application, rather than an object oriented application.

Just a quick note: this is done in notepad so the code might not be 100% accurate, but hopefully it portrays the general ideas.

Good luck.

\$\endgroup\$
3
  • \$\begingroup\$ As you suspected, this code won't compile because of a couple of typos (i => i.Trim is missing the () for instance) and misunderstandings (InsistOnValidInput expects a delegate bool TryParse<T>(string input, out T output), so int.Parse will not match it, however int.TryParse will work). It is otherwise a very helpful review and I'll upvote it as soon as those issues are fixed ;) \$\endgroup\$
    – Adam
    Commented Nov 4, 2012 at 16:16
  • \$\begingroup\$ thank you this is helpful. if dataaccess and ui do user interface stuff, what should the main program maintain responsibility for? \$\endgroup\$
    – Matt Rohde
    Commented Nov 5, 2012 at 15:55
  • \$\begingroup\$ Your main program can be the user interface. I'm just suggesting having the domain objects and data access in other libraries. \$\endgroup\$ Commented Nov 5, 2012 at 16:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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