Function to return List<t>

[Solarion]

Am I doing this right? It works just fine but I'm wondering what you guys do when you need to return a list of items from the DB, perhaps an easier/simpler way.

This is the function.

 public List<String> CreateDetails(DateTime TransactionDate, string submissionid)
        {

            string sql = "select distinct CustomerNumber from tblCustDetails where transaction_date = '" + TransactionDate + "' and submission_id = '" + submissionid + "'";

            String myConnString = ConfigurationManager.ConnectionStrings["Connection"].ToString();
            SqlConnection myConnection = new SqlConnection(myConnString);
            SqlCommand myCommand = new SqlCommand();

            myCommand.CommandType = CommandType.Text;
            myCommand.Connection = myConnection;
            myCommand.CommandText = myCommand.CommandText = sql;

            try
            {
                myConnection.Open();
                List<String> CustomerNumberInfoColl = new List<String>();
                SqlDataReader sqlDataReader = myCommand.ExecuteReader();

                if (sqlDataReader.HasRows)
                {

                    while (sqlDataReader.Read())
                    {

                        CustomerInfo customerNumber = new CustomerInfo();
                        CustomerNumberInfoColl.Add(sqlDataReader["CustomerNumber"].ToString());

                    }
                }

                return CustomerNumberInfoColl;

            }

            catch (Exception ex)
            {

                throw ex;
            }
            finally
            {
                myConnection.Close();
            }

        }

And here is where I call it.

        private void RefreshListBox()
        {

            CustomerInfo ficCustomerNumber = new CustomerInfo();
            List<String> CusList = CreateDetails(Convert.ToDateTime(dtDateTime.Value.Date.ToString("M/d/yyyy")), this.txtID.Text);

            if (CusList.Count > 0)
            {
                foreach (String item in CusList)
                {
                    listBox1.Items.Add(item.ToString());
                }
            }

        }

 

[dabean]

A few points

Firstly, always use parameters in your SQL statements:

 string sql = "select distinct CustomerNumber from tblCustDetails where transaction_date = @TransactionDate and submission_id = @submissionid";
 ...
 myCommand.Parameters.AddWithValue("@TransactionDate", TransactionDate);
 myCommand.Parameters.AddWithValue("@submissionid", submissionid);

Secondly, you're creating a CustomerInfo class but never using it. Instead of returning a List<string>, I'd expect you to return a List<CustomerInfo>.

                    while (sqlDataReader.Read())
                    {

                        CustomerInfo customer = new CustomerInfo();
                        customer.Number = sqlDataReader["CustomerNumber"].ToString();
                        CustomerNumberInfoColl.Add(customer);

                    }

... and call it like this:

        private void RefreshListBox()
        {

            List<CustomerInfo> CusList = CreateDetails(Convert.ToDateTime(dtDateTime.Value.Date.ToString("M/d/yyyy")), this.txtID.Text);
						
	    listBox1.DataSource = CusList;
	    listBox1.DisplayMember = Number;	 //Property of the class CustomerInfo
	    //listBox1.ValueMember = Id;			 

        }

That way you can fetch all the customer information you need in a single list and just bind the properties you need to whatever controls.
Of course you wouldn't do the db call in the RefreshListBox method, but rather do 1 call and bind all the controls to that list.

 

[Solarion]

That looks much better, to be able to bind it to any controls such as a DataGrid. I was trying to create something generic like that but got lost halfway through.

Thanks!

 

[CamiKaze]

I'm just going to nitpick here and there...

myCommand.CommandText = myCommand.CommandText = sql;

What????

myConnection.Open();

Rather use a "using" statement.

CustomerInfo customerNumber = new CustomerInfo();

This should not be in a while loop.

 

[semaphore]

I gagged a bit.

 

[Hamster]

Use Dapper

using Dapper;
using System;
using System.Collections.Generic;
using System.Configuration;
using System.Data;
using System.Data.SqlClient;
using System.Linq;

namespace Konsole
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            var customerNumbers = GetCustomerNumbers(DateTime.Now, "abc");

            Console.ReadKey();
        }

        public static List<string> GetCustomerNumbers(DateTime transactionDate, string submissionId)
        {
            string myConnString = ConfigurationManager.ConnectionStrings["Connection"].ToString();
            using (IDbConnection conn = new SqlConnection(myConnString))
            {
                string sql = "select distinct CustomerNumber from tblCustDetails where transaction_date = @transactionDate and submission_id = @submissionId";
                var param = new { transactionDate = transactionDate, submissionId = submissionId };
                var results = conn.Query<string>(sql, param, commandType: CommandType.Text);
                return results.ToList();
            }
        }
    }
}

 

[Hamster]

Well since we are nitpicking. From the top:

1. This is C#. Use string and not String
2. Your function name is wrong. You are getting not creating.
3. You variable naming sucks. Use camelCasing (transactionDate, submissionId)
4. The SQL injection is actually unforgivable unless you are a junior
5. You should be using the using() keyword in C#

 

[semaphore]

I'm just going to nitpick here and there...


What????


Rather use a "using" statement.


This should not be in a while loop.

You picked up that? Besides the rest of that abomination called code.

 

[CamiKaze]

You picked up that? Besides the rest of that abomination called code.

I picked up a shst lot and was going to edit my post to add the rest but then @Hamster came along with a nice and elegant solution...
So quite pointless for me to add the rest.

 

[semaphore]

