C# Encapsulation – Violation of YAGNI Principle

cencapsulation

Consider these two ways to declare a field in a C# class

option A

public class AuditController
{
    public DataAccess Service;
}

option B

public class AuditController
{
    private DataAccess service;
    public DataAccess Service { get => service; set => service = value; }
}

Which one is better? My code analyzers (sonarqube, resharper, etc) unanimously say B is better. But I disagree. I can see that B encapsulates the field, so that a person can later add validation and logging, etc. However, why not wait until you need it?

This is the YAGNI principle: you can add the getter/setter now, or wait until you need it. Since there is no penalty for waiting, you should wait. Because most likely… you ain't gonna need it.

Am I missing something?

I'm not asking whether getter/setters provide any benefit of encapsulation. I'm asking whether it is best practice to proactively add getter/setters that are doing nothing other than exposing the data.

Best Answer

There are two completely different kinds of software: libraries that need a stable binary interface across multiple versions, and applications or internal software where you can just refactor.

For internal software or applications, you are right: you can wait until something is needed, then refactor and recompile your code. So using public fields is jucky but fine.

A public^[1] library doesn't have this liberty. If you change a field to a property that is a breaking change – a property is basically a method with prettier syntax. This change is source-compatible (stable API) but not binary-compatible (unstable ABI). Any dependent code that accesses this field/property has to be recompiled.

^{[1]: Here, “public” means “consumed by developers outside of your team”.}

A lot of design advice that you see (SOLID, design patterns, …) is focused on keeping your software evolvable while still keeping ABI compatibility. This isn't bad advice, it's just not always applicable. YAGNI is the complete opposite because it assumes that the design can be fixed by refactoring. Again not bad advice, just not always applicable either.

This is not a free pass for ignoring any design advice, just a pointer that design advice tends to assume a particular context.

But even ignoring the necessity of using properties, you should always use them:

Properties are idiomatic C# code. Not using them makes your code more complicated to read.
They are not that more code to type. Deal with it.
Public fields in themselves are a design smell, and reek of insufficient object modeling for anything more complicated than a DTO. Properties can be publicly gettable and privately settable, which is often the better choice.
All databinding technologies (ASP.Net MVC Model Binding, WinForms, WFP, Xamarin, Asp.Net WebAPI, …) will only databind to properties.
Some cases need properties, so you might as well use them anywhere. The downsides – a tiny bit more typing, in rare cases a tiny performance hit – usually don't outweigh the increased consistency.

Type safety

Lets compare two bits of code

String foo(Integer a, Integer b, String s) {
  if(a.compareTo(b) == 0) {
    return s;
  } else {
    return "" + (a - b); // just as an example
  }
}

