Exporting documents from PostgreSQL into a second database

Question

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;
}

• t3chb0t: If you know what the issues are, why don't you try to fix them yourself? Expecially 1, 2 and 3.
• Spedo De La Rossa: Because it doesn't mean that it is bad? I don't know if long code is bad or if mixing two databases is bad or bad practice. I hope you know what i mean.
• t3chb0t: I guess it's because of the word problem, it seems to be ambigous here but now I know what you mean.
• 404: Don't call something arrayIds when it's not an array.

Answer 1

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.

• t3chb0t: Don't use AddWithValue() - Without AddWithValue the code becomes exponentialy more complex and is barely usable if any of the parameters is to be specified by the user. I've never had any problems using this method. The article should be more specific about the dangers because it's a very useful API that by no means should be banned.
• BCdotWEB: @t3chb0t Why would it become more complex? The alternatives offered at the end of the article are still one-liners; the only additional complexity is that you need to properly define the type of data.
• t3chb0t: They only look like one-liners. Imagine an API where the user can freely specify his own criteria for a where-clause with values resolved at runtime. With AddWithValue it's a piece of cake. Without it, there were a couple of unnecessary parameters he'd need to think of or define them in a coniguration that also becomes a lot more complex. No thanks :-] I don't understand why should anyone avoid this great API only because some guy found a couple of edge cases that virtually never occur.
• Vogel612: @t3chb0t with AddWithValue you still need to either properly define a C# runtime type for the user-entered data (and then mapping to a SQL Data Type is basically trivial) or you have it all as strings (which you can also define without any problems). I'm pretty sure that the scenario you describe is solved more elegantly without AddWithValue....