13

I have a property in my class that has a lot of logic in set accessor:

private String text;

public String Text
{
   get { return text; }
   private set
   {
      // some actions with a value
      value = value.Replace('a', 'b');

      text = value;
   }
}

How can I prevent other developers (or even me) from changing field instead of property inside of this class ?

If somebody writes something like this:

public Test(String text)
{
   this.text = text;
}

it will break the logic of my class !

Pierre Arlaud
  • 4,040
  • 3
  • 28
  • 42
eden_lane
  • 108
  • 1
  • 11

5 Answers5

19

Normally, classes should be small enough that this shouldn't be an issue. Since the field is private, only code inside the same type will have access to the field. However, if you need to highlight the importance, you could do:

[Obsolete("Use the Text property, or I break your legs; fair warning")]
private string text;

public string Text
{
#pragma warning disable 0618
    get { return text; }
    private set
    {
        // some actions with a value
        value = value.Replace('a', 'b');

        text = value;
    }
#pragma warning restore 0618
}

This doesn't stop them, but it may help prevent accidental usage of the field.

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • 5
    `or I break your legs (or mine)` (as he wanna prevent himself too) – Raphaël Althaus Apr 14 '14 at 06:58
  • Haha. It is great solution. It's not completely solve the problem, but I don't think there is a way to do it better then this, so I will mark this answer as correct. Thanks ! – eden_lane Apr 14 '14 at 07:04
  • 2
    @edencore side note: you could probably use Roslyn to implement an exact "find all illegal field usage and warn about them" - the problem, though, is defining "illegal field usage" vs "legal field usage" – Marc Gravell Apr 14 '14 at 07:22
  • Another (equally ugly) approach would be to rename the field, for example `private string textNoSanitationWhenWrittenToDontUseWeWillBreakYourLegs;`. – Jeppe Stig Nielsen Apr 14 '14 at 07:43
  • 1
    @MarcGravell: indeed, there is one function `set` that is privileged since it is permitted to modify `text` and no other function is. One further function `get` is permitted to read but not write. That's the concept to be captured, but I don't know the state of the art in expressing that for static code analysis to enforce. Other than Binary Worrier's answer, of course, which expresses it neatly in language the compiler already understands. – Steve Jessop Apr 14 '14 at 09:39
  • @Marc: With the utmost respect (and you do indeed have my utmost respect mate), see my comment on this answer of an old duplicate of this question :) http://stackoverflow.com/a/1299268/18797 – Binary Worrier Apr 14 '14 at 10:13
  • 1
    @BinaryWorrier I followed that link hoping for an explanation rather than just "you should be shot"... – Rawling Apr 14 '14 at 11:15
  • @Rawling That's why it's funny – Binary Worrier Apr 14 '14 at 11:25
  • @BinaryWorrier somebody who is touching the internals of the same class is already implicitly inside the usual protection barriers... – Marc Gravell Apr 14 '14 at 11:36
  • Grand, but I still think this is a bad way to prevent the field from being accessed directly. The warning is only useful if 1) the dev checks the warnings 2) decides to heed the warning. I wouldn't use a _hint_ to get you to do the right thing, when I can *force* you to do the right thing. If I thought every developer would look for and heed the warning, I'd probably trust them to look an inteli-sense comment. `/// Use Text property, not this field` would very nearly be as effective. I'd get the compiler to enforce this, not issue hints. My 2 cents. – Binary Worrier Apr 14 '14 at 11:47
9

There is a correct way to do this. Consider the old adage "All problems in computer science can be solved by another level of indirection".

What you do is create a class that knows how to set the value of the text field properly.

class TextSetter
{
    private string text;
    public TextSetter(string text)
    {
        Text = text;
    }
    public string Text
    {
        get{ return text;}
        set{ text = value.Replace('a', 'b');}
    }
}

Then in your first class instead of private string text; you have private TextSetter text; now there is never any chance that someone will accidentally set the value directly.

you could even generalise this, if it's a common problem

class FieldSetter<T>
{
    private T field;
    private Func<T, T> setter;
    public FieldSetter(Func<T, T> setter, T field)
    {
        this.setter = setter
        Value = field;
    }
    public T Value
    {
        get{ return field;}
        set{ field = setter(value);}
    }
}

Feel free to rename as you see fit.

