Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not a real issue #19

Closed
lvmajor opened this issue Jun 8, 2018 · 9 comments
Closed

Not a real issue #19

lvmajor opened this issue Jun 8, 2018 · 9 comments

Comments

@lvmajor
Copy link

lvmajor commented Jun 8, 2018

As the title says, it's not a real issue but more of a question.

I'd like to know why the decision was made to put the "name" property before the "value" in the constructor. Most of the time when working with key-value pairs or enums, it is logical to put the key first IMO and I'd like to know if there's any reason why it was made differently.

Thanks in advance!

@trevor-hackett
Copy link
Contributor

The name is the key. The value is, well, the value.

It does follow the convention of dealing with the key-value pairs

@lvmajor
Copy link
Author

lvmajor commented Jun 8, 2018

Isn't it inefficient to use a string as key?

And I personally find it counter-intuitive to think about the "name" as the variable supposed to hold my "key" and the "value" holding the display name or other info...

When rendering a select list for example, the current nomenclature forces me to create a SelectList using the "name" as value and "value" as the text field... that doesn't sound logical to me.

Also, on the way back, I should then use a string as the type to bind the result to? To me it doesn't seem logical at all, but that's my own opinion. Also related to this point, would it be possible to bind to the enum type when using asp.net core model binding? (okay maybe that's out of the scope of this question)

Thanks for your input though.. I'l try to wrap my head around the inconsistency (IMO) :)

@ardalis
Copy link
Owner

ardalis commented Jun 8, 2018

For a SelectList you want to display the Name but use the Value as the SelectList’s Value, right? I’m not seeing the inconsistency. Can you demonstrate with a bit of code perhaps?

@lvmajor
Copy link
Author

lvmajor commented Jun 8, 2018

You are totally right and I never said otherwise (sorry if I was unclear about that).

The only thing that bothered me in the beginning (as mentioned in the first post) was the initialization in which the "value", which is equivalent to the key, was put after the name. I would have liked to align the keys/values at the beginning as shown below to be able to see quickly the keys used, but that's a really minor thing ahah.

public class TestEnumClass : SmartEnum<TestEnumClass, int>
{
    public static readonly TestEnumClass FirstKey  = new TestEnumClass(0, "Display name for first key");
    public static readonly TestEnumClass SecondKey = new TestEnumClass(1, "Name for second item");
    public static readonly TestEnumClass ThirdKey  = new TestEnumClass(2, "etc...");
    private TestEnumClass(string name, int value) : base(name, value){}
}

I guess I read it at first as if the first param was indeed treated as the enum's equivalent key, but it's not, it's the enum's equivalent "textual" value, which is totally fine, and the value is treated the same as the underlying type, which is effectively an int in most cases. To be totally honest now that I look back at it, I wonder why I thought it was inconsistent. I guess I was half awake and the confusion came from the fact the here the "name" is used as a key, corresponding to the standard Enum's textual value, while in my head, the key was the "underlying value", which was effectively related to the standard Enum's value... don't know if it's any clearer ¯\(ツ)

Sorry for bothering :)

@lvmajor lvmajor closed this as completed Jun 8, 2018
@trevor-hackett
Copy link
Contributor

trevor-hackett commented Jun 8, 2018

Yes, the name is just a textual representation of the key. The key is actually the static field in the class (TestEnumClass.FirstKey). The name is supposed to represent that name or be meaningful in some way. Usually it is some variant of the variable name. This is why the README uses nameof() so that if the variable is renamed, the textual "key" remains the same as the variable name.

The name can be whatever you want it to be. If you're going to use it for a list and want to store the displayed message in the SmartEnum, I'd suggest extending SmartEnum and adding a description property so that you can use it in a list.

public TestEnumClass : SmartEnum<TestEnumClass, int>
{
    public static readonly TestEnumClass FirstKey  = new TestEnumClass(nameof(FirstKey), 0, "Display name for first key");
    public static readonly TestEnumClass SecondKey = new TestEnumClass(nameof(SecondKey), 1, "Name for second item");
    public static readonly TestEnumClass ThirdKey  = new TestEnumClass(nameof(ThirdKey), 2, "etc...");

    public string Description { get; }

    private TestEnumClass(string name, int value, string description) : base (name, value) {
        Descriptions = description;
    }
}

@lvmajor
Copy link
Author

lvmajor commented Jun 8, 2018

That's exactly what I did :)

@lvmajor
Copy link
Author

lvmajor commented Jun 8, 2018

Would it be useful to add a default implementation in the SmartEnum class that would get the SelectList from the value and name of the generated list directly?

@ardalis
Copy link
Owner

ardalis commented Jun 8, 2018

I wouldn't add an implementation in the SmartEnum class that was tied to SelectList or any other UI element. I could see adding either samples showing how to use SmartEnum with a SelectList or even including some extension methods that would create a SelectList from a SmartEnum or something like that. Would that help?

@lvmajor
Copy link
Author

lvmajor commented Jun 8, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants