2
\$\begingroup\$

The following code is from the DataAccess layer that calls the SQL stored procedure and returns the result set back to the calling code.

public class CoffeeDataAccess : ICoffeeDataAccess
    {
        private readonly Database coffeeDatabase;

        public CoffeeDataAccess(ICoffeeShopDatabaseFactory factory)
        {
            this.coffeeDatabase = factory.GetCoffeeDatabase();
        }

        public async Task<IEnumerable<Coffee>> GetAllAvailableCoffees()
        {
            IEnumerable<Coffee> coffees = await this.ExecuteCommandAsync("spGetAllCoffees");
            return coffees;
        }

        public async Task<IEnumerable<Coffee>> GetCofeesOfTheWeek()
        {
            IEnumerable<Coffee> coffees = await this.ExecuteCommandAsync("spGetCoffeesOfTheWeek");
            return coffees;
        }


        private async Task<IEnumerable<Coffee>> ExecuteCommandAsync(string spName)
        {
            IList<Coffee> coffees;

            using (DbCommand command = this.coffeeDatabase.GetStoredProcCommand("dbo" + spName))
            {

                Task<IDataReader> dataReaderTask =
                    Task<IDataReader>.Factory.FromAsync(this.coffeeDatabase.BeginExecuteReader, this.coffeeDatabase.EndExecuteReader, command, null);

                using (IDataReader reader = await dataReaderTask)
                {
                    coffees = new List<Coffee>();
                    while (reader.Read())
                    {
                        Coffee c = new Coffee();
                        c.Id = int.Parse(reader["ID"].ToString());
                        c.Name = reader["Name"].ToString();
                        c.ShortDesc = reader["ShortDesc"].ToString();
                        c.LongDesc = reader["LongDesc"].ToString();
                        c.Price = decimal.Parse(reader["Price"].ToString());
                        c.ImageUrl = reader["ImageUrl"].ToString();
                        c.ImageThumbnailUrl = reader["ImageThumbnailUrl"].ToString();
                        c.CategoryId = int.Parse(reader["CategoryId"].ToString());
                        coffees.Add(c);
                    }
                }
            }

            return coffees;
        }

    }

The CoffeeDataAccess object is newly instantiated every time when a client code asks for it from the IoC container (registered as Trasient in ASP.NET Core - startup class). Is the method name ExecuteCommandAsync thread safe or not?

I think it is because each time a new instance of the CoffeeDataAccessclass is created but I'm not sure.

\$\endgroup\$
  • 2
    \$\begingroup\$ Please change the title to make it a summary of the purpose of the code. (Thread safety is not a purpose of the code, it's an attribute you would like to achieve. Stored procedure is not a purpose of the code, it's an implementation detail. You can explain these in the body of the question, but the title should be about the purpose of the code.) \$\endgroup\$ – janos Mar 7 '17 at 12:14
  • \$\begingroup\$ @janos: The purpose of the code is DataAcess - thats it. Would not that be so generic title? Please help to decide a suitable title - Thank you! \$\endgroup\$ – Yawar Murtaza Mar 7 '17 at 15:06
1
\$\begingroup\$

Unless factory.GetCoffeeDatabase(); returns a shared factory I cannot see any problems.

On the Reader reader.GetInt32(0); is more efficient. There are direct calls for all the data type.

Assigning property by property is fine. I think passing in a constructor is cleaner.

\$\endgroup\$
  • \$\begingroup\$ Thank you Paparazzi, factory.GetCoffeeDatabase() method returns a Database object not a factory object which is not shared. \$\endgroup\$ – Yawar Murtaza Mar 7 '17 at 15:05
  • 1
    \$\begingroup\$ reader.GetInt32(0); doesnt take the column name. I cant depend upon the order as it can be changed later in the future. May be an extension method can be written on IDataReader to type cast like this:reader.ItemOrDefault<int>("ID"); and reader.ItemOrDefault<string>("Name"); - thanks! \$\endgroup\$ – Yawar Murtaza Mar 7 '17 at 15:12
  • \$\begingroup\$ @YawarMurtaza Well if you could count on an order you could use ordinal. \$\endgroup\$ – paparazzo Mar 7 '17 at 15:22
  • \$\begingroup\$ reader.GetValue(reader.GetOrdinal("Name")); looks less readable than eader.ItemOrDefault<string>("Name"); \$\endgroup\$ – Yawar Murtaza Mar 7 '17 at 15:25
  • \$\begingroup\$ @YawarMurtaza I was referring to efficiency. reader.GetValue(reader.GetOrdinal("Name")); is certainly not efficient. \$\endgroup\$ – paparazzo Mar 7 '17 at 15:31
0
\$\begingroup\$

Haven't tested the code, but the code should be thread safe! All ExecCommandAsync methods are called from within asynchronous functions.

\$\endgroup\$

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.