I picked up a shst lot and was going to edit my post to add the rest but then @Hamster came with a nice and elegant solution...

Hamster prefers nHibernate.

 

[Hamster]

Hamster prefers nHibernate.

 

[.QQ.]

Hamster prefers nHibernate.

You monster.

 

[Solarion]

You picked up that? Besides the rest of that abomination called code.

Try to be less of a keyboard crusader and a little more helpful next time.

 

[Mr.Jax]

If this is production code....

a) Create a Class that does all your db stuff, connection strings, transactions, etcetera. Implement methods you will typically use on SQL, ie "ExecuteSQL", "ExecuteStoredProc", they all return DataSets or they accept callbacks so that you can use DataReader to just stream data.This guy is usually known as your Database Interface Layer (DIL). Oh and make sure you use SQL parameters, this is not negotiable bro.

b) Create a class, for each functional area of your system that wraps all database related activities on your data. This sucker will use your DIL to execute SQL. The functions you implement and expose here have ZERO SQL related inputs and outputs, here you work with "business objects". For instance, you will have methods called "MySession LoginUser( string username, string password)" or "List<MyUser> GetAllUsers( MySession session)" and so on. This guy is usually called your Data Access Layer.

c) Any application code exclusively makes use of the DAL

d) You may consider creating a Business Logic Layer as well, which might make use of one or more DAL's. You might also do transaction management, authentication and authorization here, all depending on how many DAL's you use.


Why do all of the above ?
- Seperation of Concerns (See the Single Responsibility Principle https://en.wikipedia.org/wiki/Single_responsibility_principle )
- Technology Abstraction, if you'd have to use a different Db technology, keeping your db access in one class makes the changes central and easy.
- Reusability.
- Testability.

EDIT: this is a more SOLID approach which includes Seperation of Concerns/Single Responsibility Principle as one of the rules https://en.wikipedia.org/wiki/SOLID_(object-oriented_design)

Regarding your code snippet, I wouldn't return a List<String>, but rather a List<MyObject>. You might want to return more fields than just the one you're returning now.

 

[Hamster]

Regarding your code snippet, I wouldn't return a List<String>, but rather a List<MyObject>. You might want to return more fields than just the one you're returning now.

IEnumerable<string> .. or IEnumerable<object>

 

[Mr.Jax]

IEnumerable<string> .. or IEnumerable<object>

IEnumerable<>, yup agreed.

IEnumerable<object> Do you imply IEnumerable<System.Object> or "just the object of your choice", because IEnumerable<System.Object> usually isn't a good idea. The caller will have to know what it should be and cast (or check what it is and cast accordingly); this voids compile time type safety and affects efficiency.

 

[Hamster]

IEnumerable<>, yup agreed.

IEnumerable<object> Do you imply IEnumerable<System.Object> or "just the object of your choice", because IEnumerable<System.Object> usually isn't a good idea. The caller will have to know what it should be and cast (or check what it is and cast accordingly); this voids compile time type safety and affects efficiency.

I meant MyObject/Whatever. IEnumerable<T> if you'd like.

Point is you do not want to be able to change the contents of a list of data you got from another service by accident. The next implementation might decide to cash the contents and return the same List<T> the whole time. You then go add/remove/sort that list in the consumer and have a knock on effect with subsequent calls.

 

[Mr.Jax]

I meant MyObject/Whatever

 

[Solarion]

If this is production code....

a) Create a Class that does all your db stuff, connection strings, transactions, etcetera. Implement methods you will typically use on SQL, ie "ExecuteSQL", "ExecuteStoredProc", they all return DataSets or they accept callbacks so that you can use DataReader to just stream data.This guy is usually known as your Database Interface Layer (DIL). Oh and make sure you use SQL parameters, this is not negotiable bro.

b) Create a class, for each functional area of your system that wraps all database related activities on your data. This sucker will use your DIL to execute SQL. The functions you implement and expose here have ZERO SQL related inputs and outputs, here you work with "business objects". For instance, you will have methods called "MySession LoginUser( string username, string password)" or "List<MyUser> GetAllUsers( MySession session)" and so on. This guy is usually called your Data Access Layer.

c) Any application code exclusively makes use of the DAL

d) You may consider creating a Business Logic Layer as well, which might make use of one or more DAL's. You might also do transaction management, authentication and authorization here, all depending on how many DAL's you use.


Why do all of the above ?
- Seperation of Concerns (See the Single Responsibility Principle https://en.wikipedia.org/wiki/Single_responsibility_principle )
- Technology Abstraction, if you'd have to use a different Db technology, keeping your db access in one class makes the changes central and easy.
- Reusability.
- Testability.

EDIT: this is a more SOLID approach which includes Seperation of Concerns/Single Responsibility Principle as one of the rules https://en.wikipedia.org/wiki/SOLID_(object-oriented_design)

Regarding your code snippet, I wouldn't return a List<String>, but rather a List<MyObject>. You might want to return more fields than just the one you're returning now.

A production model is something I've been aiming toward for some time now especially for those two points you mentioned. I'm not very experienced but have a good idea of what I want. I'll have to do some research on this subject. Thanks Jax.

 

[oomjan]

How about using a SqlDataAdapter to fill a datatable then do this:
dataTable.AsEnumerable().Select(r => r.Field<string>(columnName)).ToList();

Or even better, use EntityFramework or LinqToSql:
db.YourTable.Where(w => w.SomeColumn == "WhatEva").Select(s => s.YourColumn).ToList();