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?
-
\$\begingroup\$ It may be worth making Continent an enum, since there is a static, finite number of valid values for it. \$\endgroup\$Dan Lyons– Dan Lyons2014年11月12日 01:24:06 +00:00Commented Nov 12, 2014 at 1:24
1 Answer 1
A few things straight away:
You have a class called India
which really just holds a collection of State
s. 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.
-
\$\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– novice2014年11月11日 22:45:02 +00:00Commented Nov 11, 2014 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 calledStateList
that inherits fromList<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 byCountry.Country
. Really, though,PopulateCountry
is not a very useful method. You can much more easily directly createCountry
s as I have. I will add another example. \$\endgroup\$Magus– Magus2014年11月11日 22:56:03 +00:00Commented Nov 11, 2014 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– novice2014年11月11日 23:10:29 +00:00Commented Nov 11, 2014 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– Magus2014年11月11日 23:17:20 +00:00Commented Nov 11, 2014 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– novice2014年11月11日 23:20:50 +00:00Commented Nov 11, 2014 at 23:20