Save multiple group permissions in one go

Question

The method SaveGroupStepPermissions(...) is a database API class. If an error occurs within the method the exception is trapped. Whether this is good design or not is not the scope of this question as I need to adhere to product guidelines.

I feel that this method is to complex and too lenghty. The obvious solution is to break out code into new functions, but I'm not sure how to do that in a way that is logical.

Important! If an error occurs the method should stop processing and return. That means both foreach loops must be exited.

Note: Input validation is performed in InsertGroupStepPermission() and UpdateGroupStepPermission().

Feel free to suggest improvements to code and comments beside the main question!

public static List<EventItem> SaveGroupStepPermissions(string connectionString, int companyID, List<Group> groups, List<GroupStepPermissions> gsp)
{
 var result = new List<EventItem>();
 result.Add(new EventItem("Insert Step permissions"));
 result.Add(new EventItem("Update Step permissions"));
 try
 {
 using (SqlConnection conn = new SqlConnection(connectionString))
 {
 conn.Open();
 using (SqlTransaction tran = conn.BeginTransaction("ProductName.GSP.cid" + companyID.ToString()))
 {
 foreach (Group group in groups)
 {
 foreach (GroupStepPermissions stepPermission in gsp)
 {
 if (stepPermission.RowID == 0) //new permission that does not exist in the database. (INSERT)
 {
 try
 {
 InsertGroupStepPermission(conn, tran, stepPermission, companyID, group.ID);
 result[0].SubEvents.Add(new EventSubItem("Added rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'", StatusEnum.OK, "Insert OK", stepPermission));
 }
 catch (Exception x)
 {
 result[0].SubEvents.Add(new EventSubItem("Failed to add rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'", StatusEnum.Failed, x.Message, stepPermission));
 throw;
 }
 }
 else //Existing stepPermission. (UPDATE)
 {
 try
 {
 UpdateGroupStepPermission(conn, tran, stepPermission, companyID, group.ID);
 result[1].SubEvents.Add(new EventSubItem("Updated rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'", StatusEnum.OK, "Update OK", stepPermission));
 }
 catch (Exception x)
 {
 result[1].SubEvents.Add(new EventSubItem("Failed to update rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'", StatusEnum.Failed, x.Message, stepPermission));
 throw;
 }
 }
 }
 } //End outer for
 tran.Commit(); //important :)
 }
 }
 }
 catch(Exception x)
 {
 var eItem = new EventItem("Failed to save step permissions");
 eItem.SubEvents.Add(new EventSubItem("Error: " + x.Message));
 result.Add(eItem);
 }
 return result;
}

• RobH: You have a typo in your catch: x.message should be x.Message. There's also nothing as upsetting as only having an exception message when debugging - don't throw away my stack trace!
• David: @RobH: Fixed the typo. Good point about the exception, will include it in the EventSubItem that is displayed in the application (targeted towards technical support staff)

Answer 1

There's no need for fairly meaningless variable names like gsp, conn, tran, eItem,...

And result is equally bad, since it doesn't tell me what it contains.

Exception x is... unexpected. ex or exc is usually used.

---

You use var, but not consistently. Why SqlConnection conn, Group group, etc.?

---

I'm sure the .ToString() isn't necessary in "ProductName.GSP.cid" + companyID.ToString() -- unless you're coding to an old version of .NET.

---

Comments shouldn't tell me what is happening, but why. //End outer for and //important :) don't tell me anything useful.

Meanwhile //new permission that does not exist in the database. (INSERT) tells me that if (stepPermission.RowID == 0) isn't perhaps the best way to check if you need to do an insert or an update.

---

An enum shouldn't be called StatusEnum. Why not simply Status?

---

GroupStepPermissions is a class name, yet it seems to represent a single item. That's a big no-no.

---

Avoid concatenation:

"Added rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'"

This is why string.Format() was invented.

---

Now, let's look at the whole method. You have four parameters, which is bordering on getting hard to maintain. Consider a class that contains companyID, groups and gsp. Note that I leave connectionString out of it; quite frankly I find it odd to see you pass a connectionstring to a method.

There's a lot of semi-duplicate logic, especially in catch (Exception x) in the insert or update. Move that to a method and call it with the required parameters.

I'd also be inclined to move everything inside foreach (GroupStepPermissions stepPermission in gsp) to a separate method, if only to reduce all those indentations.

I don't see the point in doing this:

result.Add(new EventItem("Insert Step permissions"));
result.Add(new EventItem("Update Step permissions"));

...if that results in having to do this:

result[0].SubEvents.Add

Name one of them insertEventItem and the other updateEvenItem and only compile the result list at the end, e.g.

return new List<EventItem>{ insertEventItem, updateEvenItem };

Answer 2

Method signature

You should code against the lowest common type or much better against interfaces. Right now you don't use any methods of the List<T> class which by the way is a implementation detail which shouldn't be exposed.

Using a ICollection<T> would be a good fit for the return type, for the method parameters I would like to suggest using IEnumerable<T>. The name gsp is not well named you should consider to rename it to permissions.

This would lead to

