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

[authentication-system] Missing test + Identity probably should be class #1904

Open
wischi-chr opened this issue Mar 12, 2022 · 1 comment
Open

Comments

@wischi-chr
Copy link

wischi-chr commented Mar 12, 2022

I just mentored this exercise and found out that this exercise should probably be improved.

In Task 3 the user is asked to "Remove the set accessor or make it private" and because Identity is a struct and the properties are strings it can no longer be changed (because structs are implicitly copied) but Task 4 now asks the user to prevent tempering with the Admin property (which is not possible because we prevented that in Task 3, because string is immuatble and Identity is a struct) - maybe make Identity a class and link some resources in the instructions about the difference of structs and classes in C#.

In such simple exercises I would even go as far as not to use structs at all(!) unless the exercise is about structs. Don't get me wrong structs are important in C# but such a niche that I would consider them an "advanced concept" because there are so many things that might be confusing for example in structs with mutable reference fields, structs that are larger than 16 bytes, etc.

Task 5 asks the user to make a defensive copy of the developer dictionary before returning it but no test tests for that.
I sent the student the following test to make sure they implement a defensive copy:

[Fact]
public void DevelopersCantBeTamperedWith()
{
    var authenticator = new Authenticator(new Identity { EyeColor = "green", Email = "admin@ex.ism" });

    var devs = authenticator.GetDevelopers() as IDictionary<string, Identity>;

    try
    {
        devs.Add("Evil Attacker", new Identity { Email = "evil.attacker@ex.ism", EyeColor = "transparent" });
    }
    catch (NotSupportedException)
    {
        // Ignore exception from read-only dictionaries.
    }

    Assert.False(authenticator.GetDevelopers().ContainsKey("Evil Attacker"));
}

Something like that should probably be added to the exercise.

@ErikSchierboom
Copy link
Member

We might want to redesign the exercise a bit in the future. As for the missing test for task 5, we want the student to use a readonly dictionary. We could thus have the test be:

[Fact]
public void DevelopersCantBeTamperedWith()
{
    var authenticator = new Authenticator(new Identity { EyeColor = "green", Email = "admin@ex.ism" });
    Assert.IsType<ReadOnlyDictionary<string, Identity>>(authenticator.GetDevelopers());
}

What do you think? The main problem with this, is that it sort of gives away the solution, but then again the introduction also explicitly mentions this.

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

2 participants