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

Implement new concept exercise for extension methods #1585

Merged
merged 36 commits into from
Sep 29, 2021

Conversation

yzAlvin
Copy link
Contributor

@yzAlvin yzAlvin commented Sep 5, 2021

I have followed the instructions here and read the docs listed in the issue #1445

This adds a new concept exercise focused around teaching extension-methods

The problem is to extend the IEnumerable interface with a Sum, Product, Mean, Median and Mode method, as they could be commonly used in a statistical context

Please let me know if there is anything I should revise

@yzAlvin
Copy link
Contributor Author

yzAlvin commented Sep 5, 2021

/dotnet-format

@yzAlvin
Copy link
Contributor Author

yzAlvin commented Sep 7, 2021

Actually I don't think this is a very good exercise for learning extension methods

This only goes into how to write one but doesn't actually use them except for the tests

But I am curious if this could be revised to be practice for some other thing?

@ErikSchierboom
Copy link
Member

I don't have much time at the moment, but I'll try and look at it this week!

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thank you so much for working on this! It's greatly appreciated.

Actually I don't think this is a very good exercise for learning extension methods

This only goes into how to write one but doesn't actually use them except for the tests

This is totally valid point. We should try to design the exercise such that we encourage/force people to use extension methods too (writing them is the more important part here I feel). One way we do this in Concept Exercises is to have later tasks depend on functionality that is implemented in a previous task.

The main thing about the current exercise that I'm unsure of is the task: the statistical analysis part of it. In general, we try to shy away from maths as much as possible (which is not always possible), especially for Concept Exercises as it is often hard to come up with a nice story/theme around math problems.

Maybe we could brainstorm a bit on what could make a good story for this exercise?

Personally I think that maybe extending the string class could be useful. We could even save us a lot of trouble by re-using the existing log-levels exercise story/theme, which is something that we do a lot actually. This exercise already deals with strings and has a student extract parts of information from the string, which could work well for extension methods.

What do you think?

concepts/extension-methods/about.md Outdated Show resolved Hide resolved
concepts/extension-methods/about.md Outdated Show resolved Hide resolved
concepts/extension-methods/about.md Outdated Show resolved Hide resolved
concepts/extension-methods/introduction.md Outdated Show resolved Hide resolved
concepts/extension-methods/introduction.md Outdated Show resolved Hide resolved
@yzAlvin
Copy link
Contributor Author

yzAlvin commented Sep 9, 2021

Yeah I agree :D, I'll try and design an exercise around extending log levels for extension methods.

@ErikSchierboom
Copy link
Member

Awesome! Thanks.

@yzAlvin
Copy link
Contributor Author

yzAlvin commented Sep 14, 2021

😬 😬
That was harder than I thought it would be, I am not creative at all..

I couldn't think of anything else other than re-using the idea of a log level aha, and tbh I'm still not sure this makes for a good concept exercise, but happy to hear what others think

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was harder than I thought it would be, I am not creative at all..

I couldn't think of anything else other than re-using the idea of a log level aha, and tbh I'm still not sure this makes for a good concept exercise, but happy to hear what others think

Yeah, I struggled with this too at first. But once you've done a couple, things do become easier.

I've been giving this some more thought, and I think I've found a nice way to have students define and use the extension method. What if we would replace the WordCount extension method with a SubstringAfter method, which can be defined like:

public static string SubstringAfter(this string str, string delimiter)
{
    return str.Substring(str.IndexOf(delimiter) + 1);
}

and also have the students define a SubstringBetween method:

public static string SubstringBetween(this string str, string start, string stop)
{
    return str.Substring(str.IndexOf(start) + 1, str.IndexOf(stop) -1);
}

If we make this the very first two tasks of the exercise (like WordCount is the first task now), we could then reasonably expect the student to use these methods for the Message and LogLevel methods. We can then drop the Truncate method I think, which would have become unnecessary. What do you think?

p.s. it is totally normal for a Concept Exercise to go through several iterations before arriving at a satisfactory implementation

exercises/concept/log-analysis/.docs/introduction.md Outdated Show resolved Hide resolved
exercises/concept/log-analysis/LogAnalysis.csproj Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.meta/Exemplar.cs Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.docs/instructions.md Outdated Show resolved Hide resolved
@ErikSchierboom
Copy link
Member

@yzAlvin What do you think about #1585 (review)?

@yzAlvin
Copy link
Contributor Author

yzAlvin commented Sep 19, 2021

I like it :) I wanted an extension method that had some arguments and that achieves it

@ErikSchierboom
Copy link
Member

ErikSchierboom commented Sep 21, 2021

@yzAlvin Great! Ping me when you've updated the PR and I'll review. It will be great to have an extension methods exercise (I'm a big fan).

@yzAlvin
Copy link
Contributor Author

yzAlvin commented Sep 22, 2021

@ErikSchierboom Updated!

Not finished yet, wanted to get some feedback on tests (above comment)
I am planning on adding more test cases and naming the tests with better names

@yzAlvin yzAlvin marked this pull request as draft September 23, 2021 03:20
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! I added some minor comments, nothing big. Just some tweaks and then we're ready to merge!

exercises/concept/log-analysis/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/log-analysis/LogAnalysis.cs Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/log-analysis/.docs/instructions.md Outdated Show resolved Hide resolved
config.json Show resolved Hide resolved
config.json Outdated Show resolved Hide resolved
@yzAlvin
Copy link
Contributor Author

yzAlvin commented Sep 23, 2021

Will add the things about this and namespaces soon, is it alright that the test case for SubstringAfter is generic and unrelated to the story?

@ErikSchierboom
Copy link
Member

is it alright that the test case for SubstringAfter is generic and unrelated to the story?

Yeah sure.

@yzAlvin
Copy link
Contributor Author

yzAlvin commented Sep 25, 2021

@ErikSchierboom alright, thanks for all your input, feel free to point out anything else :D

@yzAlvin yzAlvin marked this pull request as ready for review September 26, 2021 06:11
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! I've left two small comments, but I'd be totally fine with merging this as is and addressing them in a follow-up PR. Whatever you prefer.

Extension methods are static methods, but they're called as if they were instance methods on the extended type. It achieves this by using `this` before the type, indicating the instance we put the `.` on is passed as the first parameter. For client code, there's no apparent difference between calling an extension method and the methods defined in a type.

```csharp
public static int WordCount(this string str)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to be a bit more explicit here and give the class a namespace, and then have a second code sample indicating another file that has a using statement for importing the extension method. That might make it a bit more explicit.

@ErikSchierboom ErikSchierboom merged commit f02dcd1 into exercism:main Sep 29, 2021
@ErikSchierboom
Copy link
Member

Merged, thanks! This should appear on the website shortly. Great work @yzAlvin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants