Share via

Class to return a list or single item

Question

Tuesday, November 26, 2013 6:18 PM

So I'm writing some code in c# to provision virtual machines in VMware, and i'd like to consolidate my classes/functions as much as possible. What I have noticed is there are times when I know i'm only going to get one item back from a call, and yet i'll use the same call to get more than one item back.

for example when I need a list of datacenters the call returns a list of datacenters, whereas once I've selected a datacenter, the call will return a list of the one datacenter.

on the c# side i'm fairly new, what I've been doing is just grabbing [0] from the return, is it possible to have a class that returns either a list of items, or when there is only one item, simple return that one item not as a list?

        protected List<Datacenter> GetDataCenter(VimClient vimClient, string dcName = null)
 
        {
 
            //
 
            // Get a list of datacenters
 
            //
 
            List<Datacenter> lstDatacenters = new List<Datacenter>();
 
            List<EntityViewBase> appDatacenters = new List<EntityViewBase>();
 
            try
 
            {
 
                if (dcName == null)
 
                {
 
                    //
 
                    // Return all datacenters
 
                    //
 
                    appDatacenters = vimClient.FindEntityViews(typeof(Datacenter), null, null, null);
 
                }
 
                else
 
                {
 
                    //
 
                    // Return the named datacenter
 
                    //
 
                    NameValueCollection dcFilter = new NameValueCollection();
 
                    dcFilter.Add("name", dcName);
 
                    appDatacenters = vimClient.FindEntityViews(typeof(Datacenter), null, dcFilter, null);
 
                }
 
                foreach (EntityViewBase appDatacenter in appDatacenters)
 
                {
 
                    Datacenter thisDatacenter = (Datacenter)appDatacenter;
 
                    lstDatacenters.Add(thisDatacenter);
 
                }
 

 
                return lstDatacenters;
 
            }
 
            catch (VimException ex)
 
            {
 
                //
 
                // VMware Exception occurred
 
                //
 
                txtErrors.Text = "A server fault of type " + ex.MethodFault.GetType().Name + " with message '" + ex.Message + "' occured while performing requested operation.";
 
                Error_Panel.Visible = true;
 
                return null;
 
            }
 
        }

This is the first example, I have several classes similar, and it's possible that I've written this one improperly for what I need it to do.

Jeffrey S. Patton Jeffrey S. Patton Systems Specialist, Enterprise Systems University of Kansas 1001 Sunnyside Ave. Lawrence, KS. 66045 (785) 864-0242 | http://patton-tech.com

All replies (11)

Tuesday, November 26, 2013 6:40 PM ✅Answered

Well, imho this is an unnecessary optimization. Cause it does not improve the sematics of your code.

You method is call GetDataCenter(). Get should be used only for property getter methods. A method should also do what it says. You used the singular, thus it should always return a single instance. In your case the return type should be DataCenter.

So I would use two methods:

  1. protected List<Datacenter> RetrieveDatacenters(VimClient vimClient)
  2. protected Datacenter RetrieveDatacenter(VimClient vimClient, string dcName)

Tuesday, November 26, 2013 9:55 PM ✅Answered | 1 vote

Others have sugested to rename the function to "List<DataCenter> getDataCenters", this is a good idea. Asuming you renamed it, you could use this very short code for a function that only returns one DataCenter:

Datacenter getDataCenter(VimClient vimClient, string dcName){
  //This one must have a dcName
  if(dcName == null)
    throw new ArgumentNullException ("dcName must not be null");

  //Just let the Plural version do all the work
  List<Datacenter> temp = GetDataCenters(vimClient, dcName);

  //Check if the Datacenter was found, if not return null
  //Alternatively throw an exception, if you think that fits better
  if(temp.Count == 0)
    return null;

  //Check if more then one Datacenter was found under this name
  //This is a bit on the paranoid side, but better safe then having hard to track bugs
  //There might be a better exception clas then Exception for this
  if(temp.Count > 1)
    throw new Exception("More then one Datacenter was returned when looking for a Datacenter named " + dcName);

  //Okay, we got one dataCenter
  return temp[0];
}

It's a common technique to have only one function overload with the real code and have all others just chain call to that one.
While it might not look very efficient, the JIT can usually optimise it so those chained function calls will not relevantly impact performance.

Let's talk about MVVM: http://social.msdn.microsoft.com/Forums/en-US/wpf/thread/b1a8bf14-4acd-4d77-9df8-bdb95b02dbe2 Please mark post as helpfull and answers respectively.

Thursday, November 28, 2013 8:42 AM ✅Answered

Kelmen: without knowing the exact purpose and usage of this pieces of code, it's hard to tell.

Overloading makes imho only sense when it is the same operation as seen from the next higher layer. But it should not change the semantics. The different return type indicates that it may is different.

Tuesday, November 26, 2013 6:48 PM

I think in a very round-a-bout way that is what I was asking. In my head it seems that I'm heavily duplicating code as this is really only one class of about dozen or so. But, if this is the preferred way of doing it, then cool.

Jeffrey S. Patton Jeffrey S. Patton Systems Specialist, Enterprise Systems University of Kansas 1001 Sunnyside Ave. Lawrence, KS. 66045 (785) 864-0242 | http://patton-tech.com

Tuesday, November 26, 2013 10:10 PM | 1 vote

You don't duplicate code with these two methods I've proposed. Cause the code itself is much of a glue. It's necessary. Withoout error handling it's more obvious:

