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

Update circular_buffer.h #903

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update circular_buffer.h #903

wants to merge 1 commit into from

Conversation

lucaato
Copy link

@lucaato lucaato commented Jun 20, 2023

I've added function definition in header files to let the user know what are the function that need to be created.

Added function definition in header files.
@github-actions
Copy link
Contributor

Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed.

That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Jun 20, 2023
@siebenschlaefer
Copy link
Contributor

I'm not really opposed to this PR, but:

As far as I know it was a deliberate decision not to provide any declarations in most exercises. Students have to read the tests anyway to understand the requirements, extracting the definitions/declarations of the public types/functions should not be that hard, and there are a few choices that students can make (public definitions of struct in the .h file vs. opaque types, sometimes void return types vs. some sort of error code, etc.), and (in general) writing .h files is part of the development process in C.

Also, Exercism prefers to discuss these kinds of changes in the forum first to get feedback and reach some consensus. (Tracks can opt out and keep the discussion here on Github, but AFAIK that's not the case for this C track, right @wolf99 or @ryanplusplus?)

@lucaato
Copy link
Author

lucaato commented Jun 21, 2023

Hi,

it is true that writing .h files is part of the development process, but if I already have an idea of what a circular buffer is I just need to know what are the function name to get going.
It's also true that users should look at the tests but from the website small side panel is not always easy to do, so i think the header file should at least give an idea of what are the function names, and maybe a little explanation of what they should do, everything can be written down in a minute comment.
Later when test fail users will have the chance to look at testcases in more details.

It's just an idea, if I have time I'll write about this in the forum.
Thanks for the support.

@ryanplusplus
Copy link
Member

Also, Exercism prefers to discuss these kinds of changes in the forum first to get feedback and reach some consensus. (Tracks can opt out and keep the discussion here on Github, but AFAIK that's not the case for this C track, right @wolf99 or @ryanplusplus?)

I personally don't make it into the forum much and like to have these conversations in a GitHub issue so we can discuss the path we want to take prior to a PR.

@ryanplusplus
Copy link
Member

Hi,

it is true that writing .h files is part of the development process, but if I already have an idea of what a circular buffer is I just need to know what are the function name to get going. It's also true that users should look at the tests but from the website small side panel is not always easy to do, so i think the header file should at least give an idea of what are the function names, and maybe a little explanation of what they should do, everything can be written down in a minute comment. Later when test fail users will have the chance to look at testcases in more details.

It's just an idea, if I have time I'll write about this in the forum. Thanks for the support.

Thanks for this information, it helped me to understand why it's not already easy to figure out what the function names are by looking at the tests. I think this is good feedback to bring to the Exercism website team. Reading the tests and understanding the requirements that they encode is an important part of the development process in Exercism. If the online editor is not making that easy, then we should make a fix in the online editor instead of trying to design the tracks so that you don't need to see the tests.

@siebenschlaefer
Copy link
Contributor

I personally don't make it into the forum much and like to have these conversations in a GitHub issue so we can discuss the path we want to take prior to a PR.

If the other three maintainers (@ryanplusplus, @bobahop, @patricksjackson) agree you can let @ErikSchierboom know so that the automatic closing of PRs that directs to the forum gets disabled.

@ryanplusplus
Copy link
Member

If the other three maintainers (@ryanplusplus, @bobahop, @patricksjackson) agree you can let @ErikSchierboom know so that the automatic closing of PRs that directs to the forum gets disabled.

I'm on board. I've just been reopening issues and PRs that the bot closed.

@wolf99
Copy link
Contributor

wolf99 commented Jun 23, 2023

I personally don't make it into the forum much and like to have these conversations in a GitHub issue so we can discuss the path we want to take prior to a PR.

If the other three maintainers (@ryanplusplus, @bobahop, @patricksjackson) agree you can let @ErikSchierboom know so that the automatic closing of PRs that directs to the forum gets disabled.

I'm ok with this also.

@bobahop
Copy link
Member

bobahop commented Jun 23, 2023

If the other three maintainers (@ryanplusplus, @bobahop, @patricksjackson) agree you can let @ErikSchierboom know so that the automatic closing of PRs that directs to the forum gets disabled.

Okay by me as well.

@ErikSchierboom
Copy link
Member

Seems to have been done in f9dfff1?

@siebenschlaefer
Copy link
Contributor

@ErikSchierboom Yes. See issue #904

@wolf99
Copy link
Contributor

wolf99 commented Jul 22, 2023

I had a quick try of the circular-buffer exercise on the website.
For me the website UI looks OK, but the barrier to entry for this exercise is high because so much implementation needs to be in place before the code will even compile without errors.

Suggestions for how to resolve this:

  1. Break up the tests to simpler steps. This could be tests additional, but earlier, to those from teh exercise specification, or we could work to update the specification.
  2. Raise the difficulty level from easy to something above medium.
  3. Provide header stub for this exercise.

Personally I think a mix of 1 & 2 would be the correct approach, but I'm open to 3.

My reasoning:

  • 1 is better than 3 because it fixes the issue rather than "hiding" the issue.
  • 2 is better than 3 because it gives correct information rather than "hiding" the issue (also, IIRC we dont have many, iexercises marked above medium difficulty).
image

@wolf99 wolf99 reopened this Jul 22, 2023
@siebenschlaefer
Copy link
Contributor

I'm not sure I understand what you mean with "1. Break up the tests to simpler steps."
Do you want to add additional tests at the beginning of test_circular_buffer.c that make the required interface easier to extract?

And I agree with (2), this is not an easy exercise,
But I'm a mentor, not a maintainer, so take this just as input, not some sort of vote.

@bobahop
Copy link
Member

bobahop commented Jul 24, 2023

It's certainly worth considering these things.

A common suggestion I make when mentoring circular buffer is to define the buffer as an array as the last field of the struct, instead of as a pointer, so that it doesn't need to be separately allocated and freed. By providing the header stub, the student may not know why it is like that, unless they learn by the example, or we provide some explanation in an About, or they ask about it. So I'm a bit wary of providing the header stub.

I definitely don't think Circular Buffer is "Easy", so I think at least "Medium" is better.

As for fiddling with the tests, as a student who's done 870 exercises, I do not do a happy dance when an exercise becomes outdated. If it is not substantive, it's annoying, And if it is a substantial change that breaks my solution, I curse that it just wasn't made a different exercise. So I would hope that any test changes bring some real benefit. It might be helpful to have stats on how many try circular buffer and abandon it. I think they are available on the site somewhere, and I always forget where they are. I'm not sure that CB has been a problem to more than a few students over the years.

Edit: Build Status

About a 48% completion rate, which is even higher than Armstrong Numbers. If we're not looking to change Armstrong Numbers, then perhaps CB is okay?

@siebenschlaefer
Copy link
Contributor

@bobahop
armstrong-number gets displayed as the first exercise after hello-world. I guess it has a low completion rate because this is the first time students have to solve a problem in C on their own and some give up. (but that's just a guess)

@bobahop
Copy link
Member

bobahop commented Jul 25, 2023

Perhaps that's all the more reason to look at Armstrong Numbers first? Maybe move it further back? It has over 80% completion rate on C++ track.

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

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

Review for circular_buffer.h
Strengths

  1. Clear Functionality: The header file provides a concise interface for a circular buffer, a common data structure, with all the essential operations such as reading, writing, overwriting, clearing, and deleting.
  2. Type Definitions: The use of typedef for buffer_value_t allows easy modification of the data type for buffer elements, making the code more flexible.
  3. Structured Approach: Encapsulation of the buffer functionality within the circular_buffer_t structure ensures clean abstraction and modularity.
  4. Error Handling Ready: The use of int16_t for return types in the functions suggests preparation for implementing error codes, which is good for robustness.
  5. Documentation: Each function is accompanied by a brief description, improving readability and providing insights into the intended usage.
    Suggestions for Improvement
  6. Error Code Descriptions:
    o Define specific error codes or macros (e.g., BUFFER_SUCCESS, BUFFER_FULL, BUFFER_EMPTY, etc.) to make error handling more intuitive for users.
    o Document these error codes in the header file.
  7. Memory Safety:
    o Consider adding explicit details about how memory is allocated and freed for circular_buffer_t to guide users on proper usage.
  8. Thread Safety:
    o Clarify whether the buffer operations are thread-safe or need external synchronization in multithreaded environments.
  9. Consistency in Comments:
    o The comment for overwrite() duplicates "write a value to the buffer." It can be revised to focus on its unique functionality of replacing the oldest value when the buffer is full.
    Proposed Change:
    // overwrites the oldest value if the buffer is full
  10. Additional Features:
    o Provide a peek() function to allow inspection of the next element without removal.
    o Add a size() or is_empty() function to check the buffer's current status, which can be useful in various scenarios.
  11. Example Usage:
    o Add a short example of how to use the circular buffer (e.g., in a README or as inline comments) to help new users quickly understand its API.
    Code Style Enhancements
    • Include Guards:
    o Use a more unique macro name for the include guard, such as #ifndef CIRCULAR_BUFFER_H_ or incorporate a project-specific prefix to avoid potential naming conflicts in larger projects.
    Testing
    Ensure comprehensive unit tests for the following scenarios:
    • Reading from an empty buffer.
    • Writing to a full buffer (both with and without overwriting).
    • Clearing the buffer and verifying its state.
    • Handling edge cases like buffer overflows or invalid pointers.
    Overall Assessment
    This header file provides a solid foundation for a circular buffer implementation with essential functionality and a clean interface. By addressing the above suggestions, it can be made even more robust, user-friendly, and ready for production use.

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 this pull request may close these issues.

7 participants