Creating custom objects with custom properties using generics

Question

I have two base classes: City and Building:

public class City
{
 public string Name { get; set; }
 public double Area { get; set; }
}
public class Building
{
 public string Name { get; set; }
 public double Area { get; set; }
 public int Stories { get; set; }
}

With two classes that inherit from Building, with their own Id property:

public class MyBuilding : Building
{
 public int MyId { get; set; }
}
public class HisBuilding : Building
{
 public int HisId { get; set; }
}

And two classes that inherit from City, with their own Id and IEnumerable<Building> properties:

public class MyCity : City
{
 public int MyId { get; set; }
 public IEnumerable<MyBuilding> Buildings { get; set; }
}
public class HisCity : City
{
 public int HisId { get; set; }
 public IEnumerable<HisBuilding> Buildings { get; set; }
}

Below is a method I've created that returns a KeyValuePair<City, Building> using generics:

public static KeyValuePair<TCity, IEnumerable<TBuilding>> GetData<TCity, TBuilding>()
 where TCity : City, new()
 where TBuilding : Building, new()
{
 TCity city = new TCity();
 IEnumerable<TBuilding> buildings = new List<TBuilding>();
 return new KeyValuePair<TCity, IEnumerable<TBuilding>>(city, buildings);
}

And in my Main method I've logic that instantiates a specific City object based on a user's input.

int input = -1;
string strInput = Console.ReadLine();
int.TryParse(strInput, out input);
string json = string.Empty;
switch (input)
{
 case 1:
 KeyValuePair<MyCity, IEnumerable<MyBuilding>> myCity
 = GetData<MyCity, MyBuilding>();
 MyCity my = myCity.Key;
 my.MyId = 1;
 my.Buildings = myCity.Value;
 json = JsonConvert.SerializeObject(my, Formatting.Indented);
 break;
 default:
 KeyValuePair<HisCity, IEnumerable<HisBuilding>> hisCity
 = GetData<HisCity, HisBuilding>();
 HisCity his = hisCity.Key;
 his.HisId = 2;
 his.Buildings = hisCity.Value;
 json = JsonConvert.SerializeObject(his, Formatting.Indented);
 break;
}
Console.WriteLine(json);
Console.ReadLine();

The above generic method is just something I cobbled together in the last half hour, but I'm wondering if there is a more efficient way of creating these custom objects of mine, especially when each City has its own, different IEnumerable<Building> property.

EDIT

Here is the code I have for building the IEnumerable inside my GetData method:

public static IEnumerable<TBuilding> GetBuildingData<TBuilding>() 
 where TBuilding : Building, new()
{
 DataTable table;
 string myConnectionString = @"Data Source=(LocalDb)\MSSQLLocalDb;Initial Catalog=Test;Integrated Security=True;";
 List<TBuilding> buildings = new List<TBuilding>();
 using (SqlConnection connection = new SqlConnection(myConnectionString))
 {
 using (SqlCommand command = new SqlCommand("[dbo].[GetListData]", connection) { CommandType = CommandType.StoredProcedure })
 {
 connection.Open();
 using (SqlDataAdapter adapter = new SqlDataAdapter(command))
 {
 table = new DataTable();
 adapter.Fill(table);
 }
 connection.Close();
 }
 }
 foreach (DataRow row in table.Rows)
 {
 TBuilding building = new TBuilding();
 building.Area = Convert.ToDouble(row["Area"]);
 building.Name = row["Name"].ToString();
 building.Stories = Convert.ToInt32(row["Stories"]);
 buildings.Add(building);
 }
 return buildings;
}

• dfhwze: Could you provide a working example of how you would use such functionality? Atm you just have 2 objects with empty lists. What is the point of this?
• CharlesNRice: Best practices you should make a constructor for types that have properties of ienumerable or collection types and fill them in with a default value and not return null. For IEnumerable just use Enumerable.Empty docs.microsoft.com/en-us/dotnet/api/…
• Delfino: @dfhwze I work with multiple clients and send them all notifications periodically. The notifications contain some of the same data, but Id properties are client-specific, so I'm working to build out the base objects generically, then add the custom ids later on.
• dfhwze: Your method creates an empty list. How would you fill this list? I find this part crucial to review your code :)
• Delfino: @dfhwze I edited my question to include the code I'm using to build that list.

Answer 1

I will focus on..

Creating custom objects with custom properties using generics

Which you do in this method..

