Returning latest database ID

Question

I want my method to use a finally block so I can be ure that my EF entity is disposed after usage, even if an exception is thrown, but I would also like to not handle exceptions in that method.

1. The current code works but is it possible to omit the catch{} block? Would an exception from the try{} block be re-thrown or would it be forgotten?

2. Is it necessary to set tm_entity to null in the finally block, or will Dispose take care of everything?

Any other suggestions are welcome!

/// <summary>
/// Returns ID of the latest result database.
/// </summary>
/// <param name="managementEntity">EF Test Management entity.</param>
/// <returns>Latest result database ID.</returns>
public static int GetLatestDatabaseIteration(TEST_MANAGEMENTEntities managementEntity)
 {
 if (managementEntity == null)
 {
 throw new ArgumentNullException("Passed entity can not be null!.");
 }
 TEST_MANAGEMENTEntities tm_entity = managementEntity;
 int? latestDatabaseID = null;
 try
 {
 latestDatabaseID = (from trb in tm_entity.TestRunsDatabases
 select trb.DatabaseID).Max();
 if (latestDatabaseID == null)
 {
 throw new Exception("Cannot evaluate latest Test Result Database ID.");
 }
 }
 catch (Exception ex) //Could I just omit this block?
 {
 throw ex;
 }
 finally
 {
 tm_entity.Dispose();
 tm_entity = null; //is it necessary?
 }
 return (int)latestDatabaseID;
 }

EDIT:

After making suggested changes my method looks like this.

public static int GetLatestDatabaseIteration(TEST_MANAGEMENTEntities managementEntity)
 {
 if (managementEntity == null)
 {
 throw new ArgumentNullException("Passed entity can not be null!.");
 }
 TEST_MANAGEMENTEntities tmEntity = managementEntity;
 int? latestDatabaseID = (from trb in tmEntity.TestRunsDatabases
 select (int?)trb.DatabaseID).Max();
 if (latestDatabaseID == null)
 {
 throw new NoRecordsFoundException("Cannot evaluate latest Test Result Database ID.");
 }
 return (int)latestDatabaseID;
 }

Changes I made:

1. I have renamed variables to match CamelCase.
2. I have removed .Dispose on a passed reference as it would also Dispose object in the Caller method.
3. I have abandoned calling generic exception and replaced it with custom one that more accurately describes this exception.

Calling .Max() if query return null may cause problems, but I will check it with UnitTests soon.

Answer 1

You really shouldn't rethrow caught exceptions like that.

catch (Exception ex) //Could I just omit this block?
 {
 throw ex;
 }

Erases the call stack of the exception, instead use

catch (Exception) //Could I just omit this block?
 {
 throw;
 }

However the point is moot, because the try-finally block is syntactically legal, but you should bear in mind a very major caveat (emphasis my own):

Within a handled exception, the associated finally block is guaranteed to be run. However, if the exception is unhandled, execution of the finally block is dependent on how the exception unwind operation is triggered. That, in turn, is dependent on how your computer is set up.

As such, it would be a good idea to make sure you catch any exceptions you can reasonably expect, but then that's just standard good practice.

As an aside, I really don't like your use of caps. Type names should be PascalCase, and you should avoid underscores in type names or variable names.

Lastly, you really should throw a subclass of Exception, and never the parent class.

• Rafał.S: I have mixed throw and throw ex all together. I have thought that throw ex will retain same stack but I was wrong. Thanks for pointing that out. Same goes for other things you pointed out, I must have missed this "tm_entity" somehow as I usually enforce CamelCase in my codebase. I'll post changed code soon ;)

Answer 2

By calling GetLatestDatabaseIteration(TEST_MANAGEMENTEntities managementEntity) you are not passing an object of type TEST_MANAGEMENTEntities to the method but a reference of that object to the method.

See: https://stackoverflow.com/a/8708674/2655508

Objects aren't passed at all. By default, the argument is evaluated and its value is passed, by value, as the initial value of the parameter of the method you're calling. Now the important point is that the value is a reference for reference types - a way of getting to an object (or null). Changes to that object will be visible from the caller. However, changing the value of the parameter to refer to a different object will not be visible when you're using pass by value, which is the default for all types.

If you want to use pass-by-reference, you must use out or ref, whether the parameter type is a value type or a reference type. In that case, effectively the variable itself is passed by reference, so the parameter uses the same storage location as the argument - and changes to the parameter itself are seen by the caller.

So by calling tm_entity.Dispose(); you are basically disposing the original object.

private static void foo()
{
 TEST_MANAGEMENTEntities theEntity = new TEST_MANAGEMENTEntities();
 int latestID = GetLatestDatabaseIteration(theEntity);
 // now theEntity is already disposed
}

If this is what you wanted to do, then it isn't the right place. A consumer/caller of this method would not expect that the passed in "object" whould be disposed after returning.

If you are working with IDisposable objects you should use using blocks.

Naming

In addition to Nick Udell's answer please see the naming guidelines .

• Rafał.S: I wanted to keep object that was passed alive outside of the method so thanks for pointing that out! Yup I have noticed the error of using underscore and I do know that I should use using block for disposable items. I'll rethink my design of this class and post it later. Thanks for input once again!