Oh, the joys of legacy. Decisions were made, nobody knows why and now you “just” have to work with it. Today was one of those days.It is not as bad as I’ve seen before (Hello critical Visual Basic 6 code), but still. Having an awesome quality tool like SonarQube regularly scanning your solutions will catch a lot of things you did not think of before. This time, it was a non-standard use of property setter:
Yes, this is weird and not intuitive to have a setter for a calculated property that doesn’t do anything. So, my first instinct was to remove the setter all together, since it is not needed. Great, warning gone, lets move on.
…Or maybe not. On the next item, I needed to make a new migration, and to my surprise the following lines were included:
public override void Up()
{
DropColumn("CM.tbl_ProcesstatusBenaderstap", "AantalNietInAanmerking");
}
Oops. The properties are actually stored in the database (one of those decisions…), so this is not what I want. Apparently Entity Framework Code First migrations really needs the setter to determine that yes, we really want to store this value in the database.
SonarQube gives us the tip to use throw new InvalidOperationException() in the setter, so let’s try that solution. Yay, migration allows it and the column is no longer dropped. However, when retrieving the entity from the database, the exception does get thrown… so unfortunately it’s not an option.
OK, moving on. Maybe there is a way to signal to Entity Framework that yes, we do want to store this variable, but not have a setter. Like using the inverse of the [NotMapped] attribute. Maybe using the [Column] attribute on the property works? But alas, the column is still removed in the migration.
How about using backing fields? Sure, its actually a thing for Entity Framework Core, but we can build our own version, right?
private int _aantalNietInAanmerking;
public int AantalNietInAanmerking
{
get
{
return this.AantalNietGeselecteerd + this.AantalGeblokkeerd;
}
private set { this._aantalNietInAanmerking = value; }
}
Ok, that looks like something, the column still exists in the migration. But now I’m getting warnings about a private field that should only be used as a local variable. Plus, it still looks like a code smell, having a private field just for storing the value that was in the database. The longer I look at it, the more I dislike it. What is the most elegant solution? I need the setter to be there for Entity Framework, but I don’t want anybody to actually use it. So I made the following compromise:
public int AantalNietInAanmerking
{
get
{
return this.AantalNietGeselecteerd + this.AantalGeblokkeerd;
}
private set
{
// Set is nodig voor migratie
}
}
I guess I can live with this now. The warning is still there in SonarQube, but the code is only actually usable in the constructor and by EntityFramework, so I think it will be good enough for now.
(Also, please don’t roast me for the Dutch language used in the code. Another one of those requirements…)