6
\$\begingroup\$

This snippet works and does as desired, but I just wonder if I am being too repetitive.

What I mean by this is, eventually I will have all 12 months in there. So.... 12 Update Queries, 12 Commands, 12 Parameters, 12 values, and 12 executions. All doing similar but slightly different things.

        protected void addshopbaseyear(object sender, EventArgs e)
       {        
        string dateoneJan = "UPDATE tblshopstatus Set dateone = (SELECT CAST(date_trunc('year', now())as date)) WHERE areaid = (SELECT id FROM tblarea WHERE areaname = @shop) AND monthid = '1'";
        string dateoneFeb = "UPDATE tblshopstatus Set dateone = (SELECT CAST(date_trunc('year', now())as date) + interval '1 month') WHERE areaid = (SELECT id FROM tblarea WHERE areaname = @shop) AND monthid = '2'";

        NpgsqlCommand dateonecmdJan = new NpgsqlCommand(dateoneJan, con);
        NpgsqlCommand dateonecmdFeb = new NpgsqlCommand(dateoneFeb, con);

        dateonecmdJan.Parameters.Add ("@shop", NpgsqlDbType.Char);
        dateonecmdFeb.Parameters.Add ("@shop", NpgsqlDbType.Char);

        dateonecmdJan.Parameters["@shop"].Value = txtNameofBranch.Text;
        dateonecmdFeb.Parameters["@shop"].Value = txtNameofBranch.Text;

        con.Open();
        dateonecmdJan.ExecuteNonQuery();
        dateonecmdFeb.ExecuteNonQuery();
        con.Close();
     }

What does this do? - The purpose of this code is to update a table that is used by a Gridview on one of my .aspx pages. The user clicks a Save button which calls this void method and the Gridview is updated.

The purpose of the Gridview The Gridiview is used for shop scheduling. For example setting Open/Close days for a shop.

So in the end three tables are used for the GridView as follows

  • tblshopstatus - stores dates, monthid, and areaid

  • tblarea - stores the shop names

  • tblmonths - stored the month names

The tblshopstatus has 33 columns 31 to cover the days of the months, monthid, and areaid.

So in the end to fill the grid I combine the three tables into a view in SQL to get the the final grid data.

A screen of that is here:

Screen of Gridview _

As you can see it is colored red meaning holidays closed and grey meaning regular days closed.

In the case of the void in my question, I already have 12 rows in the table tblshopstatus,(this is populated by a different void at a different time) with the areaid and monthid defined. All I need to do now is populate the other 31 columns with dates so I can display the day of the week as shown in the image.

For example my date columns in tblshopstatus are named like this

  • dateone
  • datetwo
  • datethree and so on....until datethirtyone

For any given area I would have just 12 rows a record for each month, Back to the void in question, here you see I run an UPDATE for Jan in dateone and then do the same thing again for Feb in dateone. I am using a SQL function to increase the value by one month etc... it works.... or it does what I want, it puts 1/1/2018, in dateone on the Jan row, and 2/1/2018 in dateone on the Feb row when the button is clicked, no errors are thrown and the data is updated this verifies to me that the void is working. . .

Why isn't it complete yet At this point I stopped and took a step back. Even though it works that is a lot of code, I will need to do the same thing for 31 columns and for each month for each column.

What is con? con is a reference to my database connection which is public. I don't want to have to type that all the time so in my class file I put directly at the top under public partial like this:

public partial class operations : System.Web.UI.Page
{
    //Database Connection
    NpgsqlConnection con = new NpgsqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings["bsmanager"].ConnectionString);

}

It seems to work I never have a problem connecting to my database

NpgsqlCommand comes from this name space

using Npgsql;

Conclusion: Based on the comments and the answers I supplied above more specifically what I need to know is how can I take the current void in question and use loops/arrays. I was looking for a better way to do the same thing with out all the long code. I don't have much experience with loops and arrays, which prompted this question.

