Accessing SQL stored procedures

Question

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.

• janos: 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.)
• Yawar Murtaza: @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!

Answer 1

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.

• Yawar Murtaza: Thank you Paparazzi, factory.GetCoffeeDatabase() method returns a Database object not a factory object which is not shared.
• Yawar Murtaza: 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!
• paparazzo: @YawarMurtaza Well if you could count on an order you could use ordinal.
• Yawar Murtaza: reader.GetValue(reader.GetOrdinal("Name")); looks less readable than eader.ItemOrDefault<string>("Name")
• paparazzo: @YawarMurtaza I was referring to efficiency. reader.GetValue(reader.GetOrdinal("Name")); is certainly not efficient.

Answer 2

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