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

Improved and consistent "Please implement..." messages #2211

Open
michalporeba opened this issue Jan 15, 2024 · 3 comments
Open

Improved and consistent "Please implement..." messages #2211

michalporeba opened this issue Jan 15, 2024 · 3 comments

Comments

@michalporeba
Copy link
Contributor

Hi @ErikSchierboom,

In the Binary Search Tree PR, we discussed the consistency of messages in the `NotImplementedException' and the benefits the changed wording has, especially for the web editor. See comment 1, comment 2 for reference.

I'd be happy to standardise the messages between concept and practice exercises, but I wanted to discuss options more.

  • Function - while technically could be correct, it is not helpful and so not right in the context.
  • The messages are useful to guide people doing the exercises through the web interface.
  • The examples of current messages (with a little bit of context) are below.

The current concept message examples are:

public static Permission Default(AccountType accountType)
{
    throw new NotImplementedException("Please implement the (static) Permissions.Default() method");
}

public static int[] LastWeek()
{
    throw new NotImplementedException("Please implement the (static) BirdCount.LastWeek() method");
}

public int CountForFirstDays(int numberOfDays)
{
    throw new NotImplementedException("Please implement the BirdCount.CountForFirstDays() method");
}

protected Character(string characterType)
{
    throw new NotImplementedException("Please implement the Character() constructor");
}

public static string Describe(Destination destination)
{
    throw new NotImplementedException("Please implement the (static) GameMaster.Describe(Destination) method");
}

There are some potential inconsistencies or issues above.

  • Methods signatures usually don't include parameters, but sometimes they do. It does make sense, as in the last example the method Descibe is overloaded.
  • When the method is not static the notation . suggests it anyway.
  • Static in the parenthesis looks a bit confusing. The keyword is static not (static) and so is the word. However, since we can only have the message in a situation where the method is defined and implemented with the NotImplementedException, that might be redundant information. The signature will be there.
  • I'm really missing full stops at the end of a sentence in the message.

Could we do the following:

  • for static methods
"Please implement the Permissions.Default(AccountType) method."
  • for class instance methods
"Please implement the CountForFirstDays(int) method in the BirdCount class."
  • for constructors
"Please implement the Character class constructor."
  • for read/write properties
"Please implement the Name property in the Actor class."
  • for a property with a getter only
"Please implement the getter of the Age property in the Actor class."

What do you think about the above proposal? Whichever way we agree to go, I'm happy to implement it.

I'm starting the discussion here on an issue, as it relates to PRs and comments already here, and the scope is relevant only to the track; however, if you think it would be better, I'll post it on the forum, too.

Copy link
Contributor

Hello. Thanks for opening an issue on Exercism 🙂

At Exercism we use our Community Forum, not GitHub issues, as the primary place for discussion. That allows maintainers and contributors from across Exercism's ecosystem to discuss your problems/ideas/suggestions without them having to subscribe to hundreds of repositories.

This issue will be automatically closed. Please use this link to copy your GitHub Issue into a new topic on the forum, where we look forward to chatting with you!

If you're interested in learning more about this auto-responder, please read this blog post.

@ErikSchierboom
Copy link
Member

I think we could do without the static/non-static distinction entirely. I really like the suggestion about including the type of the parameter.

In my mind, there are two options now.

  1. Combine Class + Method:
  • "Please implement the Permissions.Default(AccountType) method."
  • "Please implement the BirdCount. CountForFirstDays(int) method."
  • "Please implement the Character.Character() constructor method."
  • "Please implement the Actor.Name property."
  • "Please implement the getter of the Actor.Age property."
  1. Separate Class + Method:
  • "Please implement the Default(AccountType) method of the Permissions class."
  • "Please implement the CountForFirstDays(int) method of the BirdCount class"
  • "Please implement the Character() constructor method of the Character class."
  • "Please implement the Name property of the Actor class."
  • "Please implement the getter of the Age property of the Actor class."

Whilst I like the brevity of the first option, I think for people unknown to object oriented-programming, the second option is probably clearer.

@michalporeba
Copy link
Contributor Author

I agree. The second option is clearer. Let's go with it.

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