\$\endgroup\$
  • 4
    \$\begingroup\$ Who knows... maybe you could first explain what it does and why it isn't complete yet? Btw, have you heard about loops and arrays? \$\endgroup\$ – t3chb0t Jan 10 '18 at 13:45
  • 2
    \$\begingroup\$ What exactly is the snippet supposed to do? How are you verifying that it works? What is con? From what namespace is NpgsqlCommand? \$\endgroup\$ – Snowbody Jan 10 '18 at 14:25
  • \$\begingroup\$ Most DBMSes support an Update using a Join or From, thus you should run a single to Update all days/months at once. CAST(date_trunc('month', now()) returns the first of a month and then you just have to cut&paste&modify to add the days in month. \$\endgroup\$ – dnoeth Jan 11 '18 at 7:37
  • 1
    \$\begingroup\$ + interval '1 month' does not look like valid syntax to me \$\endgroup\$ – paparazzo Jan 11 '18 at 18:50
  • 2
    \$\begingroup\$ @Paparazzi - it is PostgreSQL \$\endgroup\$ – Tommy Jan 11 '18 at 23:52
4
\$\begingroup\$

There are two main problems with this:

Firstly, as you've identified, your approach will involve a lot of repeated code. The only thing that changes for each of your SQL commands is the monthid field and the interval. So you can easily loop through the numbers 1-12, and use that number for both changes.

for (var i = 1; i <= 12; i++)
{
    var cmdString = $"UPDATE tblshopstatus Set dateone = (SELECT CAST(date_trunc('year', now())as date) + interval '{i - 1} months') WHERE areaid = (SELECT id FROM tblarea WHERE areaname = @shop) AND monthid = '{i}'";

The second problem is that, if your query execution throws an exception, you're potentially not going to close the SQL connection.

It might be better to refactor the class completely so you can make use of the connection's IDisposable implementation, but you can also just use a try...finally to do the same thing.

protected void addshopbaseyear(object sender, EventArgs e)
{
    con.Open();
    try
    {
        for (var i = 1; i <= 12; i++)
        {
            var cmdString =
                $"UPDATE tblshopstatus Set dateone = (" +
                $"SELECT CAST(date_trunc('year', now())as date) + interval '{i - 1} months') " +
                $"WHERE areaid = (SELECT id FROM tblarea WHERE areaname = @shop) AND monthid = '{i}'";
            var cmd = new NpgsqlCommand(cmdString, con);
            cmd.Parameters.Add("@shop", NpgsqlDbType.Char);
            cmd.Parameters["@shop"].Value = "foo";
            cmd.ExecuteNonQuery();
        }
    }
    finally
    {
        con.Close();
    }
}
\$\endgroup\$
3
\$\begingroup\$

What is con? con is a reference to my database connection which is public.

I have to disappoint you beceause it is not public. Class members are private by default.

Having con as a field is a very bad idea. If you ever decide to make your application run queries in parallel they will close each other's connections so they should not share it.

It's always peferable to make everything as much function local as possible if you don't want to debug tricky bugs.

\$\endgroup\$
  • \$\begingroup\$ Yes you are correct. But, when I say public I am not meaning in terms of sharing across classes just within the current class. For instance in the current class I am really only working with one database. So, rather than write a con for each and every void I just reference it. I have never ran into an issue using it like this as long as I close the con when I am done with it in the void. Are you saying that is incorrect? Or maybe I should reword it in my question to make my intent more clear, I suppose. \$\endgroup\$ – Tommy Jan 13 '18 at 3:56
  • \$\begingroup\$ @Tommy programming is a very precise world. If you say pulic it means public so if you mean private then say also private, everyone who knows access modifiers will instantly know what you mean without additional explanation. Otherwise you will have to explain yourself each time. It doesn't matter with how many databases you work. It's a bad habit to share resources like the connection the way you do it. I'm just saying. At some point a day will come when you will be chasing a bug that could have easily be avoided :-] \$\endgroup\$ – t3chb0t Jan 13 '18 at 8:35
  • \$\begingroup\$ So what you are saying is write a separate connection for every void where I need to work with the database. No matter. Just trying to understand your advice. \$\endgroup\$ – Tommy Jan 13 '18 at 9:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.