protected List<Datacenter> RetrieveDatacenters(VimClient vimClient)
{
    List<Datacenter> result = new List<Datacenter>();
    List<EntityViewBase> appDatacenters = vimClient.FindEntityViews(typeof(Datacenter), null, null, null);
    foreach (EntityViewBase appDatacenter in appDatacenters)
    {
        Datacenter thisDatacenter = (Datacenter)appDatacenter;
        result.Add(thisDatacenter);
    }

    return result; 
} 

protected Datacenter RetrieveDatacenter(VimClient vimClient, string dcName = null)
{
    NameValueCollection dcFilter = new NameValueCollection();
    dcFilter.Add("name", dcName);
    List<EntityViewBase> appDatacenters = vimClient.FindEntityViews(typeof(Datacenter), null, dcFilter, null);
    if (appDatacenters.Count > 1) 
    {
        throw new Exception("There can be only one.");
    }
    
    return appDatacenters.FirstOrDefault(); 
}

Tuesday, November 26, 2013 11:45 PM

Ahhh, yes, the power of refactoring and events!  Take a look at this solution which will give any listener just one or more datacenters!  It's up to the listener to decided when enough is enough...

//Other programs will listen for this event
public static EventHandler<Datacenter> OnNewData;
protected List<Datacenter> GetDataCenter(VimClient vimClient, string dcName = null)
{            
    // Get a list of datacenters
    List<EntityViewBase> appDatacenters = new List<EntityViewBase>();
    try
    {
        if (dcName == null)
        {
            // Return all datacenters
            appDatacenters = vimClient.FindEntityViews(typeof(Datacenter), null, null, null);
        }
        else
        {
            // Return the named datacenter
            NameValueCollection dcFilter = new NameValueCollection();
            dcFilter.Add("name", dcName);
            appDatacenters = vimClient.FindEntityViews(typeof(Datacenter), null, dcFilter, null);
        }
        ParseAndNotifyDataCenters(appDatacenters);
    }
    catch (Exception ex)
    {
        // VMware Exception occurred
        txtErrors.Text = "A server fault of type " + ex.StackTrace +  ex.Message + "' occured while performing requested operation.";
        Error_Panel.Visibility = Visibility.Visible;
        return null;
    }
}

Ahh and as I was posting this I saw another refactor opportunity.  See if you can spot it.

JP Cowboy Coders Unite!

Tuesday, November 26, 2013 11:47 PM

And here's the parse and notify:

private static void ParseAndNotifyDataCenters(List<EntityViewBase> appDatacenters)
{
    foreach (EntityViewBase appDatacenter in appDatacenters)
    {
        Datacenter thisDatacenter = (Datacenter)appDatacenter;
        DataReady(thisDatacenter);                
    }
            
}

private static void DataReady(Datacenter thisDatacenter)
{
    if (OnNewData != null)
    {
        OnNewData(this, thisDatacenter);
    }
}

JP Cowboy Coders Unite!

Tuesday, November 26, 2013 11:54 PM

Allright, I couldn't stand it any longer, here's the next refactor.

  • No Duplicate code or even duplicate patterns.
  • Think about delivering cargo as soon as possible no need to build a list here in this case.
  • On entry to methods validate input params, and immediately set state logic.
  • Finally dig deep into Separation of Concerns, no method should do 1 thing more than name implies.  Keep methods short and focused.
internal class DummyClass
{
    //Other programs wilt listen for this event
    public static EventHandler<Datacenter> OnNewData;

    protected void GetDataCenter(VimClient vimClient, string dcName = null)
    {
        // Get a list of datacenters
        List<EntityViewBase> appDatacenters = new List<EntityViewBase>();
        try
        {
            // Return the named datacenter
            NameValueCollection dcFilter = new NameValueCollection();
            dcFilter.Add("name", dcName);
            if (dcName == null)
            {
                // Return all datacenters
                dcFilter = null;
            }                   
            appDatacenters = vimClient.FindEntityViews(typeof(Datacenter), null, dcFilter, null);         
            ParseAndNotifyDataCenters(appDatacenters);
        }
        catch (Exception ex)
        {
            // VMware Exception occurred
            txtErrors.Text = "A server fault of type " + ex.StackTrace + ex.Message + "' occured while performing requested operation.";
            Error_Panel.Visibility = Visibility.Visible;
        }
    }

    private void ParseAndNotifyDataCenters(List<EntityViewBase> appDatacenters)
    {
        foreach (EntityViewBase appDatacenter in appDatacenters)
        {
            Datacenter thisDatacenter = (Datacenter)appDatacenter;
            DataReady(thisDatacenter);
        }
    }

    private static void DataReady(Datacenter thisDatacenter)
    {
        if (OnNewData != null)
        {
            OnNewData(this, thisDatacenter);
        }
    }
}

JP Cowboy Coders Unite!

Wednesday, November 27, 2013 9:51 AM

@JP: without knowing the exact purpose and usage of this pieces of code, it's hard to tell.

But I would assume that our GetDatacenter() is an abstraction layer for the VimClient class. So I would think it makes sense to hide the implementation details. Thus EntityViewBase should not be used anywhere else than in the glue code.

Wednesday, November 27, 2013 10:48 PM

Stefan;

  Agreed there were some puzzling things in the refactor, which you mentioned, EntityViewBase being one of them.  One good question would be, should the data model actually do the casting?  Typically I never cast much in the model layers, but I will on occasion use ObservableCollections :)

JP Cowboy Coders Unite!

Thursday, November 28, 2013 3:00 AM

... is it possible to have a class that returns either a list of items, or when there is only one item, simple return that one item not as a list?

you want to use same/one method name, for 2 different operations?

yes, if the pass-in parameter (signature) of both are different. just use method overloading

List<data> GetData(List<arg> args);

data GetData(arg arg);

  • Last updated on