Click here to Skip to main content
15,877,966 members
Please Sign up or sign in to vote.
0.00/5 (No votes)
See more:
What would be the best practice for using these properties. I want the Weed Items (Chicory etc..) to be encapsulated within the Weed class and be accessed through my Windows Form (Weeds_Data_Window).


C#
   public partial class Weeds

{       private string name;
        private string edible;
        private string regularity;
        
        public string Name
        {
            get { return this.name; }
            set { this.name = value; }
        }
        public string Edible
        {
            get { return this.edible ; }
            set { this.edible = value; }
        }
        public string Regularity
        {
            get { return this.regularity; }
            set { this.regularity = value; }
        }
}


Example 1:

C#
public partial class Weeds
{
public void Chicory()
{

    Weeds Chicory = new Weeds();
    Chicory.name = "Chicory";
    Chicory.edible = "Yes";
    Chicory.regularity = "Moderate";

Use:
public partial class Weeds_Data_Window : Window
{
        Weeds cWeeds = new Weeds();
        private void CollectWeeds()
        {
            if (list_collection.SelectedItem != null)
            {
                if (list_collection.SelectedItem.ToString() == "Chicory")
                {
                    cWeeds.Chicory();
}
}


Example 2:

C#
public partial class Weeds
{ public void Chicory(string name, string edible, string feed)
{
    Weeds Chicory= new Weeds();
    Chicory.Name = name;
    Chicory.Edible = edible;
    Chicory.Regularity = feed;
}
}


Use:

C#
public partial class Weeds_Data_Window : Window
{

Weeds cWeeds  = new Weed();
private void CollectWeeds()
{
    if (list_collection.SelectedItem != null)
    {
        if (list_collection.SelectedItem.ToString() == "Chicory")
        {
            cWeeds.Chicory("Chicory","Yes","Moderate");
}



I would like to ask for some advice for which of these would be better to use (if any, if there is another better solution please suggest).

Any help is greatly appreciated! Thank you.
Posted
Updated 1-Jul-13 2:30am
v5
Comments
lukeer 1-Jul-13 7:51am    
1) Should Chicory be a special case of Weed?
2) Edible and Regularity should not be of type string. Better use bool[^] for the former and an enum[^] for the latter.
SteveBaldwin21 1-Jul-13 7:56am    
Sorry might not of been so clear; Properties and Chicory() are within (Public Class Weed) and the Use examples are within the Windows Form.

Edible and Regularity will contain more information but for these examples I just kept "Yes" and "Moderate".

The problem of your code is not in the differences you've demonstrated, but in other parts.

First of all, your field declarations are redundant. You should better use auto-implemented properties (as far as I remember, with C# v.3 or later):

C#
public partial class Weeds {
    public string Name { get; set; }
    // ...
}


Also, if this class or these properties are only used inside the same assembly, consider using internal instead of public. Same goes about your constructors. Besides, if you initialize the backing fields in a constructor (your second option, which is perfectly fine), consider keeping setter private, as the constructor might be the only place where you would allow to modify the fields:

C#
public partial class Weeds { // or internal
    
    public void Chicory(string name, string edible, string feed) { // or internal
        this.Name = name;
        //...
    }

    // only reading is (public or internal):    
    public string Name { get; private set; }  // or internal instead of public

   // ...
}


So, the choice between having the fields initialized in constructor and letting the properties read/write is defined by semantic of your code, design of it from the convenience point of view and other design consideration. There is not a one clear-cut decision; either way could be fine.

And, finally, the <bbiggest> problem of your code is the use of hard-coded immediate constants like "Chicory". I cannot see situations when it can be potentially used, except some preliminary version existing only during development and first tests. You should use something else, depending on your goals. In particular, it could be enumerations, possibly with resources (please see my article Human-readable Enumeration Meta-data[^], for a clear example), but it could be something else. Hard-coding as you demonstrated it is simply not acceptable.

—SA
 
Share this answer
 
Comments
SteveBaldwin21 1-Jul-13 13:36pm    
Thank you for your reply Sergey, I understood some of your reply but found the last part quite confusing.

"Hard-coding as you demonstrated it is simply not acceptable."

If you have the time please could you point me or explain with an example of hard-coding and how I am using it also why it is unacceptable.

Again thank you for your time.
Sergey Alexandrovich Kryukov 1-Jul-13 15:07pm    
Here, "hard-coding" means writing immediate constants in code, instead of calculation those strings from types, explicit constants, resources, etc.

Imagine you decided to rename "Chicory" to something else. How are you going to support it? The string is mentioned in more than one place in your project. You should never eve allow such thing, otherwise you will get lost. Each constant should appear in code once and only once. (1, 0, null are apparent acceptable exclusions.) Please see:
http://en.wikipedia.org/wiki/Don%27t_repeat_yourself.

Actually, this is the most fundamental problem of your code.

—SA
SteveBaldwin21 1-Jul-13 15:43pm    
This made sense also, again thank you for taking the time to answer my question. I have become a bit stumped on how to approach this after this information.
Sergey Alexandrovich Kryukov 1-Jul-13 15:56pm    
Well, you are welcome to ask some follow-up questions. Anyway, will you accept this answer formally (green button)?
—SA
SteveBaldwin21 1-Jul-13 16:10pm    
I would love to, but only if you have time to answer it would be greatly appreciated.

1. My concern with using the class constructor is that, if I increase the amount of parameters on the constructors say for an image, this is going to be quite a lengthy code for each weed purely in the constructor parameters, Seeing as there could 100+ weeds.

2. DRY problem; for each of the weeds, I have the properties passed to a textblock in the form, which results in a large number of repetitive code. What could I do to avoid this? Could a solution be;

For the list_box conditional statements override the properties with 1 instantiation..

Weed WeedItem = new Weed(...)

but then it is still repeating everything again...

Hope this made sense.

Thank you
#1: Make sure a property is never called "Name" because it can clash with a standard class property. (you could call it WeedName eg.)
#2: I would most certainly NOT use a method "Chicory" because it does not describe what it does and you would get a method for each new weed.
#3 you can use different methods inside the Weed class eg a default constructor and a override constructor.
#4 Edible is Yes/No ? use a boolean.
#5 Depending on how you want to use it, you can also make an interface or abstract class Weeds and an inherited class Chicory.

Perhaps you can

keep the default constructor
C#
public Weeds(){ 
    // init variables to defaults if necessary
}


override the default constructor
C#
public Weeds(string name, string edible, string regularity){ 
    // init variables to argument values 
}


use the properties
C#
public string WeedName
{
    get { return this.name; }
    set { this.name = value; }
}
public string Edible
{
    get { return this.edible ; }
    set { this.edible = value; }
}
public string Regularity
{
    get { return this.regularity; }
    set { this.regularity = value; }
}


all in one class. usage would then be:
or
C#
Weeds chicory = new Weeds();
chicory.WeedName = "chicory";
chicory.Edible = "Yes";
chicory.Regularity = "Moderate;


or
C#
Weeds chicory = new Weeds("chicory", "Yes", "Moderate");
//if necessary one could override the properties.



hope this helps.
 
Share this answer
 
v5
Comments
SteveBaldwin21 1-Jul-13 8:50am    
Thank you for the this reply, very informative.

"#4 Edible is Yes/No ? use a Boolean"
Edible - "Yes, Stems Only" or "Yes, Flowers Only" for example.

Thank you again.
Sergey Alexandrovich Kryukov 1-Jul-13 8:58am    
No! Use Boolean! If you want its value to produce English statement, format it properly. Never use strings representing data instead of data.
OK, I criticized many items of this answer, so I advise you not to follow them. And please see my answer.
—SA
V. 1-Jul-13 8:53am    
You're welcome :-).
Sergey Alexandrovich Kryukov 1-Jul-13 8:56am    
Let me review it.

Advice #1 is a pure bogus. There is nothing special with "Name". If it is semantically fit, only "Name" should be used, never avoided. Very, very, very bad advice. I'm not going to vote 1 for it, but you almost deserve it.

#2: Of course you are right.

#3: There is no such thing as override constructor on .NET; just think about it.

#4: Of course you are right.

#5: Who knows; I would say, the code does not make enough sense to advice anything like that. You could add too many advice like that with the same success.

The advice to keep the default constructor is poor bogus. You only need it when someone uses it. And adding another constructor does not remove the default one. The way to disable default constructor is adding a private parameterless constructor. And it may make perfect sense. For example, for this reason: don't allow uninitialized properties. And "there are more than one way of doing a thing" is not always the best principle (however, I don't want to be overly Pythonic: it really depends).

Properties don't use auto-implement feature, so, this is redundant code.

Thank you for understanding.

—SA
V. 1-Jul-13 8:59am    
Sergey, play nicer please...

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900