Static Data Helper for an Oracle Backend

What I would like to do is have a method call like:

Database.Modify.InsertOrUpdate(address,
() =>
{
    var result = ctx.ADDRESS.Where(o => o.JURIDICAL_ADDRESS == "Y")
        .OrderByDescending(o => o.ID)
        .FirstOrDefault();
    return (result != null ? result.ID : 0);
},
() => ctx.ADDRESS.Add(address),
() => address.ID = resultId.Value);

and generic method like:

public static bool InsertOrUpdate(ADDRESS address, Func exist, Action insert, Action update)
{
    decimal? resultId = null;
    Helper.Console.Try(()=> resultId = exist.Invoke(),
        string.Format("Finding ({0}) information ...", table.GetType()),
        string.Format("Finding ({0}) information done", table.GetType()),
        string.Format("Finding ({0}) information failed", table.GetType())
    );
    if( resultId == null || resultId == 0 )
    {
        return Helper.Console.Try(insert,
            string.Format("Adding ({0}) information in local context ...", table.GetType()),
            string.Format("Adding ({0}) information in local context done", table.GetType()),
            string.Format("Adding ({0}) information in local context failed", table.GetType())
        );
    }
    return Helper.Console.Try(update,
        string.Format("Updateing ({0}) information in local context ...", table.GetType()),
        string.Format("Updateing ({0}) information in local context done", table.GetType()),
        string.Format("Updateing ({0}) information in local context failed", table.GetType())
    );
}

At the moment I have :

```
public static bool InsertOrUpdate(ADDRESS address, Entities ctx, KnowledgeBase knowledgeBase)
{
decimal? resultId = null;
Helper.Console.Try(() =>
{
var result = ctx.ADDRESS.Where(o => o.JURIDICAL_ADDRESS == "Y")
.OrderByDescending(o => o.ID)
.FirstOrDefault();
resultId = (result != null ? result.ID : 0);
},
"Finding (ADDRESS) information ...",
"Finding

The code itself is generally pretty clean looking, but I think it suffers in a few areas:

• InsertOrUpdate screams: "one method that is concerned with two distinct things". I see InsertOrUpdate as an anti-pattern.

• Coupling between Console/Output writing and persistence logic is too tight.

• The use of Func/Action seems unnecessary and overly complex. Their use doesn't really constrain the use of this method to the intended purpose.

• Catching all exceptions at this depth in the code is probably not a good idea.

On #1: I see InsertOrUpdate type methods a lot. It seems straightforward enough to combine them now, but as the logic grows, you will eventually wish you didn't have to check a whole bunch of conditions to see which operation you actually want to do. You can already see this seeping into your code by the fact that you have an exist lambda to give a place for this additional logic.

The problem is usually with code further up the call stack. What is your application doing that you don't already know if this is an insert or update operation? This will lead to additional ambiguity elsewhere in your code when other developers aren't sure which is supposed to be happening in certain cases. What if I have a method that I know is only capable of an insert? Now I'm forced to use a method called InsertOrUpdate when I only need Insert (and its dangerous to open this method up to Update operations.

On #2: This is similar to #1 in that you have a method that is responsible for multiple things (persistence and writing to a console). What if I want to use this method but the Console is not available? What if I want to write to a different log? Any changes to the console writing have an impact on persistence logic, which is not desirable. If you refactor this to separate these concerns into their own methods, you will be able to change them independently.

On #3:

Database.Modify.InsertOrUpdate(address,
() => return -200D;
() => MakeAPie(),
() => HireAClown());

See what I did there? This method is overly generic. Is the exist Func ever going to compare on anything else than id? Entity Framework relies on unique keys for all Entities, so you could just use that framework and take the check out of the callers hands entirely.

On #4: By catching all exceptions, you are not letting callers properly handle certain cases (or any case really). It's taking away their ability to listen for certain exceptions and handle them. Instead, they just get a general "something bad happened, but I can't really tell what it was or if it was even related to the Insert or Update calls.

Edit - In response to your comments:

On #2: I'm not surprised that you can swap out the implementation easily, but writing functions that have a single responsibility is also a good practice for a number of other reasons, such as:

• Easier to understand. Looking at your method signature, a developer would have no clues about the fact that it even does any logging.

• Easier to test. When testing a method with a well defined single purpose, it's easier to test for the expected result. Baking in this additional functionality makes an external call that doesn't have to do with the method, and now we have to test that functionality as well. We also can't supply a test logger in this case.

• Single location to diagnose problems or make changes. Decided not to use the Helper.Console class in favor of something else? Well now you have to change your persistence code when there really wasn't a need to. Save method fails? New developer doesn't have to look at Console writing code and can easily get to the meat of the problem (Helper.Console can throw its own exceptions too!). etc. etc.




On #4: There's a difference between DbSaveException (made that up) and an OutOfMemoryException. You've removed the applications ability to make the distinction here. Perhaps you would stop processing records if you couldn't connect to the db at all? Or maybe fail gracefully in the face of a full output log. EF tends to throw exceptions of only a few types related to db saving. At any rate, you could move this exception logging a step up in the stack and have it no longer be a direct concern of the Insert and Update` methods.

I will note that a lot of these suggestions can be taken to extremes. If you're writing a throwaway application, or one that is so small it will take another developer a minute or so to get acclimated to your design choices, then some of this can be chalked up to YAGNI (extensive exception handling of edge cases, for example). There is a benefit to developing with these things in mind though, because in reality applications are changed frequently and get maintained by a number of different people. Some of these have a low cost of implementation so it's often a no-brainer to keep things loosely coupled and concise. In the end it takes experience to decide when you're going to be wasting time by doing

StackExchange Code Review Q#54381, answer score: 10