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

[c++/atbash-cypher] Create mentoring.md #2331

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

denisf2
Copy link

@denisf2 denisf2 commented Mar 17, 2024

Short proposal to use one function for symmetrical cipher.

@github-actions github-actions bot added the track/cpp C++ track label Mar 17, 2024
@@ -0,0 +1,4 @@
The atbash cipher is symmetrical that is why it is possible to use one function to encode and decode.
Copy link
Member

Choose a reason for hiding this comment

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

  • This doesn't follow the structure of other mentoring notes.
  • These notes are very sparse. If you'd like to tackle creating new notes, it would be nice to have them a bit more fleshed out.

@siebenschlaefer
Copy link

Thank you!

There is one subtle issue and two stylistic things. I also have some questions:


  1. The functions from <cctype> like std::isalpha(), std::isdigit(), std::tolower(), etc. take an int and their behavior is only well defined if the parameter can be represented by an unsigned char or the special value EOF. I strongly recommend never passing a char that might be negative to these functions. You could use a static_cast to convert the argument to unsigned char, or you could replace the classic for loop with a range-based for loop and make the loop variable an unsigned char.
  2. Exercism style guides want One sentence per line.
  3. The second "Talking points" appears to be empty now. Do you need it at all?

Passing function non-POD arguments by value (here usually std::string).

I'm not sure what this means. Are you arguing that functions should take "non-POD"-type arguments always by value or always by reference to const? What's different for "POD"-type arguments? And doesn't this example solution have functions that take a std::string by value (apply_cypher) and by reference to const (the other three functions)?

Students usually use mapping algorithm (map/array chars)

Sorry, I don't understand. Do you mind rephrasing that (at least here in a comment, for me)?

Ask a student to refactor. Could you extract chunk splitting code and make refactoring of you code taking in account that fact?

That sounds like you want students to solve this exercise in a way similar to yours.
I think your approach with the two helper functions apply_cypher and apply_spaces is perfectly valid.
However, I do not think it's the only valid approach. For example IMHO an equally valid approach would be to write a single helper function that encodes a single char and call this function from encode and decode. That avoids looping over the string twice during encode (once to encode the letters, copy the digits, omit anything else, then again to add the separating spaces) and creating additional instances of std::string.

@denisf2
Copy link
Author

denisf2 commented Mar 18, 2024

1. The functions from `<cctype>` like `std::isalpha()`, `std::isdigit()`, `std::tolower()`, etc. take an `int` and their behavior is only well defined if the parameter can be represented by an `unsigned char` or the special value `EOF`. I strongly recommend never passing a `char` that might be negative to these functions. 

Thank you. I've never seen this remark before. Live and learn.

I'm not sure what this means. Are you arguing that functions should take "non-POD"-type arguments always by value or always by reference to const? What's different for "POD"-type arguments? And doesn't this example solution have functions that take a std::string by value (apply_cypher) and by reference to const (the other three functions)?

As I can understand task template provoke to copy/paste function interface and students write helper functions passing string by value. My tactic depends from student's code idea, usually explain penalties and give other possibilities to make a choice. But they leave as it is.

Sorry, I don't understand. Do you mind rephrasing that (at least here in a comment, for me)?

Something like this:

std::map<char,char> table ={{a,z},{b,y},...};
// or 
const std::string alphabet {"abcd..yz"}; //-> to to find character index in a loop
const std::string rev_alpha {"zy...cba"};

That sounds like you want students to solve this exercise in a way similar to yours. I think your approach with the two helper functions apply_cypher and apply_spaces is perfectly valid. However, I do not think it's the only valid approach. For example IMHO an equally valid approach would be to write a single helper function that encodes a single char and call this function from encode and decode. That avoids looping over the string twice during encode (once to encode the letters, copy the digits, omit anything else, then again to add the separating spaces) and creating additional instances of std::string.

I thought that these tips just variants to start dialog. Prefer a single purpose function with corresponding name if no big time/memory penalty in general.

@denisf2 denisf2 changed the title Create mentoring.md Create mentoring.md for atbash cypher /c++ Mar 19, 2024
@siebenschlaefer
Copy link

As I can understand task template provoke to copy/paste function interface and students write helper functions passing string by value. My tactic depends from student's code idea, usually explain penalties and give other possibilities to make a choice. But they leave as it is.

I agree, discussing pass-by-value vs. pass-by-reference is always a good idea.
I just don't fully understand the point in the "Common suggestions": "Passing function non-POD arguments by value (here usually std::string)." and "Ask use references or lightweight objects (here std::string_view)" and how that applies to the example code. If you think this is something that mentors should discuss then it might help describing it in more than one sentence.

I thought that these tips just variants to start dialog. Prefer a single purpose function with corresponding name if no big time/memory penalty in general.

IMHO there's a small difference between "Common Suggestions" and "Talking Points".


I'm probably not the only mentor who is not a native English speaker. I'd really like these suggestions and talking points to be easy to read, even by people like me. Do you mind fleshing them out or rephrasing them to make them unambiguous and easy to understand?

But this all might just be a misunderstanding on my side. Maybe somebody else reading this can weigh in.

@IsaacG
Copy link
Member

IsaacG commented Mar 20, 2024

Do you mind fleshing them out or rephrasing them to make them unambiguous and easy to understand?
But this all might just be a misunderstanding on my side. Maybe somebody else reading this can weigh in.

Native English speaker. I agree these bullet points are very terse and hard to understand.

@github-actions github-actions bot added the type/mentor-notes Mentor notes label Mar 20, 2024
@denisf2
Copy link
Author

denisf2 commented Mar 20, 2024

IMHO there's a small difference between "Common Suggestions" and "Talking Points".

I have read some mentoring.md from the repository. I still have no clue the difference. Could you provide me some clarification? Is 'common' for what level? For similar tasks? Cases of programming language using? Something else?
Thanks

@denisf2 denisf2 changed the title Create mentoring.md for atbash cypher /c++ [c++/atbash-cypher] Create mentoring.md Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
track/cpp C++ track type/mentor-notes Mentor notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants