[lsochoa]

Good day guys, I'm a semi-intermediate here in c#. Actually, the said topic name it won't work with my codes implemented, i need some help on how to improve my timer tick procedure code as shown what I've tried below.

private void timer1_Tick(object sender, EventArgs e)
        {
            statmessage();

            int i = listBox1.SelectedItems.Count + 1;

          if (i < 0)
            {
                var exp = (EItem)listBox1.Items[i];
                var timeVisible = DateTime.Now - exp.Added;
                if (timeVisible.TotalSeconds > 5)
                    if (counter == 100)
                    {
                   
                    SendMessage();
                    counter = counter - 1;
                    ++i;
                    }
                    else if (counter <= 0)
                    {

                    statmessage();
                    }
                
            }

        }

here is the program output:



Your feedback is highly appreciated and will help to improve the said concern, Thank you very much.

[Skydiver]

The very first thing to do to improve that code there is to use indentation consistently.

The next thing is to name your controls well. Don't settle for timer1 or listbox1. Give them better names.

The next thing after that is follow the .NET Framework naming conventions consistently. What is statmessage()? Why doesn't it have a good .NET Framework style name like SendMessage().

Next after those is to keep your UI, data model, and business logic separate from each other. In other words, don't use the UI as your data model. Don't embed your business logic within the various event handlers for te UI. That style of programming was used with Win32 API back in the 90's because of memory constraints and the belief that "having the right control will solve your problems". We've learned better since that time to keep our data model separate from our UI.

[lsochoa]

Thank you for good advice mate. Actually, the statmessage() procedure code purpose is if the main program creates a transaction, the said middle ware program adds an item list to the listbox and the selected list is send an email to a specific recipient.

By the way, here are my codes for the said procedure.

   #region EItem
        class EItem
        {
            private string text;
            public EItem(string text)
            {
                this.text = text;
                this.Added = DateTime.Now;
            }
            public DateTime Added { get; private set; }
            public override string ToString()
            {
                return text;
            }
        }

        #endregion

        #region status_message


        public void statmessage()
        {
            string connstr = "server = 192.168.128.8; database = ifern_uat; UID = joshua; password = joshua;";
            SqlConnection dbconn = new SqlConnection(connstr);
            string sqlstr = "select a.or_date, a.TransactionNo , a.email, a.bemail from vw_forEmailSending a where a.or_date between '2019-01-01 00:00:00.000' and '2500-12-31 00:00:00.000' order by or_date asc";
            SqlCommand cmd = new SqlCommand(sqlstr, dbconn);

            dbconn.Open();

            SqlDataReader rdr = cmd.ExecuteReader();
            listBox1.BeginUpdate();
            listBox1.Items.Clear();

            while (rdr.Read())
            {

                //listBox1.Items.Add(new EItem(rdr.GetSqlDateTime(0) + " - " + "Transaction No.: " + rdr.GetString(1) + " has been submitted successfully all the details at " + rdr.GetString(2) + "."));
                listBox1.Items.Add(new EItem("Transaction No.: " + rdr.GetString(1)));

                lblcount.Text = listBox1.Items.Count.ToString();
                listBox1.SelectedIndex = listBox1.Items.Count - 1;
                

                
            }

            rdr.Close();
            dbconn.Close();
            listBox1.EndUpdate();
            listBox1.Refresh();

        }


        #endregion

This post has been edited by Skydiver: 30 January 2019 - 08:35 PM
Reason for edit:: Removed unnecessary quote. No need to quote the post above yours.

[lsochoa]

Additional code for SendMessage() Procedure

        #region Emailtotop

        public void SendMessage()
        {
            string connstr = "server = ***.***.**.**; database = dbname; UID = ******; password = ******;";
            SqlConnection dbconn = new SqlConnection(connstr);
            string sqlstr = "select top(1) * from vw_forEmailSending a order by or_date desc";
            SqlCommand cmd = new SqlCommand(sqlstr, dbconn);
            dbconn.Open();
            SqlDataReader rdr = cmd.ExecuteReader();

            while (rdr.Read())
            {
                TransactionNo = rdr[0].ToString();
                retail_price = double.Parse(rdr[7].ToString());
                discount = double.Parse(rdr[8].ToString());
                gross = double.Parse(rdr[9].ToString());
                netofvat = double.Parse(rdr[13].ToString());
                vat = double.Parse(rdr[14].ToString());
                or_date = DateTime.Parse(rdr[2].ToString());
                ar_address = rdr[1].ToString();
                uid = rdr[16].ToString();
                SI = rdr[3].ToString();
                Teller = rdr[6].ToString();
                FreightCharge = double.Parse(rdr[10].ToString());
                emailaddress = rdr[5].ToString();
                delMode = rdr[4].ToString();
                CustName = rdr[15].ToString();
            }
            rdr.Close();
            dbconn.Close();

            emailMessage = "<table width = '100%' border = '0' cellspacing = '0' cellpadding = '0'>";
            emailMessage = emailMessage + "<tr>";
            emailMessage = emailMessage + "<td width = '80%' valign = 'top' ><p class='nodisplay'>Sold to : </p>&nbsp;&nbsp;&nbsp;&nbsp;<strong>" + CustName + "</strong></td>";

            emailMessage = emailMessage + "<td width = '20%' rowspan= '2' valign= 'top'>";
            emailMessage = emailMessage + "<table width= '100%' border= '0' cellspacing= '0' cellpadding= '5' class='smallheight'>";
            emailMessage = emailMessage + "<tr>";
            emailMessage = emailMessage + "<td><div>Date : </div>";
            emailMessage = emailMessage + "" + or_date + "</td>";
            emailMessage = emailMessage + "</tr>";
            emailMessage = emailMessage + "<tr>";
            emailMessage = emailMessage + "<td><div>Transaction No:</div>";
            emailMessage = emailMessage + "" + TransactionNo + "</td>";
            emailMessage = emailMessage + "</tr>";
            emailMessage = emailMessage + "<tr>";
            emailMessage = emailMessage + " <td><div>Teller:" + Teller + "</div>&nbsp;";
            emailMessage = emailMessage + " </td>";
            emailMessage = emailMessage + " </tr>";
            emailMessage = emailMessage + "<tr>";
            emailMessage = emailMessage + "<td><div>DR#</div><br /></td>";
            emailMessage = emailMessage + "</tr>";
            emailMessage = emailMessage + "</table>";
            emailMessage = emailMessage + "</td>";
            emailMessage = emailMessage + "</tr>";
            emailMessage = emailMessage + "<tr>";
            emailMessage = emailMessage + "<td><div>Address :</div>&nbsp;&nbsp;&nbsp;&nbsp;" + ar_address + "<br> Delivery Mode: " + delMode + "</td>";
            emailMessage = emailMessage + "</tr>";
            emailMessage = emailMessage + "<tr>";
            emailMessage = emailMessage + "<td><div>Email :</div>&nbsp;&nbsp;&nbsp;&nbsp;" + emailaddress + "<br> Delivery Mode:" + delMode + "</td>";
            emailMessage = emailMessage + "</tr>";
            emailMessage = emailMessage + "</table>";

            EmailMiddle();
            EmailBottom();
            EmailSend();

        }

        #endregion

This post has been edited by andrewsw: 31 January 2019 - 01:39 AM
Reason for edit:: removed redundant quote, just use the Reply button

[Skydiver]

Are you sure lines 5-9 of your original post are correct?

05            int i = listBox1.SelectedItems.Count + 1;
06 
07          if (i < 0)
08            {
09                var exp = (EItem)listBox1.Items[i];

If there are no selected items, then i will be 1. If there is at least one selected item, then i will be greater than 1. So how will line 7 ever be true?

And if i < 0 is true, then you'll be using a negative index on line 9. How is that not throwing an exception?

[Skydiver]

I'm suspecting more and more that the code that you posted is not the code that you are running (even disregarding removing the connection string details). For example the picture you posted in your original post shows a list box with multiple fields being displayed as a string, but in the code in post #3, your statmessage() has line 38 commented out, and line 39 only puts a single field into the listbox.

Anyway, there's even more cases of your business logic being intimately intertwined into your UI. Your basis for determining whether to send an email is if the last item displayed in the listbox has been displayed for more than 5 seconds. So what happens if 2 minutes later, no new items are loaded from your database? You'll send the same email again?

[lsochoa]

actually mate, every time i posted in any forum sites, i was supposed to change my code and more thinking progress while waiting for any response for my concern. based on your question that if 2 minutes later and no new items are loaded from my database and that will send the same email again is obviously no. let me clear that if once a new transaction was added into the listbox, it automatically email the target recipient.

This post has been edited by Skydiver: 01 February 2019 - 05:04 AM
Reason for edit:: Removed unnecessary quote. No need to quote the post above yours.

[Skydiver]

There is no need to quote the post above yours. Just use the big Reply button or the Fast Reply area.

Okay, where is the logic in the code that you've shown that detects if no new transaction has been loaded since the last time all the transactions were loaded that determines whether to send an email out not? Is that when counter is set 100? But where is counter ever set to another value?

[hexagod]

lsochoaYou know for

            emailMessage = emailMessage + "</tr>";
            emailMessage = emailMessage + "<tr>";

you can just do

            emailMessage += "</tr>";
            emailMessage += "<tr>";

and I know because of the timer you're using winforms... WPF prolly better if you're in C# IMO .. binding context is a lot easier

[hexagod]

I know as a fellow winforms user how tempting it is do things this way... but you might want to consider porting your code over to WPF before this project gets too big. Really, you shouldn't be adding items to any listbox you should be creating a Collection<T> of your email objects EItems and then do a binding context.

McAfee broke my work computer yesterday so I'm on a loaner ATM and @ werk without VS on this one so I can't do the code for you but basically you would create a collection of your EItems and then fire off an email when that collection changes. Then you're going to setup a binding context for the listbox ItemsSourceor DataSource with a return for ToString() on the properties you want to be DISPLAYED in the listbox

https://docs.microso...framework-4.7.2

https://www.wpf-tuto...he-datacontext/

You won't have to .Add() anything to the listbox itself you're going to add into the collection and then the listbox will automatically update. This code will be completely seperate from your SQL but the SQL/statmessage will be what adds items to the collection.

EDIT actually you might be better off using a List but here's an example of listening for a change

https://stackoverflo...event-on-change

Either way iBelieve you can do a binding context on a list or collection (for a listbox). If not, use the one that you can with the BC. I would test it but like I said my work computer got toasted so I can't atm.

This post has been edited by hexagod: 01 February 2019 - 03:16 PM

[lsochoa]

if there is no transaction created, then no send email out because that is a previous transaction, Actually i tried to put a counter value due to i encounter an email spamming to the last created transaction. if i removed a counter variable, it continuous to email every 5 seconds.

[Sheepings]

First, move to WPF, Winforms has seen better days, WPF is the way forward now, and its mostly what I use when tasked with creating a new app.

Second, bind your data instead.

Lastly, introduce multithreading when working with databases, and its more practical to update your UI, from non UI threads as opposed to timers etc.

Curious why you appear to ignored post 5. If I was you, I'd rewrite it from scratch and move to WPF while you're at it.