Open linkdotnet opened 3 years ago
Hi Steven,
The addition of this type is a good idea. I was thinking of that too -- I'm using something similar in my projects as well.
A couple of comments regarding the implementation:
int
, long
, string
)Id
or Key
(generic) and Name
(always string). This is similar to C# enums that also effectively have 2 fields: the underlying value (Id
) and the name of the enum. A one-field enum is fine too -- it may inherit from the 2-field enum such that it would set both the Id
and the Name
to the same value (of type string
)Create
method ( https://github.com/linkdotnet/EnumValueObject/blob/master/LinkDotNet.EnumValueObject/EnumValueObject.cs#L76 ) need to return Maybe
instead of Result
. It's the responsibility of the client code to decide how to interpret the missing value. I think it's also better to name this method FromId
instead of Create
GetEqualityComponents
PopulateOnLoadingFromDb
method is a good idea. It's best to use integration tests to tie the enum values to the values in the database.Hey Vladimir, thanks for the response and the points you mentioned.
PopulateOnLoadingFromDb
is more of a legacy code from a recent project and should not be used anymore. Definitely not in a more general library like yoursMaybe
instead of a Result. But if you take the general approach: "Maybe is used to verify value, Result verifies the result of an operation" then it should be Maybe
Maybe to see why I only used a string
as key / mandatory field. It is true, that normal enums
have an underlying id and a name. The question is, how I as developer use enums. And normally I prefer the readable string-notion rather than having something like MyEnum operation = 2;
.
The current implementation still allows the freedom to add an underlying Id (and add a custom static CreateFromId method)
public class Color : EnumValueObject<Color>
{
public static readonly Color Red = new Red("red", 1);
public int ColorId { get; private set; }
private Color(string key, int colorId ): base(key)
{
ColorId = colorId;
}
public static Maybe<Color> FromColorId(int colorId)
// ...
}
This tailor made solution allows to have better naming instead of a generic Id Property.
In my projects string
as identifier was always enough (I never added an Id
field). But that is just what I experienced and doesn't necessarily translate well when putting into a library.
Do you have an example to demonstrate where an additional mandatory Id
field would be helpful?
Maybe interfaces with external systems could be one use-cases. (Even though my example from above would work here as well).
Then I would also provide two functions to create the ValueObject: FromName
and FromId
.
Greetings Steven
The main use case for the custom Id field type is to be able to persist it to database. For example, let's say that a Customer
table has a CustomerType
field. It's possible to make it a string
, but that would require more space on the disk ("Regular", "Preferred" as opposed to 1, 2). Also, DB indexes don't work as fast with string data as they do with numbers.
Then I would also provide two functions to create the ValueObject:
FromName
andFromId
Yep, this will be needed, indeed
Hmm I see your point. For me it would be more an edge case than the normal case. My proposed solution would also work here as the mapping is under your control.
I am totally fine if you see the Id solution as favorable. As you initial proposed I can create a PR with two EnumValueObjects. Right now I am not sure if one should inherit from the other or if they should be two distinguished types
How does that sound to you?
Sounds great. You can also submit just 1 class if you are short on time, the one that you already have written. I plan to spend some time actively working on the library in 1-2 months, I'll be able to add the 2nd class then.
I created an initial "draft-like" PR. Some Unit Tests are missing. If you disagree the current concept I would go with your former recommendation to only merge the "simpler" one.
One comment: I'd rename Key
to Id
in the first class too, for consistency.
Looks good. The inheritance can be sorted out later.
If I am not mistaken, there is nothing in the proposed approach to prevent duplicate keys (other than an eventual exception on SingleOrDefault calls). Consider a static Dictionary that gets populated from a factory that will throw on duplicate key when the the static enum instance is created. (very early). Also this will avoid the reflection. I hope that makes sense, I was trying to find some of my code that takes that approach but can not locate it. If you are interested I can provide code to illustrate.
Update: terrible code but illustrates the approach:
public class SimpleEnum
{
static Dictionary<string, SimpleEnum> values = new Dictionary<string, SimpleEnum>();
SimpleEnum()
{
}
static SimpleEnum Create(string key, string value)
{
var simpleEnum = new SimpleEnum
{
Value = value
};
// throws when duplicate key:
values.Add(key, simpleEnum);
return simpleEnum;
}
public string Value { get; private set; }
public static readonly SimpleEnum One = Create(nameof(One), "One");
//fails immediately at run time:
public static readonly SimpleEnum Two = Create(nameof(One), "Two");
// the correct key:
// public static readonly SimpleEnum Two = Create(nameof(Two), "Two");
}
@arthg I like the idea of the dictionary. From a public API point of view: How can an user create the EnumValueObject
? Exposing another Create
one which is protected, one which is public?
The current implementation exposes the FromId
which returns Maybe<TEnumeration>
:
var input = Console.ReadLine();
var myEnum = MyEnum.FromId(input);
if (myEnum.HasNoValue) throw new Exception();
_repository.GetByMyEnum(myEnum.Value);
Your code gets a bit more complex when you want to make a derived version of SimpleEnum
.
I made a small PoC:
public abstract class EnumValueObject<TEnumeration> : ValueObject
where TEnumeration : EnumValueObject<TEnumeration>, new()
{
private static readonly Dictionary<string, TEnumeration> _enumerations = new Dictionary<string, TEnumeration>();
private EnumValueObject()
{
}
public virtual string Id { get; protected set; }
public static bool operator ==(EnumValueObject<TEnumeration> a, string b)
{
if (a is null && b is null)
{
return true;
}
if (a is null || b is null)
{
return false;
}
return a.Id.Equals(b);
}
public static bool operator !=(EnumValueObject<TEnumeration> a, string b)
{
return !(a == b);
}
public static bool operator ==(string a, EnumValueObject<TEnumeration> b)
{
return b == a;
}
public static bool operator !=(string a, EnumValueObject<TEnumeration> b)
{
return !(b == a);
}
public static Maybe<TEnumeration> FromId(string id)
{
return _enumerations.ContainsKey(id) ? _enumerations[id] : null;
}
public static bool Is(string possibleKey) => _enumerations.ContainsKey(possibleKey);
public override string ToString() => Id;
protected override IEnumerable<object> GetEqualityComponents()
{
yield return Id;
}
protected TEnumeration Create(string id)
{
if (_enumerations.ContainsKey(id))
{
throw new InvalidOperationException($"The id {id} already exists for {GetType().Name}");
}
var enumeration = new TEnumeration
{
Id = id
};
_enumerations.Add(id, enumeration);
return enumeration;
}
}
@vkhorikov what is your opinion?
I like the idea with the dictionary, but no need for the Create
method. We can check for duplicates in the GetEnumerations
methods and throw an exception then. I'm not sure how early it's going to be, but it should trigger right away since All
is a static field.
I'm travelling this weekend and may be slow to respond.
Maybe I got you wrong, but why is there no need for the Create
method? We have to populate the dictionary somewhere outside the c'tor to get rid of the Reflection completely and replace it with the dictionary.
Furthermore All
would be populated before any of the constructor calls are made... at least it is not guaranteed that All
is instantiated after the other static fields are initialized so the dictionary is properly populated.
I got it now ;) just recapped everything. I updated the PR.
Great! Pushing this with v2.15.0.
Not closing the issue as a reminder to see if the code can be refactored toward inheritance.
Apart from inheriting the ValueObject
class in this project, isn't this just a basic version of Steve Smith's SmartEnum?
Looks similar indeed.
Well it is just an extension for an enum. If you have a look on msdn you find a very similar approach.
All of those enum classes look alike. The difference here is mainly that it inherits from ValueObject
so you can use all the nice extensions. Plus in it's original form you only need to set a key as the additional Id seemed very counterintuitive for me in a DDD perspective on an enum.
Yep, I've been using something similar for years too.
While the usage of EnumValueObject is fairly simple, is there documentation, or a plan to have documentation on how to use EnumValueObject? Or should people just look at the tests for this?
Yes, I plan to do a full-blown documentation of all features, including this one. Issue for tracking: https://github.com/vkhorikov/CSharpFunctionalExtensions/issues/255
Am I mistaken, or is this simply the "typesafe enum" concept?
Yes basically
That's what I thought... I do my typesafe enums the way you did in the OP. But then I saw there's a lot more going on in here. :smile:
Hey together,
before creating a pull request I wanted to retrieve some feedback for the following new type:
EnumValueObject
The basic idea is that have a better enumeration type in the eco-system of the CSharpFunctionalExtensions.It has the following construct when used:
The main advantage over a "normal" enum would be:
Result<TEnumValueObject>
)To see the reference implementation which I would like to merge in this repository head over to my repository: EnumValueObject