Save multiple group permissions in one go

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 SaveGroupStepPermissions(string connectionString, int companyID, List groups, List gsp)
{
var result = new List();
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)

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{ insertEventItem, updateEvenItem };

StackExchange Code Review Q#117228, answer score: 2