What would be best practice for this scenario?

Question

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 1-Jul-13 1:37am

SteveBaldwin21

Updated 1-Jul-13 2:30am

v5

Add a Solution

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".

2 solutions

Solution 1

#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.

Posted 1-Jul-13 2:41am

V.

Updated 1-Jul-13 2:46am

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...

SteveBaldwin21 1-Jul-13 14:57pm

"#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."

For what reason would methods be worse than the class constructors for each weed.

Weeds Chicory = new Weeds(...)
Weeds Dandelion = new Weeds(...)
etc..

Thank you

V. 1-Jul-13 15:18pm

More dynamic for starters. If you really wish to use a method to fill in the properties I would call it differently, like setChicory() or something. But methods, IMHO is something you use to DO something. You could argue that setting properties is doing something, but I´m rather talking about performing algorithms. eg. Class car has a property speed, color, etc.. it also has a method Accelerate(...). One would think it just changes the property speed, but not necessarely only that. Accelerating DOES something, perhaps it also changes the throttle, the range (mileage), ... . Hope that explains a bit what I mean. Chicory, in this case looks rather a property than something the weed does. (Grow() would be an example of Weed method)

SteveBaldwin21 1-Jul-13 15:22pm

Perfect example and explanation, thank you.

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

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

Sergey Alexandrovich Kryukov · Accepted Answer · 2013-07-01T02:45:00

Solution 2

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

Posted 1-Jul-13 2:45am

Sergey Alexandrovich Kryukov

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

Sergey Alexandrovich Kryukov 1-Jul-13 18:23pm

1) If there are too many parameters to the constructor, you need to a) use your approach #1, b) review your general design, which is more likely. Perhaps the solution would be migration from your 100% non-OOP code to some OOP. For example, you might have a new class for each "weed". Or for each higher-rank taxon. Are those a plant species? It's way non-biological to call them "weeds". Or is it horticulture stuff?
The final advice could be done only if you describe the application field in sufficient detail.

2) You should not have a single line of repeated code. Again, the final advice could be done only if you describe the application field in sufficient detail.

You really need to review the whole design.

—SA

SteveBaldwin21 1-Jul-13 18:39pm

I have been reading most the afternoon on the DRY and SOLID concepts and will completely review my design.

I thank you very much for your advice and help + the kick up the ass to realise mistakes and learn correctly.

Thank you for your time Sergey.

Sergey Alexandrovich Kryukov 1-Jul-13 18:57pm

You are very welcome.
Good luck, call again.
—SA