Make code more modular #5
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Separate the logic for initialising, moving, wrapping and flipping the state of a pipe.
I know I'll forget the "+1" in `(rand() / (RAND_MAX / width + 1)` at some point, so refactor this into a function called `random_i` (`random` will conflict with a built-in function with `--std=gnu99`).
Rendering the pipes to the screen and animating a frame are two separate concepts. Rendering takes care of drawing the pipes and flushing everything to the terminal. Animating involves rendering a frame, determining the delay required before the next frame needs rendering and sleeping for that length of time. This commit separtes the two concerns out. The `animate` function now calls an arbitrary renderer, which is passed as a function pointer. The `animate` function could call the rendering function directly, but in the future we may want to pass a list of renders to allow for overlays.
This moves the "should flip" condition into pipe.c. It also changes "flip_pipe_state" so that it returns the previous state.
Actually referencing an element in these arrays was a painful exercise. We can get around this by just using C-style arrays of C-strings.
Just so that if we want to render a pipe in a separate rendering function (not that any exist yet), it's a bit easier.
livibetter
reviewed
Feb 17, 2018
util.c
Outdated
|
|
||
| ///Get a random integer in the range [lo, hi). | ||
| int random_i(int lo, int hi){ | ||
| return (rand() / (RAND_MAX / (hi - lo) + 1)) + lo; |
There was a problem hiding this comment.
This doesn't look right, I think you miss a ():
return (rand() / (RAND_MAX / (hi - lo) + 1) ) + lo;
return (rand() / (RAND_MAX / ((hi - lo) + 1))) + lo;There was a problem hiding this comment.
It was roughly correct as written, except for the obvious division by zero when lo == hi. E.g. if random_i(0, N) returns the maximum of RAND_MAX / ((RAND_MAX / N) + 1), which is < N.
I've altered this in 7d2e17e to give less surprising behaviour. It's still slightly biased when hi - lo is close to RAND_MAX (see http://c-faq.com/lib/randrange.html), but it's not important for our use.
util.c
Outdated
| #include <stdlib.h> | ||
| #include "util.h" | ||
|
|
||
| ///Get a random integer in the range [lo, hi). |
There was a problem hiding this comment.
Shouldn't be range be [lo, hi], hi included as that's that +1 for?
random_i(0, 3); // returns one of 0, 1, 2, 3This commit replaces the function `random_i` with `randrange`. This matches the Python nomenclature of using `randrange` for `[lo, hi)` and `randint` for `[lo, hi]`, and removes a glaringly obvious possibility for a divide-by-zero error when `random_i` was called with `lo == hi`. For the sake of consistency, I also added `randbool` to avoid scattered calls to `rand()` and usage of `RAND_MAX`. Note: The current implementation of `randint` is slightly biased when `hi - lo` is close to `RAND_MAX`, but that really doesn't matter for our application.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request amalgamates some changes I made years ago that separates much of the logic into functions. I've learned how to use git more effectively since then, so the commits have been heavily modified, which is why the
CommitDateis three years after theAuthorDate.The original plan was to add separate rendering layers on top of one another: the current randomly-moving pipes, path-directed pipes, a clock overlay, etc. I've discarded my previous attempts to do that as being ill-conceived, but separating the code into modules will make anything more than the current very simple program much easier.
I will merge this later this weekend unless anyone has any objections.