CODEWITHSUNDEEP
c#
user11735291
user11735291

Reputation:

Is this the correct way C# set getter and setter on class property?

As you can see under I have create getter and setter for class property. I'm wondering, is it fine to set it up like I did under? I didn't use the private variable in the get, is it fine or should I use it? Could someone correct me how my class in this case, should look like, if my approach is wrong? The class is CloneAble

public int? Id { get; set; }
public string Code { get; set; }

private string _name;
public string Name
{
    get
    {
        if (this.Id == null)
            return null;
        else
            return this.Id+ "/" + this.Code;
    }
    set
    {
        if (this._name != value)
        {
            this._name= value;
        }
    }
}

public bool IsValid
{
    get
    {
        if (this.Id == null)
            return false;
        else
            return true;
    }
}

The class is Cloneable:

public object Clone()
{
    var elem= new Element();

    this.SetPropertyValues(elem);

    return elem;
}

private void SetPropertyValues(elem)
{
    var propertyInfo = this.GetType().GetProperties().Where(p => p.CanWrite && (p.PropertyType.IsValueType || p.PropertyType.IsEnum || p.PropertyType.Equals(typeof(System.String))));

    foreach (PropertyInfo property in propertyInfo)
    {
        if (property.CanWrite)
        {
            property.SetValue(elem, property.GetValue(this, null), null);
        }
    }
}

Upvotes: 0

Views: 1042

Answers (3)

Fildor
Fildor

Reputation: 16049

I think you can get rid of your struggles by simplifying the Clone Method:

// Private parameterized CTOR
private Element( int id, string code ) 
{ 
    this.Id = id;
    this.Code = code;
}

// Simplyfied Clone
public object Clone()
{
    return new Element(Id, Code);
}

Now you can have the readonly Property Name and Clone:

public string Name => $"{Id}/{Code}";

Upvotes: 1

0009laH
0009laH

Reputation: 1990

This code works, but what is the utility of _name? You can set its value but not the opposite. In addition, creating IsValid is not very useful, as Id is a public attribute; you could simply create a method whose value is this.Id != wrongValue and make Id private.

The result would be the following:

private int Id { get; set; }
public string Code { get; set; }

private readonly int IMPOSSIBLE_ID = -1000;

private string _name;
public string Name
{
    get {
        return (this.Id == null) ? null : this.Id+ "/" + this.Code;
    }
    set {
        if (this._name != value)
            this._name= value;
    }
}

public bool IsValid() => return this.Id != IMPOSSIBLE_ID;

UPDATE : now Id is a nullable integer

private int? Id { get; set; }
public string Code { get; set; }

private string _name;
public string Name
{
    get {
        return (this.Id == null) ? null : this.Id+ "/" + this.Code;
    }
    set {
        if (this._name != value)
            this._name= value;
    }
}

public bool IsValid() => return this.Id != null;

Upvotes: 0

Dmitrii Bychenko
Dmitrii Bychenko

Reputation: 186668

First of all int Id can never be null since int is struct; that's why this.Id == null is always true. You can either make Id be nullable:

 public int? Id { get; set; }

or change this.Id == null into, say, this.Id == 0

 // Let Id be nullable 
 public int? Id { get; set; }
 public string Code { get; set; }

Since Name can be easily computed, you don't want _name. Another issue with Name setter: you can assign, say, "bla-bla-bla" to Name then read it again and get something unexpected:

 var demo = MyClass();

 // "0/"
 Console.WriteLine(demo.Name);

 demo.Name = "123/Test";

 // "0/" instead of expected "123/Test";
 Console.WriteLine(demo.Name);

That's why I suggest either to remove the setter:

 public string Name {
   get {
     return Id.HasValue
       ? $"{Id}/{Code}"
       : ""; // often, it's better return an empty string, not null
   }
 }

Or implement some kind of parsing:

 public string Name {
   get {
     return Id.HasValue
       ? $"{Id}/{Code}"
       : ""; // often, it's better return an empty string, not null
   }
   set {
     if (string.IsNullOrEmpty(value)) {
       Id = null;
       Code = ""; // often, it's better return an empty string, not null
     } 
     else {
       string[] parts = string.Split(new char[] {'/'}, 2);

       if (string.IsNullOrEmpty(parts[0])) {
         Id = null;
         Code = parts.Length > 1 ? parts[1] : ""; 
       }
       else if (int.TryParse(parts[0], out int id)) {
         Id = id;
         Code = parts.Length > 1 ? parts[1] : ""; 
       } 
       else 
         throw new FormatException("Name is inincorrect format.");
     }
   }
 }

Finally, IsValid can be shortened into

 public bool IsValid => Id.HasValue;

Upvotes: 0

Related Questions