String foo(Map<String, Object> m) {
  if(m.get("a").compareTo(m.get("b")) == 0) {
    return (String)m.get("s");
  } else {
    return "" + (((Integer)m.get("a") - (Integer)m.get("b"));
  }
}

In the second example, we have to use Object as the value because its the only object that String and Integer share in common. This means that all objects further down need to be cast. This has the distinct possibility of throwing a ClassCastException (javadoc) anywhere or everywhere in your code... unless you add a ton of boiler plate code to prevent that (guard conditions for instanceof all over).

Whats worse, is that those exceptions (if you don't check everything - and the paths you follow if you do) are runtime errors. The errors of your types won't be found until you run the program rather than when you compile it. Errors found when you compile are easier to fix than ones you discover at runtime. For that matter, the static analysis tools and inspections that most IDEs give you will catch them for you as you type them (and throw up horrendous warnings about trying to do it with the abstract data type).

Overloads

Passing all of the arguments as a single ADT means you have a single method. In the example above, what if you had:

String foo(Integer a, Integer b) { ... }
String foo(Integer a, Integer b, String s) { ... }
String foo(Integer a, Integer b, String s, String t) { ... }

as different ways to call the function. With passing one ADT, you've got just

String foo(Map<String, Object> m) { ... }

There is no way to differentiate the overloads. This leads to code that will look like:

String foo(Map<String, Object> m) {
  if(m.containsKey("t")) { ... }
  else if(m.containsKey("s")) { ... }
  else { ... }
}

The complexity of the function will go through the roof (unless you start extracting those to other methods that are somehow indicating what type they're dealing with and my head is starting to hurt thinking about the hungarian notation you're going to be sticking into the method names).

This further gets problematic when you have different types that are valid arguments.

String foo(Integer a, Integer b, String s) { ... }
String foo(BigInteger a, BigInteger b, String s) { ... }

Typo safety

Long ago, I was a perl programmer - back in the days before OOP found its way into perl (bless its reference). The only way to pass around complex objects was with %hashes, and @arrays. And you had to pull out the keys of the hash. There were numerous bugs that could only be caught at runtime (and the joys of autovivication made that challenging at times).

A simple typo in a key would mean some parameter was there wasn't found (when it was there) or was just created in the wrong spot and passed to another method.

String plusOne(Map<String, Integer> m) {
  if(m.containsKey("type") && m.containsKey("1") {
    return m.get("typo") + m.get("I");
  }
  return 0;
}

These are not fun bugs to have to find. No bugs are really fun, but these bugs just smack you in the face because they are completely preventable if you had just used a proper parameter list and work with the language and the compiler.

Calling Packaging

So far, I've talked about the dangers in the method itself. What about work that the callee has to do.

System.out.println(foo(1,2,"bar"));

And we're done with the simple parameter list.

Map<String, Object> m = new HashMap<String, Object>();
m.put("a", 1);
m.put("b", 2);
m.put("s", "bar");
System.out.println(foo(m));

Now, you want to debug this? What are you passing into foo? You've got to look back through the code and track it. Besides being a significantly larger block of code to do any method call of this type, its also obscufcating what is going into this.

Refactors

foo now takes Doubles rather than Integers. You change the method signature and fix all the compile time errors. You've found them all and it compiles correctly (see type safety above).

However, the map version works just as well with Integer as it does with Double. You can't find out if you've fixed the refactoring or not.

Why are they in a `Map` together?

This is more a philosophical question. Why are these objects in a map together? What common attribute do they share? If they really are part of some other data structure

class Cell {
  int x, y;
  String value;
}

make them into a proper data structure that sticks together. If they aren't things that are honestly related to each other, well... don't.

Putting things together in some structure makes them related to each other in our mind, even if they aren't. If they aren't this adds significant mental gymnastics in order to keep what things are related to each other and what ones aren't apart.

C# – Better Options Than Friend Class in C#

One way of priveliging OperationsService over the rest of the system in regards to specific data is directly passing such data to it on a need to use basis: You could keep your config manipulation logic inside OperationsService and add a public AcceptOperations() method in StorageService that takes an OperationsService object as an arg and calls upon its relevant config manipulation logic, directly passing to it any privately held relevant config data.

Concretely, you could still have GetConfigPropertyByNameAndDoSomeMagicWithIt() be a member of OperationsService, that would call AcceptOperations() on the relevant StorageService instance and pass this as an arg. AcceptOperations() itself would call some concrete ManipulateConfig() method on the OperationsService instance given to it as a param, passing the internal config data to be manipulated as args for ManipulateConfig().

This proposal takes in part some resemblance to the classic OOP Visitor Pattern, excluding the Double Dispatch idiom but keeping the concept of restrictively defining who can visit what and definitely in keeping with the original idea behind the pattern which is to perform such separation of responsibilities such that "... [it provides] the ability to add new operations to existent object structures without modifying the structures." The whole idea of such a pattern is to aid follow the Open-closed principle which is one of SOLID principles, keeping of which you have mentioned to be a concern of yours to begin with.

Best Answer

Related Solutions

Java Encapsulation – How to Encapsulate Method Parameters