Skip to content

Solution # 2 - My attempt to remove the two if #8

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emadb
Copy link

@emadb emadb commented Jul 23, 2014

I'm not sure that this solution works, but the test is green ;-)

@arialdomartini arialdomartini changed the title My attempt to remove the two if Solution # 2 - My attempt to remove the two if Jul 23, 2014
@arialdomartini
Copy link
Contributor

Amazing! Thank, Ema.
As a matter of fact, two of us think that initializing the collections in the constructor is a good idea. I still wonder why @ayende made a different choose.

Comparing @emadb solution with @matteobaglini's one I noticed that the extension method

 public static class Extensions
 {
     public static HashSet<T> ToHashSet<T>(this IEnumerable<T> source)
     {
          return new HashSet<T>(source);
     }
}

could be easily replaced with a simpler

new HashSet<string>(_ensuredFieldNames);

I think it's slightly less invasive, and I tend to prefer @emadb's approach, so far.

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

Successfully merging this pull request may close these issues.

2 participants