Creating multiple objects from SQL server extract

Question

I've created a simplified prototype solution as a sort of proof of concept before starting a larger program.

Here is the test data used to build the objects:

create table blah.dbo.tb_sandpitCreateMultipleLinkedObjects
(
 CustomerName varchar(50),
 ItemCode varchar(50)
);
insert into blah.dbo.tb_sandpitCreateMultipleLinkedObjects
values 
 ('jq','bat'),
 ('jq','blackRubber'),
 ('jq','redRubber'),
 ('mt','redRubberX'),
 ('mt','redRubberY'),
 ('mt','redRubberZ'),
 ('mt','blackRubberA'),
 ('mt','blackRubberB'),
 ('mt','bladeFast'),
 ('mt','bladeVeryFast'),
 ('mt','bladeTooFast');

It has two concrete classes representing customers and items that the customers have bought. There are a few customers but potentially many items.

The only use case I have to address is:

An administrator identifies a user, then the system will display the items bought by that user. The system does not need to send any information back to SQL server.

Here are the two concrete classes:

class: Customer

using System;
using System.Collections.Generic;
namespace SandpitCreateMultipleLinkedObjects
{
 class Customer
 {
 //links
 List<Item> items;
 Item i;
 //property
 public String Name { get; private set; }
 //constructor
 public Customer(String name)
 {
 items = new List<Item>();
 Name = name;
 }
 //method to add link to item bought by customer
 public void addItem(Item i)
 {
 items.Add(i);
 }
 }
}

class: Item

using System;
namespace SandpitCreateMultipleLinkedObjects
{
 class Item
 {
 //name property
 public String Name { get; private set; }
 public Item(String name)
 {
 this.Name = name;
 }
 }
}

I have an orchestrating/coordinating class that ultimately will talk to the user interface:

using System;
using System.Data;
using System.Data.SqlClient;
using System.Configuration;
using System.Collections.Generic;
using System.Collections.ObjectModel;
namespace SandpitCreateMultipleLinkedObjects
{
 class AppCoordinator
 {
 Dictionary<String,Customer> customers;
 private string connString;
 private const string targetTableName = @"blah.dbo.tb_sandpitCreateMultipleLinkedObjects";
 private DataTable dt;
 public AppCoordinator()
 {
 //this will contain links to the customers
 customers = new Dictionary<string,Customer>();
 //fill the main datatable
 this.dt = fillDataTable();
 //get a distinct view of the customers
 //instantiate the customer objects
 DataView view = new DataView(dt);
 DataTable distinctValues = view.ToTable(true,new String[] { "CustomerName" });
 foreach(DataRow row in distinctValues.Rows)
 {
 String custName = row["CustomerName"].ToString();
 customers.Add(custName,new Customer(custName));
 }
 //then the instantiate the item objects
 foreach(DataRow row in dt.Rows)
 {
 String custName = row["CustomerName"].ToString();
 Customer linkedCustomer;
 customers.TryGetValue(custName,out linkedCustomer);
 String itemName = row["ItemCode"].ToString();
 Item i = new Item(itemName);
 linkedCustomer.addItem(i);
 }
 }
 //gets data from the server
 private DataTable fillDataTable()
 {
 this.connString = ConfigurationManager.ConnectionStrings["foo"].ConnectionString;
 SqlConnection conn = null;
 SqlDataAdapter ad = null;
 DataSet ds = null;
 DataTable returnDt = null;
 try
 {
 conn = new SqlConnection(this.connString);
 ad = new SqlDataAdapter("SELECT * FROM " + targetTableName,conn);
 ds = new DataSet();
 ad.Fill(ds,"AvailableReports");
 returnDt = ds.Tables["AvailableReports"];
 }
 catch(SqlException ex)
 {
 Console.WriteLine(ex.ToString());
 }
 finally
 {
 if(conn != null)
 {
 conn.Close();
 conn.Dispose();
 }
 }
 return returnDt;
 }
 //a read-only copy of the dictionary
 //read-only for hiding purposes
 public ReadOnlyDictionary<String,Customer> getCustomers()
 {
 return new ReadOnlyDictionary<String,Customer>(customers);
 }
 }
}