Binary Worrier
  • 50,774
  • 20
  • 136
  • 184
  • Not great in terms of allocations, though. – Marc Gravell Apr 14 '14 at 09:48
  • This is true, however it is _just another allocation_. If one more allocation on the class is going to break your heart you can make the class a struct and then you're back to a single allocation for Setter and the string reference `text`. 99% of the time, for 99% of the software folks write, the extra allocation matters less than keeping the intent of the code honest. We don't all suffer [Assult by GC](http://marcgravell.blogspot.ie/2011/10/assault-by-gc.html), few of us work with StackOverflow type volumes :p – Binary Worrier Apr 14 '14 at 09:56
  • . . . with the obvious caveat that you'd need to implement the struct properly (i.e. as an immutable type), which means you're back to an allocation for every assignment . . . maybe don't make the class a struct. – Binary Worrier Apr 14 '14 at 10:17
  • creating a new struct each time is not an allocation; the size would be the size of the contained data, presumably a single `string` reference - so no larger than the original – Marc Gravell Apr 14 '14 at 10:36
  • So something like `this.myDate = new DateTime(2014, 04, 14);` doesn't cause an allocation? This I did not know. It looks like it does, the `new` to create a new struct, which would then be copied over the old. I may look into this and see how it actually works, thanks. – Binary Worrier Apr 14 '14 at 10:42
  • In the case of `new SomeStruct(with args)`, the constructor simply writes over the top of the existing point in memory (which is why it is required that a struct constructor assigns to all fields). In the case of `new SomeStruct()` (no args), it simply zeros the memory. In neither case is there an allocation. The struct is inherently part of the containing scope: it is already allocated. – Marc Gravell Apr 14 '14 at 10:47
  • I added a separate answer to illustrate the `struct` usage more completely – Marc Gravell Apr 14 '14 at 11:04
6

As an alternative approach, building on the discussion with Binary Worrier (comments), you could do something like:

internal struct SanitizedText {
    private readonly string value;
    private SanitizedText(string value) {
        this.value = value;
    }
    public static implicit operator string (SanitizedText value) {
        return value.value;
    }
    public static implicit operator SanitizedText(string value) {
        if (value != null) value = value.Replace('a', 'b');
        return new SanitizedText(value);
    }
}

and then simply:

private SanitizedText text;
public string Text {
    get { return text; }
    set { text = value; }
}

This does all the same validation, but it is impossible to abuse: you cannot create a SanitizedText value that bypasses the validation; equally, it takes no more space (the size of the struct is the size of the contents: 1 reference), and requires no allocations. It even defaults to a null string in the same way.

Community
  • 1
  • 1
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • +1 from me! (extra characters) – Binary Worrier Apr 14 '14 at 12:08
  • I would like this approach if there were some way to have a type include an "eager" promotion operator such that if compilation would fail with a type "as-is", the compiler should see what happens if it's eagerly promoted, so as to allow `someSanitizedString.Length`, or `someOtherTypeConvertibleFromString = someSanitizedString;` rather than `((string)someSanitizedString).Length` or `someOtherTypeConvertibleFromString = (string)someSanitizedString;. Without such a feature, attempts to make a type behave like a built-in type end up creating very leaky abstractions. – supercat Apr 14 '14 at 15:37
  • @supercat (late comment): since the property `Text` (that you are only supposed to be using) returns `string`, then `Text.Length` is fine. If you use `someSanitizedString.Length`, you are using the backing field and that is what is being prevented. – NetMage Aug 17 '21 at 20:56
  • @NetMage: My point was that it would be useful if one could use a field of a string-like type as though it was a string, without having to manually access the `Text` property. – supercat Aug 17 '21 at 21:07
  • @supercat Except the whole point of this is to disallow access to the field. If you want a string-like type that can be used as a string, that is a different issue entirely, and perhaps instead of a wrapper class, you should inherit from `String`. OTOH, if you want to complain that wrapper classes are too much work to create, that is an issue for C# development. – NetMage Aug 18 '21 at 15:10
  • @NetMage: Beyond the question of whether something is a field or property, I think the idea I was getting at was the possibility of having a property that returns a low-cost wrapper object which could construct a potentially-expensive object on demand. For example, one could have a `ConcatenatedString` object which holds a `String[]` where elements 1..N hold strings that were concatenated, and element 0, though initially null, will receive their concatenation the first time any code tries to use it as a string. Attempting to concatenate multiple such strings, however,... – supercat Aug 18 '21 at 17:23
  • ...wouldn't need to create any actual `String[]` objects unless or until one tried to use the `ConcatenatedString` in a manner that would require a `String`. – supercat Aug 18 '21 at 17:23
5

What you did already by setting the modifier to private for the field is as far as you can go. Within the class you should generally know what you do with its fields, where the logic happens.

fixagon
  • 5,506
  • 22
  • 26
1

You could use collections (one per type) as "backing storage":

private static readonly Dictionary<string, int> __intFields = new Dictionary<string, int>();
private static readonly Dictionary<string, string> __stringFields = new Dictionary<string, string>();

public static string StringProp
{
    get
    { 
        var fieldValue = default(string);
        __stringFields .TryGetValue("StringProp", out fieldValue);
        return fieldValue;
    }
    set
    {
        var manipulatedValue = applyOtherCode(value); // the rest of the code
        __stringFields ["StringProp"] = manipulatedValue
    }
}

public static int IntProp
{
    get
    { 
        var fieldValue = default(int);
        __intFields .TryGetValue("IntProp", out fieldValue);
        return fieldValue;
    }
    set
    {
        var manipulatedValue = applyOtherCode(value); // the rest of the code
        __intFields ["IntProp"] = manipulatedValue
    }
}
// There is no field for anyone to mistakenly access!

Ugly, verbose, un-refactorable, ... but it gets the job done. If performance is the last of your concerns (if it even is one), you could grab the property name via Reflection and avoid magic strings.

I originally had a single <string, object> collection ... but I hated it. One per type avoids casting data all the time

Alex
  • 23,004
  • 4
  • 39
  • 73
  • Ugly, yes. In the getter, you should consider `TryGetValue` on the dictionary to not look for the key twice. In the setter you could just say `__backingFields["Prop"] = manipulatedValue;` with no `if` since that will replace or add as needed. The field could be made `readonly`. After these changes, it will still be very ugly, especially because of the cast from `object` to `string`. – Jeppe Stig Nielsen Apr 14 '14 at 08:09
  • Optimized. I also was hating that string to object cast and I changed the code to make it one-dictionary-per-type (this is starting to look like it would fit on codegolf ...). Cheers. – Alex Apr 14 '14 at 08:19