trjhtr

This is my first try with a database. I read some articles/books and now i tried it myself and I want to know if I can write it better or what are my mistakes. All in all I am using try/catch/finally, making more statements in one method, more DBs and have the Connection in a private method. I am not using functions on the DB side, because this will come in the future.

Problems:

1. It is pretty long > many statements


2. Mixing 2 databases in one method


3. I don't know where to use try/catch


4. For example setting the connection and for getting the next ID I wrote an other method


5. Is it good to declare the variables at the beginning? Declare it when I am using it?

public void ExportDocuments(List<string> arrayIds)

 var getNpgsqlConnection = SetNpgsqlConnection();
 var getMRDGlrConnection = SetMRDGlrConnection();
 int f_id = 0;
 int period = 0;
 int mrnumber = 0;
 bool mrCreated = true;
 bool f_exportcheckbox;
 string f_name = String.Empty;
 string f_subject = String.Empty;
 string f_agendanumber = String.Empty;
 string foldername = String.Empty;
 DateTime mrdate = DateTime.Now;
 Dictionary<int, string> ressortList = new Dictionary<int, string>();

 try
 
 int getNextMRId = GetNextMRId(); //gets the last ID+1 from DB
 foreach (var item in arrayIds)
 
 using (var cmd = new NpgsqlCommand())
 
 cmd.Connection = getNpgsqlConnection ;
 cmd.CommandText = "SELECT f_id, f_name, f_subject, f_agendanumber, f_exportcheckbox, period, mrnumber, foldername, mrdate FROM mrd_folder.files INNER JOIN mrd_folder.folders ON files.f_folders_id = folders.id WHERE f_id = @arrayId;";
 cmd.Parameters.AddWithValue("arrayId", Convert.ToInt32(item));

 using (var reader = cmd.ExecuteReader())
 
 while (reader.Read())
 
 try
 
 f_id = (int)reader["f_id"];
 f_name = reader["f_name"].ToString();
 f_subject = reader["f_subject"].ToString();
 f_agendanumber = reader["f_agendanumber"].ToString();
 f_exportcheckbox = (bool)reader["f_exportcheckbox"];
 period = (int)reader["period"];
 mrnumber = 1002;// (int)reader["mrnumber"];
 mrdate = (DateTime)reader["mrdate"];
 
 catch (Exception e)
 
 Debug.WriteLine(e.Message);
 throw;
 
 
 
 
 using (var getRessorts = new NpgsqlCommand())
 

 getRessorts.Connection = getNpgsqlConnection ;
 getRessorts.CommandText = "SELECT ressortsbeantragung.r_id, ressorts.r_abbrevention FROM mrd_folder.ressortsbeantragung INNER JOIN mrd_folder.ressorts ON ressortsbeantragung.r_id = ressorts.r_id WHERE ressortsbeantragung.f_id = @arrayId";
 getRessorts.Parameters.AddWithValue("arrayId", Convert.ToInt32(item));
 using (var readRessort = getRessorts.ExecuteReader())
 
 while (readRessort.Read())
 
 try
 
 ressortList.Add((int)readRessort["r_id"], readRessort["r_abbrevention"].ToString());
 
 catch (Exception e)
 
 Debug.WriteLine(e.Message);
 throw;
 
 
 
 

 //Import to MRO
 try
 
 using (var cmd = new NpgsqlCommand())
 
 cmd.Connection = getMRDGlrConnection ;
 if (mrCreated)
 
 string currentDate = DateTime.Now.GetDateTimeFormats()[5];

 cmd.CommandText = "SELECT insertmrsitzung(@period, @mrnumber, @mrdate)";
 cmd.Parameters.AddWithValue("period", period);
 cmd.Parameters.AddWithValue("mrnumber", mrnumber.ToString());


 cmd.Parameters.Add("mrdate", NpgsqlTypes.NpgsqlDbType.Date);
 cmd.Parameters[2].Value = mrdate;

 cmd.ExecuteNonQuery();
 mrCreated = false;
 

 cmd.CommandText = String.Empty;
 cmd.Parameters.Clear();

 cmd.CommandText = "SELECT inserttopunkt(@f_id, @f_agendanumber, @f_subject, '')";
 cmd.Parameters.AddWithValue("f_id", getNextMRId);
 cmd.Parameters.AddWithValue("f_agendanumber", f_agendanumber);
 cmd.Parameters.AddWithValue("f_subject", f_subject);

 cmd.ExecuteNonQuery();

 foreach (var res in ressortList)
 
 cmd.CommandText = String.Empty;
 cmd.Parameters.Clear();
 if (res.Value == "BKA")
 
 string rename = "BK";
 //cmd.Parameters.AddWithValue("r_kurz", rename);
 cmd.CommandText = @"SELECT inserttopunktressort(@f_id, @f_agendanumber, '" + rename + "')";
 
 else
 cmd.CommandText = @"SELECT inserttopunktressort(@f_id, @f_agendanumber, '" + res.Value + "')";

 cmd.Parameters.AddWithValue("f_id", getNextMRId);
 cmd.Parameters.AddWithValue("f_agendanumber", f_agendanumber);


 cmd.ExecuteNonQuery();
 
 
 
 catch (Exception e)
 
 Debug.WriteLine(e.Message);
 throw;
 
 ressortList.Clear();
 
 
 catch (Exception e)
 
 Debug.WriteLine(e.Message);
 throw;
 
 finally
 
 getNpgsqlConnection.Close();
 getMRDGlrConnection.Close();
 


private NpgsqlConnection SetNpgsqlConnection()


 var setConnectionString = ConfigurationManager.ConnectionStrings["Test1"];
 string getConnectionString = setConnectionString.ConnectionString;

 var npgsqlConnection = new NpgsqlConnection();
 npgsqlConnection.ConnectionString = getConnectionString;
 try
 
 npgsqlConnection.Open();
 
 catch (Exception e)
 
 Debug.WriteLine(e.Message);
 throw;
 

 return npgsqlConnection;

Don't have methods like SetNpgsqlConnection(). DB Connections should be properly disposed of, and thus they should be used inside a using block, e.g.

using (SqlConnection con = new SqlConnection(connectionString))

 // do stuff

There is no point in keeping your connections alive as long as you do. Typically db connections should be kept as short as possible.

Give variables etc. proper names.

• 
  arrayIds is almost Hungarian notation (which is discouraged). Moreover, it isn't even an array! Why not simply name it "ids"?


• 
  f_id, f_exportcheckbox: don't use underscores etc. in the middle of names. Names should be camelCase or PascalCase. The only place I expect an underscore in a variable name is at the start of a class-wide variable.


• 
  mrnumber, mrCreated: I have no idea what "mr" refers to. Don't use abbreviations unless they're well-known.

Your method starts of with a dozen "definitions": that's usually a bad sign. In this case it's a bunch of data that seems to belong together: so put them in a class of their own and pass that class to other methods.

Avoid writing ADO.NET by hand; instead use an ORM like Dapper or Entity Framework and work with classes.

Don't use AddWithValue().

Your method is 140+ lines long. Even when you apply the above, it still is doing multiple things. Split it up into smaller methods, e.g

• one that retrieves the data from mrd_folder.files,


• another that gets the data from mrd_folder.ressortsbeantragung,


• a third one to execute insertmrsitzung,


• a fourth one to execute inserttopunkt,


• a fifth one to execute inserttopunktressort.

And please, do not keep your connection open for those last three: make sure each method manages its own db connection and trust the framework to allocate resources effectively.