public static KeyValuePair<TCity, IEnumerable<TBuilding>> GetData<TCity, TBuilding>()
 where TCity : City, new()
 where TBuilding : Building, new()
{
 TCity city = new TCity();
 IEnumerable<TBuilding> buildings = new List<TBuilding>();
 return new KeyValuePair<TCity, IEnumerable<TBuilding>>(city, buildings);
}

• A KeyValuePair is archaic, you can use a ValueTuple nowadays
• TBuilding does not require the new() constraint in this method
• The choice for IEnumerable over IList is questionable, since you retrieve the items later on and have no way of adding them to an IEnumerable. So why create the IEnumerable in this method?
• The name should reflect what you are doing. CreateCity seems more appropriate.

I would refactor this method..

public static (TCity city, IList<TBuilding> buildings) CreateCity<TCity, TBuilding>()
 where TCity : City, new()
 where TBuilding : Building
{
 var city = new TCity();
 var buildings = new List<TBuilding>();
 return (city, buildings);
}

EDIT:

Thinking about the refactored method. This doesn't even need to be a method anymore. It doesn't do anything but create objects for the types you specify.

What I would really do is throw out this ancient way of ORM, and use an existing API (EF, NHibernate, Dapper).

• t3chb0t: I'm afraid we might here at some point this code is simplified, in my real code I actually do this and that :-\
• Delfino: When I refactor my method using the code you've posted, by replacing the KeyValuePair<TCity, IEnumerable<TBuilding>> with (TCity city, IList<TBuilding> buildings I just end up with a bunch of errors. Could this be because I'm using Visual Studio 2015?
• dfhwze: ValueTuple is supported since blogs.msdn.microsoft.com/mazhou/2017/05/26/…. You could keep using KeyValuePair instead, or use a regular Tuple.
• t3chb0t: ...about the ancient tools... it looks like you should add another one to this list and suggest using VS2019 :-P

Answer 2

public class MyBuilding : Building
{
 public int MyId { get; set; }
}
public class HisBuilding : Building
{
 public int HisId { get; set; }
}

Why do you have the id as properties of the subclasses, it's a candidate for a base class member:

public class Building
{
 public int Id { get; set; }

If the subclasses must have specialized names for their Id property, then provide that as:

public class MyBuilding : Building
{
 public int MyId { get { return Id; } set { Id = value; } }
}

but that is a rather odd concept that you should avoid if possible.

The same holds for Cities.

---

public class MyCity : City
{
 public int MyId { get; set; }
 public IEnumerable<MyBuilding> Buildings { get; set; }
}

Normally you would have a materialized data set for buildings instead of an IEnumerable<T> - unless you're creating the instances lazily/dynamically. You could maybe consider using a IReadonlyList<T> as type, if you don't want it to be modifiable or else just a IList<T>

---

public static KeyValuePair<TCity, IEnumerable<TBuilding>> GetData<TCity, TBuilding>()
 where TCity : City, new()
 where TBuilding : Building, new()
{
 TCity city = new TCity();
 IEnumerable<TBuilding> buildings = new List<TBuilding>();
 return new KeyValuePair<TCity, IEnumerable<TBuilding>>(city, buildings);
}

If you changed the City base class to a generic like:

public class City<TBuilding> where TBuilding: Building
{
 public IList<TBuilding> Buildings { get; set; }
}

and dropped the buildings on the specialized cities, then you could return just the city from GetData, because you can initialize the Buildings property inside GetData, which I would rename to CreateCity()

public static TCity CreateCity<TCity, TBuilding>()
where TCity : City<TBuilding>, new()
where TBuilding : Building, new()
{
 TCity city = new TCity();
 city.Buildings = new List<TBuilding>();
 return city;
}

---

Ideally you could have a common baseclass for City and Building, because they share some significant properties like Id, Area and Name:

public abstract class AreaObject
{
 public int Id { get; set; }
 public string Name { get; set; }
 public double Area { get; set; }
}

A complete refactoring of your data model could then be:

public abstract class AreaObject
{
 public int Id { get; set; }
 public string Name { get; set; }
 public double Area { get; set; }
}
// For convenience a city base class without the generic type parameter:
public abstract class City : AreaObject
{
 public abstract IEnumerable<Building> GetBuildings();
}
public class City<TBuilding> : City where TBuilding : Building
{
 public IList<TBuilding> Buildings { get; set; }
 public override IEnumerable<Building> GetBuildings()
 {
 return Buildings;
 }
}
public class Building : AreaObject
{
 public int Stories { get; set; }
}
public class MyBuilding : Building
{
 public int MyId { get { return Id; } set { Id = value; } }
}
public class HisBuilding : Building
{
 public int HisId { get { return Id; } set { Id = value; } }
}
public class MyCity : City<MyBuilding>
{
 public int MyId { get { return Id; } set { Id = value; } }
}
public class HisCity : City<HisBuilding>
{
 public int HisId { get { return Id; } set { Id = value; } }
}

It's a little odd to have two different sub cities having specialized buildings instead of just Buildings, but you may have reasons for that? (What if MyCity buys a building from HisCity can it then change from His to My?)

---

Here is the code I have for building the IEnumerable inside my GetData method:

public static IEnumerable<TBuilding> GetBuildingData<TBuilding>()...

If that is going to be used inside GetData() shouldn't it then take a city id as argument in order to minimize the query? If so you can change the GetData to:

public static TCity CreateCity<TCity, TBuilding>(int id)
where TCity : City<TBuilding>, new()
where TBuilding : Building, new()
{
 TCity city = new TCity();
 city.Buildings = GetBuildingData<TBuilding>(id);
 city.Id = id;
 return city;
}

And you'll then have to modify your database query to only return the buildings for that city:

public static IList<TBuilding> GetBuildingData<TBuilding>(int cityId)
where TBuilding : Building, new() {...}

---

All in all this could simplify your use case to:

 int cityId = -1;
 string strInput = Console.ReadLine();
 int.TryParse(strInput, out cityId);
 if (cityId > 0)
 {
 City city = null;
 switch (cityId)
 {
 case 1:
 city = CreateCity<MyCity, MyBuilding>(cityId);
 break;
 case 2:
 city = CreateCity<HisCity, HisBuilding>(cityId);
 break;
 default:
 throw new InvalidOperationException("Invalid Id");
 }
 string json = JsonConvert.SerializeObject(city, Formatting.Indented);
 Console.WriteLine(json);
 Console.ReadLine();
 }

---

Update

As mentioned a couple of times in the above, the data model and class structure for especially City is rather unusual. A more conventional structure could be something like:

public abstract class AreaObject
{
 public int Id { get; set; }
 public string Name { get; set; }
 public double Area { get; set; }
}
public abstract class City : AreaObject
{
 public IList<Building> Buildings { get; set; }
}
public class MyCity : City
{
 // Uncomment if you really need this: public IList<MyBuilding> MyBuildings => Buildings.Where(b => b is MyBuilding).Cast<MyBuilding>().ToList();
}
public class HisCity : City
{
 // Uncomment if you really need this: public IList<HisBuilding> HisBuildings => Buildings.Where(b => b is HisBuilding).Cast<HisBuilding>().ToList();
}
public class Building : AreaObject
{
 public int Stories { get; set; }
}
public class MyBuilding : Building
{
}
public class HisBuilding : Building
{
}

As shown, City owns the Buildings property. The original concept of having specialized building properties per city sub class, doesn't make much sense in real life, because what defines a building: its relation to a city or its materials, functionality etc.? Can't two cities have a copy of the same building? And a MyCity should be able to Buy a building from HisCity etc. And a city should be able to own different types of building.

The factory method could then be simplified to this:

 public static TCity CreateCity<TCity>(int id)
 where TCity : City, new()
 {
 TCity city = new TCity();
 city.Buildings = GetBuildingData(id);
 city.Id = id;
 return city;
 }
 public static IList<Building> GetBuildingData(int cityId)
 {
 // The sql stuff...
 }

And the use case to:

 public static void Run()
 {
 int cityId = -1;
 string strInput = Console.ReadLine();
 int.TryParse(strInput, out cityId);
 if (cityId > 0)
 {
 City city = null;
 switch (cityId)
 {
 case 1:
 city = CreateCity<MyCity>(cityId);
 break;
 case 2:
 city = CreateCity<HisCity>(cityId);
 break;
 default:
 throw new InvalidOperationException("Invalid Id");
 }
 string json = JsonConvert.SerializeObject(city, Formatting.Indented);
 Console.WriteLine(json);
 Console.ReadLine();
 }
 }

• Delfino: Why both an abstract City class and a normal City class?
• user73941: @Delfino: In order to make the polymorphism work properly, so you'll be able to write: City city = new MyCity();. Your need to have two different sub cities with specialized Building properties is highly unusual, and breaks the normal concept of inheritance and polymorphism. There is no good solution to that IMO. I'll update my answer with a more conventional data model in a moment.