Method that return related entities using OData

Question

There is method GetFull that should use OData return related entities.

When I call GetFull(12, "entity1,entity2") then I will get to current object with id = 12 and related entity1 and entity2 (http://.../api/City/12?$expand=Region)

When I call GetFull(12, "0") then I want to automatical find and get all related entities from current object (http://.../api/City/12?$expand=Region,Country). For that I'm using reflections.

How can I improve this method?
It's working but I'm sure that I implemented it not properly.

P.S : It was done for user-friendly link
before:http://.../api/City/12?$expand=Region,Country
now:http://.../api/City/12/GetFull or http://.../api/City/12/GetFull/Region

public virtual HttpResponseMessage GetFull(int id, string entities)
{ 
 string propertyList = string.Empty;
 if (!entities.Equals("0"))
 {
 propertyList = entities;
 }
 else
 {
 foreach (var prop in typeof(T).GetProperties().Where(p => p.GetGetMethod().IsVirtual))
 {
 if (prop.PropertyType.IsClass && !prop.PropertyType.FullName.StartsWith("System."))
 {
 propertyList += prop.Name + ",";
 }
 }
 propertyList = propertyList.Remove(propertyList.Length - 1);
 }
 string entityName = typeof(T).Name;
 var baseUrl = Request.RequestUri.GetLeftPart(UriPartial.Authority);
 var response = Request.CreateResponse(HttpStatusCode.Found);
 StringBuilder sb = new StringBuilder();
 sb.Append(baseUrl);
 sb.Append("/api/");
 sb.Append(entityName);
 sb.Append("/");
 sb.Append(id);
 sb.Append("?$expand=");
 sb.Append(propertyList);
 response.Headers.Location = new Uri(sb.ToString());
 return response;
}

Sorry for my English and if I have to add more informations, please let me know. Thanks

Answer 1

• GetFull() is a methodname which doesn't tell any caller anything about what the method will do. Please choose always names which are as descriptive as possible.

• this loop

  foreach (var prop in typeof(T).GetProperties().Where(p => p.GetGetMethod().IsVirtual))
  {
   if (prop.PropertyType.IsClass && !prop.PropertyType.FullName.StartsWith("System."))
   {
   propertyList += prop.Name + ",";
   }
  }
  propertyList = propertyList.Remove(propertyList.Length - 1); 
  

  could be improved by either using a StringBuilder or by using a ILIst<string> together with string.Join(). This should maybe be extracted to a separate method.

• if (!entities.Equals("0")) you are using entities without doing any null check, so better use if (!"0".Equals(entities)). I usually like positive checks more so I would like to suggest to revert this if..else.

• by default the newed StringBuilder has a capacity of 16 which is doubled if the added value won't fit anymore. So better initialize it with a higher number by using the overloaded constructor which takes the initial size as an argument.
  In addition you can take advantage of the fluent methods of the StringBuilder which return the StringBuilder itself.

All mentioned points (except for the method name) implemented leads to

public virtual HttpResponseMessage GetFull(int id, string entities)
{ 
 string propertyList = string.Empty;
 if ("0".Equals(entities))
 {
 propertyList = GetJoinedPropertyList();
 }
 else
 {
 propertyList = entities;
 }
 string entityName = typeof(T).Name;
 var baseUrl = Request.RequestUri.GetLeftPart(UriPartial.Authority);
 var response = Request.CreateResponse(HttpStatusCode.Found);
 StringBuilder sb = new StringBuilder(1024);
 sb.Append(baseUrl)
 .Append("/api/")
 .Append(entityName)
 .Append("/")
 .Append(id)
 .Append("?$expand=")
 .Append(propertyList);
 response.Headers.Location = new Uri(sb.ToString());
 return response;
}
private string GetJoinedPropertyList()
{
 IList<string> propertyNames = new List<string>();
 foreach (var prop in typeof(T).GetProperties().Where(p => p.GetGetMethod().IsVirtual))
 {
 if (prop.PropertyType.IsClass && !prop.PropertyType.FullName.StartsWith("System."))
 {
 propertyNames.Add(prop.Name);
 }
 }
 return string.Join(",", propertyNames);
}

• Roman Marusyk: Great! Thank you for help. It is very helpful for me