Here is Main which resides in Program:

using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
namespace SandpitCreateMultipleLinkedObjects
{
 class Program
 {
 private AppCoordinator appCoord;
 static void Main(string[] args)
 {
 Program p = new Program();
 p.appCoord = new AppCoordinator();
 ReadOnlyDictionary<String,Customer> dics = p.appCoord.getCustomers();
 Console.WriteLine("{0}",dics.Count);
 foreach(String aName in dics.Keys)
 {
 Customer c;
 dics.TryGetValue(aName,out c);
 Console.WriteLine("{0} has this many items: {1}",c.Name, c.getItems().Count);
 }
 Console.WriteLine();
 Console.WriteLine("press [enter] to exit");
 Console.Read();
 }
 }
}

It works ok. I am new to OOP so any tips and pointers on this code would be appreciated.

Answer 1

Customer and Item classes

Well, to start with, Customer.i isn't used and I don't see how it could ever be. So that should just go away.

Your naming style is off for C#; you should be using PascalCase instead of camelCase for public methods (eg., AddItem).

Customer.AddItem and Customer.GetItems don't add much value; they're OK as-is, and are actually a reasonable encapsulation - but they're kind of overkill here. (as an aside, your Customer class doesn't include GetItems, but I inferred it from your Main). This comment could really be directed to the whole structure, actually - since you really just need a Dictionary<string, List<string>> to solve the problem. Not to say the structure can't be useful, but I would consider it overkill unless there's some future plans being made here.

Program class

Your Program class sets a private AppCoordinator member, but never uses it outside of Main; it's odd to have a class instantiate itself like this. In this case, you can just use the AppCoordinator as a local.

Console.WriteLine will take an object and call .ToString on it for you; no reason to use the format overload.

 Console.WriteLine(dics.Count);

You can foreach over the Dictionary directly, getting a KeyValuePair<string, Customer>. Or you can foreach over Dictionary.Values and just get the Customer. Either would be preferable to going over the Keys just to get to the Value (and you don't check the return of TryGetValue). I'd also change the name of your dics variable since the trailing s implies that it's multiple dictionaries (FWIW, I'd probably go with just d just so I didn't have to ever read a potentially offensive word over the phone; but that's just me!)

 foreach(Customer customer in dic.Values)
 {
 Console.WriteLine("{0} has this many items: {1}", customer.Name, customer.GetItems().Count);
 }

Console.Read will exit at any key; you want to change your message or change that to Console.ReadLine to require an Enter.

AppCoordinator

Your AppCoordinator class is a major code smell; it's a (no offense) terrible name that really doesn't describe what it's doing, and is likely to become a "god object". I would rename that to CustomerRepository or CustomerDataAccess or something.

Since this is your largest class, the meat of my comments are here:

• prefer smallest scope possible for variables: targetTableName and dt should all be locals; technically so should connString, but we'd generally want the ability to set that one from a constructor or something
• don't do work you don't have to: SqlDataAdapter.Fill will fill just a DataTable, don't pass it a DataSet just to pull out the DataTable
• prefer using over explicit Dispose: it reads better, and is much easier to get correct
• Some will quibble, but I don't like the assignment to a local just to return at the end
• Don't catch exceptions that you can't do anything about; if you do want to log, then throw at the end (otherwise, you just returned an empty datatable which someone is going to read and think there's been no sales)
• Pet peeve, but I prefer explicit cast to calling ToString; that makes you explicitly deal with DbNull
• You generally don't want to be doing data access or other "real" work in the constructor; it opens you up to possible exceptions, makes inheritance harder, and is generally unexpected. Move all of that to the GetCustomers method. I'm not sure that it's a requirement, but your existing code does end up caching the database results; you can do that lazily instead.
• Maybe not now, but you'll definitely want to consider finding a mini (or full) ORM to cut out the repetitive ADO.NET code.
• Using DataTables isn't the end of the world, but not really standard at this point. I'd switch those to a DataReader instead since you don't really need the extra functiuonality of the DataTable. You do use it to build a distinct Customer list, but that can be done with a ContainsKey on your Dictionary instead.

So, I'd suggest it'd end up looking something like (it's been awhile since I've written straight ADO.NET, so there may be some minor issues here):

 class CustomerDataAccess 
 {
 private string readonly connectionString;
 private ReadOnlyDictionary<string, Customer> readOnlyCustomers;
 public CustomerDataAccess() 
 : this(ConfigurationManager.ConnectionStrings["foo"].ConnectionString)
 {
 }
 public CustomerDataAccess(string connectionString)
 {
 this.connectionString = connectionString;
 }
 public ReadOnlyDictionary<String,Customer> GetCustomers()
 {
 if (this.readOnlyCustomers == null) 
 {
 Dictionary<String, Customer> customers = this.GetCustomersInternal();
 this.readOnlyCustomers = new ReadOnlyDictionary(customers);
 }
 return this.readOnlyCustomers;
 }
 private Dictionary<String, Customer> GetCustomersInternal()
 {
 const string targetTableName = @"blah.dbo.tb_sandpitCreateMultipleLinkedObjects";
 try 
 {
 using (SqlConnection conn = new SqlConnection(this.connectionString))
 using (SqlCommand cmd = new SqlCommand("SELECT * FROM " + targetTableName, conn))
 {
 conn.Open();
 Dictionary<string,Customer> customers = new Dictionary<string,Customer>();
 using (SqlDataReader dr = cmd.ExecuteReader())
 {
 string customerName = dr.GetString("CustomerName");
 Customer customer;
 if (!customers.TryGetValue(customerName, out customer))
 {
 customer = new Customer(customerName);
 customers.Add(customer.Name, customer);
 }
 string itemCode = dr.GetString("ItemCode");
 Item item = new Item(itemCode);
 customer.AddItem(item);
 }
 this.readOnlyCustomers = new ReadOnlyDictionary(customers);
 }
 }
 catch (SqlException ex)
 {
 Console.WriteLine(ex);
 throw;
 }
 }
 }

• whytheq: Hello Mark. I've refactored with most of your suggestions (not all). Should I re-post the new version in a further question? There maybe aspects of your suggestions that I've not implemented that you feel are more important than some which I have taken on board.

Answer 2

Please have explicit access modifiers. I do not want to figure out whether class Customer and List<Item> items; and class Item etc. are public or internal or private.

---

Comments should say why something was implemented that way. //name property doesn't tell me anything I can't see with my own eyes.

---

Use the aliases whenever possible: string instead of String, int instead of Int32,...

---

What kind of namespace is SandpitCreateMultipleLinkedObjects? Most of that seems to be a method name. Look at System.Data.SqlClient or System.Collections.Generic etc. for examples of proper namespaces. I know you said that this is a "a sort of proof of concept", but that still doesn't mean you should abandon good naming practices.

Even the code I write for applications only I will ever use is as close to "professional" code as possible for two good reasons:

• It helps create good habits.
• Plenty of times I find I reuse such "quick fix" applications over a long period and I need to maintain them; having decent code is just easy to work with.

---

All of that db code in AppCoordinator should really move to a separate layer. You should have one class that just converts db data to objects (Customer, etc.), and other classes should consume it.

---

You should look at database normalization. Your current db structure looks simple, but in reality it obscuring a far more complicated one. Hence the rather forced way you first create Customers and then create Items that are linked to that Customer. You loop through the same DataTable twice in doing so, which should be a warning sign.

---

SqlConnection etc. should be encapsulated in a using statement.

• whytheq: (upped) appreciated the pointers. I aim to write professional standard code even when writing prototype code inside a sandpit but I am still learning ...hence my post. Tomorrow I will refactor and put in place your suggestions. I assume you are saying the db code should be in a class that is then used, via composition, inside the coordinator class?
• BCdotWEB: @whytheq Ideally you should work with interfaces and use IoC/dependency injection to link the various layers. Also remember the single responsibility principle from SOLID when it comes to classes and methods.