public static ICollection<EventItem> SaveGroupStepPermissions(string connectionString, int companyID, IEnumerable<Group> groups, IEnumerable<GroupStepPermissions> permissions)
{
} 

---

You are adding two EventItem objects to the result which you are later accessing by using the indexer, which can be done but this introduces two magic numbers namely 0 and 1. This can be avoided if you assign the EventItem's to well named variables like so

var insertItem = new EventItem("Insert Step permissions");
result.Add(insertItem);
var updateItem = new EventItem("Update Step permissions"); 
result.Add(updateItem); 

which is more clear. You should maybe adjust the variable names to fit your needs.

After we have added this EventItem's we should now check if we really can and need to create a connection to the database. Validating methods parameter at least wether they are null should always be done if the method in question is public accessible.

Usually this would look like so

if (connectionString == null) { throw new ArgumentNullException(nameof(connectionString)); }
if (string.IsNullOrWhiteSpace(connectionString)) { throw new ArgumentException($"{nameof(connectionString)} may not be empty.", nameof(connectionString)); } 

next we need to check if the passed in IEnumerable<T>'s are null using the same pattern.

If this checks are passed we know, that all arguments are valid. If there are special cases for the companyID like it have to be > 0 this check should be added as well (here an ArgumentOutOfRangeException would be the correct exception to throw).

For your special case method throwing exceptions isn't the way to go, so I would suggest having a private static IEnumerable<string> ValidateSaveGroupStepPermissionsParameters() method like so

private static IEnumerable<string> ValidateSaveGroupStepPermissionsParameters(string connectionString, int companyID, IEnumerable<Group> groups, IEnumerable<GroupStepPermissions> permissions)
{
 if (connectionString == null) { yield return $"Parameter is null: {nameof(connectionString)}"; }
 if (string.IsNullOrWhiteSpace(connectionString)) { yield return $"{nameof(connectionString)} may not be empty."; } 
 if (groups == null) { yield return $"Parameter is null: {nameof(groups)}"; }
 if (permissions == null) { yield return $"Parameter is null: {nameof(permissions)}"; } 
 // possible special case for `companyID` here
} 

which leads to

public static ICollection<EventItem> SaveGroupStepPermissions(string connectionString, int companyID, IEnumerable<Group> groups, IEnumerable<GroupStepPermissions> permissions)
{
 var insertItem = new EventItem("Insert Step permissions");
 result.Add(insertItem);
 var updateItem = new EventItem("Update Step permissions"); 
 result.Add(updateItem); 
 var validationResults = ValidateSaveGroupStepPermissionsParameters(connectionString, companyID, groups, permissions).ToList(); 
 if (validationResults.Count > 0) 
 {
 var exceptionItem = new EventItem("Failed to save step permissions");
 foreach (var msg in validationResults)
 {
 exceptionItem.SubEvents.Add(new EventSubItem($"Error: {msg}"));
 }
 result.Add(exceptionItem);
 return result;
 }
 // remaining code here
}

Can we now please connect to the database? Unfortunately no because we don't need to do this if either groups or permissions does not contain any items.

So let us early return in the case that there are no items like so

if (!groups.Any() || !permissions.Any())
{
 return result;
} 

Instead of the inner foreach loop which is having that if..else statement I would calculate two permission sets like so

var inserts = permissions.Where(p => p.RowID == 0).ToList();
var updates = permissions.Except(inserts).ToList(); 

and use two inner foreach loops like so

foreach (Group group in groups)
{
 var groupId = group.ID;
 var groupName = group.Name;
 foreach (var permission in inserts)
 {
 try
 {
 InsertGroupStepPermission(conn, tran, permission, companyID, groupID);
 insertItem.SubEvents.Add(new EventSubItem($"Added rights for '{groupName}' in step '{permission.StepTitle}'", StatusEnum.OK, "Insert OK", permission));
 }
 catch (Exception x)
 {
 insertItem.SubEvents.Add(new EventSubItem($"Failed to add rights for '{groupName}' in step '{permission.StepTitle}'", StatusEnum.Failed, x.Message, permission));
 throw;
 }
 }
 foreach (var permission in updates)
 {
 try
 {
 UpdateGroupStepPermission(conn, tran, permission, companyID, groupID);
 updateItem.SubEvents.Add(new EventSubItem($"Updated rights for '{groupName}' in step '{permission.StepTitle}'", StatusEnum.OK, "Update OK", permission));
 }
 catch (Exception x)
 {
 updateItem.SubEvents.Add(new EventSubItem($"Failed to update rights for '{groupName}' in step '{permission.StepTitle}'", StatusEnum.Failed, x.Message, permission));
 throw;
 }
 }
} 

The processing of these two foreach loops could now be extracted to separate methods.

• Johnbot: I have to disagree with changing groups and permissions to IEnumerable when you're doing more with them than just enumerating. I'm pretty sure every single method in the framework taking an IEnumerable only calls GetEnumerator once. Here you're calling GetEnumerator twice for groups and three times for permissions. This will less than optimal if someone down the line passes in an IQueryable.