Mirror 534#13
Conversation
Fix expected output: change 'with' to 'wIth' in sed exercise instructions
* Expected output now in expected folder * Github action which runs after validation, when Needs Review * Script that can be run to test locally before push * Split number sys qs into automatable/manualy checkable --------- Co-authored-by: l <l>
* Add a shebang because `-v` is a bash-ism not present in whatever my default shell is on macOS. * Remove `--quiet` from `cmp` - macOS's `cmp` doesn't support it.
* Trigger on added files - Cowsay involves writing brand new files * Hide previous comments * Fix completed -> complete * Clean up output a little
This comment has been minimized.
This comment has been minimized.
Droid-An
added
NotCoursework
Needs Review
|
Thanks for opening a new PR! AI started to review it. Please notice that AI will review this PR only once |
![cyf-ai-code-reviewer[bot]](https://avatars.githubusercontent.com/u/22743767?s=60&v=4){kind=small}
| def double(value): | ||
| return value * 2 |
There was a problem hiding this comment.
In this file double is defined multiple times with the same or very similar implementation.
- Once at the top:
def double(value): return value * 2
- Then immediately again with the same body:
def double(value): return value * 2
- And later, another
double(number)for Exercise‑2 before redefining it again.
Because Python uses the last definition of a function name, the earlier versions are effectively dead code. If you ever decided to change the behaviour of double, you’d have to remember to change it in every place it’s defined in this file, or you could easily end up with different versions that behave differently.
How might you structure this file so that there is only one active implementation of double while still keeping your exercise answers clear? For example, could you keep the explanatory comments and prints, but call the same function definition instead of redefining it?
To "like" or "dislike" this comment, please follow this link
| if [[ "$1" == "individual-shell-tools" ]]; then | ||
| cd individual-shell-tools | ||
| pass=0 | ||
| total=0 | ||
| for directory in */; do | ||
| cd $directory | ||
| if [ "$directory" == "helper-files/" ]; then | ||
| cd .. | ||
| continue | ||
| fi | ||
| for exercise in *.sh; do | ||
| total=$(($total+1)) | ||
| ./$exercise > ../../test.tmp | ||
| cmp ../../test.tmp ../../expect/individual-shell-tools/$directory$exercise | ||
| if [ $? -eq 0 ]; then | ||
| pass=$(($pass+1)) | ||
| else | ||
| if [[ "$exercise" == *"stretch"* && $(stat -c%s ../../test.tmp) == "0" ]]; then | ||
| # Stretch task not attempted, ignoring" | ||
| total=$(($total-1)) | ||
| else | ||
| echo "Failed $directory$exercise, please check" >> ../../testoutput.txt | ||
| fi | ||
| fi | ||
| done | ||
| cd .. | ||
| done | ||
| cd .. | ||
| rm test.tmp | ||
| echo "You passed $pass/$total tasks." >> testoutput.txt | ||
| if [ -v GITHUB_OUTPUT ]; then | ||
| echo "attempted=y" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| if [ $pass -ge $total ]; then | ||
| echo "This task is complete!" >> testoutput.txt | ||
| if [ -v GITHUB_OUTPUT ]; then | ||
| echo "complete=y" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The script has several branches (individual-shell-tools, shell-pipelines, jq, number-systems, implement-cowsay) that repeat very similar patterns:
- Initialising
pass/total - Looping over exercises, writing output to
test.tmp - Comparing with an expected file using
cmp - Updating
pass/total - Writing failure messages to
testoutput.txt - Cleaning up
test.tmp - Optionally setting
attempted/completeflags inGITHUB_OUTPUT
For example:
- The
individual-shell-tools,shell-pipelines, andjqbranches all do a nearly identicalpass/totalloop and comparison, just with slightly different directory paths. - The pattern for setting
attempted=yandcomplete=ywhen all tests pass is also duplicated across multiple branches.
This kind of duplication can become a maintenance problem: if you ever need to change how results are counted, how failures are reported, or how GITHUB_OUTPUT is written, you’ll have to remember to update all the branches in exactly the same way.
Bash allows you to define functions. Could you imagine extracting a small function (or a couple of them) that encapsulate the common “run tests for a directory” pattern, and then have each branch call that with different arguments (like the base directory and the expect path)? That way, most of the logic would live in one place and each branch would mainly specify what’s unique about that task.
To "like" or "dislike" this comment, please follow this link
| if [[ "$1" == "individual-shell-tools" ]]; then | ||
| cd individual-shell-tools | ||
| pass=0 | ||
| total=0 | ||
| for directory in */; do | ||
| cd $directory | ||
| if [ "$directory" == "helper-files/" ]; then | ||
| cd .. | ||
| continue | ||
| fi | ||
| for exercise in *.sh; do | ||
| total=$(($total+1)) | ||
| ./$exercise > ../../test.tmp | ||
| cmp ../../test.tmp ../../expect/individual-shell-tools/$directory$exercise | ||
| if [ $? -eq 0 ]; then | ||
| pass=$(($pass+1)) | ||
| else | ||
| if [[ "$exercise" == *"stretch"* && $(stat -c%s ../../test.tmp) == "0" ]]; then | ||
| # Stretch task not attempted, ignoring" | ||
| total=$(($total-1)) | ||
| else | ||
| echo "Failed $directory$exercise, please check" >> ../../testoutput.txt | ||
| fi | ||
| fi |
There was a problem hiding this comment.
In the individual-shell-tools branch of this script, the nesting inside your loops and conditionals becomes quite deep. If you count the levels in the stretch-exercise check, you reach more than six levels:
if [[ "$1" == "individual-shell-tools" ]]for directory in */; dofor exercise in *.sh; doif [ $? -eq 0 ]; then/ itselseif [[ "$exercise" == *"stretch"* && $(stat -c%s ../../test.tmp) == "0" ]]; then/ itselse- The body of that inner
if/elseblock
This is all in the area around the stretch task handling, roughly lines 10–33. When nesting gets this deep, it becomes harder to visually follow which if matches which fi, and to reason about what happens on each code path (e.g. when a stretch test is skipped vs. when a failure is reported). That can make future debugging or small changes quite error‑prone.
You might find it helpful to think about ways to reduce one or two levels of nesting here. For example:
- Could any of the conditions be inverted with an early
continueto avoid wrapping the rest of the logic in anotherif? - Would pulling the "run one exercise and compare output" logic into a separate helper (even just a separate script or a function) help flatten the main loop?
- Is there a way to separate the handling of stretch exercises from non‑stretch exercises so they don’t both live inside the same nested
if/elsechain?
Thinking about questions like these can help keep the core structure of the loop at a simpler depth so it’s easier to read and maintain.
To "like" or "dislike" this comment, please follow this link
5 participants
No description provided.