Developing a Products Class with a nested class .Net

Question

I am looking for help in determining if the class model that I am building can be improved upon. The class that I am building is a simple Product class with a few attributes.

class clsProducts
{
    private string _name;
    private double _productionRate;         

    //Constructor
    public clsProducts()
    {
        _name = "null";
        _productionRate = 0.0;           
    }

    public clsProducts(string name, double productionRate)
    {
        _name = name;
        _productionRate = productionRate;          
    }

    //Properties
    public string Name
    {
        get { return _name; }           
    }

    public double ProductionRate
    {
        get { return _productionRate; }           
    }

}

What I would like to add is the ability to have the monthly forecasted values for each product in the class. I could add the following to do this

private double _janValue;
private double _febValue;

and so on, but this seems messy. I also considered creating a nested class called ForecastValues, such as

class clsProducts 
{
...code here....

   protected class ForecastValues
   {
       private string name;
       private double forecastValue;

       ...other code.....
   }
 }

however, I am not sure that this idea would even work. Can any one suggest a way for me to handle this cleanly?

Thank you

Answer 1

I don't think nested classes are a great idea. What I would do is create an additional class 'ForecastValues' but mark it as 'internal protected'. That way you can use it within your assembly but users of your code will only be able to reference it when it contains values.

-Shaun

Answer 2

A few things here.

1. I would recommend removing the cls hungarian prefix from the class name.
2. Depending on exactly what your "ForecastValues" are. You could make a property on the "Product" class that is a List, or possibly a Dictionary. My guess is that you might be able to go the dictionary route with ease.

Answer 3

This is what I would do,

class ClsProducts
{
    //Constructor
    public ClsProducts()
    {
        Name = "null";
        ProductionRate = 0.0;
    }

    public ClsProducts(string name, double productionRate)
    {
        Name = name;
        ProductionRate = productionRate;
    }

    //Automatic properties with private setters
    public string Name { get; private set; }
    public double ProductionRate { get; private set; }

    //since you basically have key value pair, why not use one?
    public KeyValuePair<String,Double>   Forcast{ get; set; }
}

Answer 4

I would suggest just to use an array and an indexer.

public enum Month
{
    January =  1, February =  2, March     =  3,
    April   =  4, May      =  5, June      =  6,
    July    =  7, August   =  8, September =  9,
    October = 10, November = 11, December  = 12
}

public class Product
{
    private readonly String name = null;
    private readonly Double productionRate = 0.0;
    private readonly Double[] productionRateForcast = new Double[12];

    public Product(String name, Double productionRate)
    {
        this.name = name;
        this.productionRate = productionRate;          
    }

    public String Name { get { return this.name; } }
    public Double ProductionRate { get { return this.productionRate; } }

    public Double this[Month month]
    {
        get { return this.productionRateForcast[month - Month.January]; }
        set { this.productionRateForcast[month - Month.January] = value; }
    }
}

I am not sure if month - Month.January requires an explicit cast to Int32. Alternativly one could start with January = 0 but this seems a bit odd, too.

I did also some code changes. I removed the default constructor, because I see no value in a Product instance with "uninitialized" fields and no possibilty to alter them later. In consequence I made the fields readonly, too. Finaly I removed the Hungarion notation prefix - this is a quite an outdate coding style - and turned Products into Product because it represents one product not a collection of products.

UPDATE

To catch up the dictionary idea .... I will just give the required changes.

private readonly IDictionary<Month, Double> productionRateForcast =
    new Dictionary<Month, Double>();

public Double this[Month month]
{
    get { return this.productionRateForcast[month]; }
    set { this.productionRateForcast[month] = value; }
}

This might be a even cleaner solution then using an array. You could also just expose the dictionary through a property instead of having an indexer, but I consider the indexer a cleaner solution because it hides some implementation details.

public IDictionary<Month, Double> ProductionRateForcast
{
    return this.productionForecast;
}

In all case the usage would be as follows.

Product myProduct = new Product("Great Product", 0.8);

myProduct[Month.August] = 0.7;

This looks quite odd. One could try adding a IndexerNameAttribute to the indexer, but I am not sure if this would allow to write

myProduct.ProductionValueForcast[Month.August] = 0.7;

in a language with indexer support. So I finally tend to change my mind and prefer exposing the dictionary by a property if the IndexerNameAttribute does not help.