3
\$\begingroup\$

I have written a function to populate values into a class whose structure is as given below

 public class Country
    {
        public string Name { get; set; }
        public string Continent { get; set; }
        public string Zone { get; set; }
        public India IndiaInfo { get; set; }

    }

   public class India
    {
        public List<State> StateInfo{get; set;}
    }

    public class State
    {
        public string StateName { get; set; }
        public string StateCode { get; set; }
    }

The values are populated into the class object as shown below

public void PopulateCountry(string name, string Asia , string GMT , string AbcState, string RichState)
    {
        Country objCountry = new Country();
        objCountry.Continent = Asia;
        objCountry.Name = name;
        objCountry.Zone = GMT;

        **India objIndia = new India();
        objIndia.StateInfo = new List<State>;
        State objState = new State();
        objState.StateCode = AbcState;
        objState.StateName = RichState;
        objIndia.StateInfo.Add(objState);
        objCountry.IndiaInfo = objIndia;**

    }

In the above method to populate the object of class Country I have taken a brute force approach and put the values in the class and the code is giving results too. However this code is not following some of the best practices and can be further enhanced /rewritten especially the one in Bold. Can someone point out the things/features/practices of C# I am missing.

Edit# 1 Magus pointed out some of the conventions i was not following Regarding this particular method, PopulateCountry 1) can we have better methods to instantiate the state object/india object , taking into account their relationship as per class structure 2) the last line of code where i have directly assigned the object,is that fine and according to standards?

\$\endgroup\$
  • \$\begingroup\$ It may be worth making Continent an enum, since there is a static, finite number of valid values for it. \$\endgroup\$ – Dan Lyons Nov 12 '14 at 1:24
5
\$\begingroup\$

A few things straight away:

You have a class called India which really just holds a collection of States. I would suggest not using that class at all, instead creating a List<State> States instead.

Info as a suffix is fairly meaningless, particularly when the object is a collection.

You also do not need to put obj in front of objects you create. In C#, everything is an object, so that means absolutely nothing.

In addition, the names you've chosen for parameters are particularly bad. Asia might be a good value, but continent is a far better variable name.

Last of all, you may want to use object initializers:

public class Country
{
    public string Name { get; set; }
    public string Continent { get; set; }
    public string Zone { get; set; }
    public List<State> Country { get; set; }
}

public void PopulateCountry(string name, string continent, string timeZone, string stateCode, string stateName)
{
    var state = new State
    {
        StateCode = stateCode,
        StateName = stateName
    }
    var country = new Country
    {
        Continent = continent,
        Name = name,
        Zone = timeZone,
        Country = new List<State> { state }
    }
}

This isn't everything you can do. It may make more sense to make a Country class if you plan on adding other things to it. This at the very least makes your code simpler, though.

Alternatively, you could assign a country like this, anywhere:

var country = new Country
{
    Continent = continent,
    Name = name,
    Zone = timeZone,
    Country = new List<State>
    {
       new State
       {
           Name = stateName1,
           Code = stateCode1
       },
       new State
       {
           Name = stateName2,
           Code = stateCode2
       },
    }
}

Or even:

var country = new Country
{
    Continent = continent,
    Name = name,
    Zone = timeZone
}
country.Country = names.Zip(codes, (x, y) => new State { Name = x, Code = y };

Or something close to it, assuming the two are in the correct order. You have many options.

\$\endgroup\$
  • \$\begingroup\$ Thanks Magus for the help. I ll put the suggestions into my code and improve it . One more thing,as one will notice in the class Country the fourth property IndiaInfo is the object of another class India inside which resides a list of object of second class state.Regarding this particular method, PopulateCountry 1) can we have better methods to instantiate the state object/india object , taking into account their relationship as per class structure 2) the last line of code where i have directly assigned the object,is that fine? \$\endgroup\$ – novice Nov 11 '14 at 22:45
  • \$\begingroup\$ As the main text of my answer says, your India class does not do much. You might be able to make a class called StateList that inherits from List<State>, but even that may not be worth it. India is far too specific a name for a class in almost any situation. In my example, India is replaced by Country.Country. Really, though, PopulateCountry is not a very useful method. You can much more easily directly create Countrys as I have. I will add another example. \$\endgroup\$ – Magus Nov 11 '14 at 22:56
  • \$\begingroup\$ The third example is using lambda operator which i am not particularly comfortable with blame my programming knowledge,can i have any sources having examples of the same and their usage. Thanks for the help and actually i am not able to upvote the answer i ll do it as soon as i have the reputation \$\endgroup\$ – novice Nov 11 '14 at 23:10
  • \$\begingroup\$ Generally speaking, you either initialize an object in it's constructor, or in some kind of Initialize method (less common). If you don't want to require it, which is perfectly fine, you can use the object initializers as in my examples. Assigning an object directly should not be a problem, and I'm not sure why you're even asking. \$\endgroup\$ – Magus Nov 11 '14 at 23:17
  • \$\begingroup\$ i was doubtful if it is a best practice to assign object directly as i have done when c# gives you so many options as you have pointed out \$\endgroup\$ – novice Nov 11 '14 at 23:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.