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

Adding BatchType and ChartType helper classes to convert string inputs to enums #30

Closed
wants to merge 6 commits into from

Conversation

longpshorn
Copy link
Collaborator

Not sure whether these below in the IEXSharp library or not, but as I am developing my client that is making use of the library, I wrote these helper methods and thought they might be beneficial to have in the library.

I can pull them out if you don't think it would be appropriate to add them, but I thought they could potentially be a benefit to others as well. What do you think?

@longpshorn
Copy link
Collaborator Author

If you think this makes sense, I will add similar methods for other enums as well.

@vslee
Copy link
Owner

vslee commented May 6, 2020

I did not think about the case where people might want to convert back from a description to an enum. It seems like a lot of work to go through all of the enums and write mirror methods for each one. It might also introduce a little bit of brittleness where if the description changes, two places in the code now have to be updated (although the that's probably just a minor nitpick). What do you think if, instead, we introduced a method such as this:

		public static TEnum GetEnumFromDescription<TEnum>(this string stringValue)
		{
			var enumType = typeof(TEnum);
			var members = enumType.GetMembers();
			foreach (var member in members)
			{
				if (member.GetCustomAttributes(typeof(DescriptionAttribute), false)
				.FirstOrDefault() is DescriptionAttribute descriptionAttribute
				&& descriptionAttribute.Description == stringValue)
				{
					return (TEnum)Enum.Parse(enumType, member.Name);
				}
			}
			throw new Exception("Description string not found.");
		}

Note, that is just the top of my head, I have not tested that code yet. It may need some modifications to work properly.

@longpshorn
Copy link
Collaborator Author

Yes, this makes more sense. I agree that it my approach was a bit brittle. It was admittedly just a bit of a quick hack to get it working for my use case so I appreciate your thought on this. I will take a look at adding it and making sure it works as expected.

@longpshorn longpshorn closed this May 6, 2020
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

Successfully merging this pull request may close these issues.

2 participants