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

protein-translation: slightly confusing setup #1937

Closed
ErikSchierboom opened this issue Jun 30, 2024 · 7 comments · Fixed by #1940
Closed

protein-translation: slightly confusing setup #1937

ErikSchierboom opened this issue Jun 30, 2024 · 7 comments · Fixed by #1940

Comments

@ErikSchierboom
Copy link
Member

I'm not really sure why the protein-translation exercise has an additional parse function that returns a pub struct CodonsInfo<'a>. It feels a bit weird to me. Maybe a better approach would be to model the codons as an enum?

@senekor
Copy link
Contributor

senekor commented Jun 30, 2024

Looks like the exercise was added in this PR: #603.

There's no discussion on this design descision. It seems to me that it gives students the freedom to prepare the input data for efficient lookup (e.g. hash map), which can then be performed many times. (even though the test suite doesn't do that)

I'm sure the exercise could've been designed a different way, but is it worth it to break existing solutions?

@ErikSchierboom
Copy link
Member Author

I'm sure the exercise could've been designed a different way, but is it worth it to break existing solutions?

I don't think so. I just wanted to discuss some of the design decisions to maybe improve future exercises. I'll post a more detailed reply next week when I'm back at work!

@ErikSchierboom
Copy link
Member Author

Okay, so what I found confusing/did not like was:

  • The function named parse is not expected to do any parsing at all. It just "wraps" existing information in a struct, which is not really parsing.
  • The exercise forces the student to a very specific solution, namely one where codons are looked-up from a vector or map. I'd much rather allow the students the freedom to solve the exercise how they like, e.g. by pattern matching. The struct to me is an implementation detail forced upon the user.
  • Related to the above, the struct/impl setup feels more complex than it could be, where parse (or a better named function like proteins or something) should just immediately return the translated proteins.
  • I feel that the setup of the exercise makes things unnecessarily harder for the student. I'm not saying that structs, lifetimes and impl blocks are not key Rust concepts, but I am arguing that this exercise does not need them.

Summarizing, I think this exercise has its fair share of problems. I'm not suggesting we redesign the exercise, as that would indeed break existing solutions, but I am writing this as a sort of cautionary warning for when new exercises are being added.

@senekor
Copy link
Contributor

senekor commented Jul 9, 2024

I agree. There are some situations where it might make sense to force a specific solution, if the intent of the exercise is to teach a specific concept. (I like the design of space-age for example.) On the other hand, that's what concept exercises of a syllabus are for... Either way, this exercise doesn't really teach anything, it just forces a specific, weird solution.

What do you think about deprecating it?

@ErikSchierboom
Copy link
Member Author

On the other hand, that's what concept exercises of a syllabus are for...

Indeed!

What do you think about deprecating it?

Let me ponder this a bit!

@ErikSchierboom
Copy link
Member Author

What do you think about deprecating it?

We've talked this over internally, and we feel that it might be best to restructure the exercise. Yes it'll break existing solutions, but it'll make things a lot more sane for future students.

@senekor
Copy link
Contributor

senekor commented Jul 9, 2024

Alright, I should be able to find some time in the evening.

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

Successfully merging a pull request may close this issue.

2 participants