-
-
Notifications
You must be signed in to change notification settings - Fork 261
Clean up unnecessary exercises #233
base: main
Are you sure you want to change the base?
Conversation
I don't see much value in getting the trainee to create the file
This exercises were related to strings, not arrays
The manadatory exercises didn't include some concepts that were covered in the lesson, so I've just moved some of the in-class exercises into the mandatory section. Looks like the exercises in the syllabus don't match the in-class exercises now anyway
This involves a bit of mocking of the console global, as otherwise it's difficult to test side effects which is somewhat necessary with forEach (otherwise why not just use a map?)
@@ -3,16 +3,14 @@ Write a function that: | |||
- Accepts an array as a parameter. | |||
- Returns a new array containing the first five elements of the passed array. | |||
*/ | |||
function first5() { | |||
} | |||
function first5() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can we get rid of this function too whilst we're here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow - are you asking to remove this (sub-)exercise? Or to remove the stubbed function?
If you're asking for the latter, I think we do need it as we call this specific named function in the tests, so getting trainees to write it themselves risks misnaming, which could lead to confusing errors. I guess I would be fine with going down that route, but that feels like a policy decision that should be taken across the board.
mandatory/12-fizz-buzz.js
Outdated
To run the tests for just this one file, type `npm test -- --testPathPattern 12-fizz-buzz` into your terminal | ||
(Reminder: You must have run `npm install` one time before this will work!) | ||
=================================================== | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need additional test cases for smaller values here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not sure I follow what you're asking here?
Is your question that we're not testing for example fizzBuzz([1, 2, 3])
? If so, then I can see it both ways: the existing test has assertions for all of those values, but we'd lack good test descriptions for those. Personally I would probably add more individual test cases with explicit test descriptions but that didn't seem to be the style in other test files in the coursework.
Closes CodeYourFuture/syllabus#502.
Summary of changes
exercises
code & content (which doesn't match the syllabus any more)More mandatory exercises
I noticed that the mandatory exercises don't cover all of the content from the lesson (e.g. some array methods weren't used by the tests), so I've moved some of the in-class exercises into the mandatory folder. I did some minimal work to convert them to actual tests instead of just expected output comments.
Given that the purpose of this work was to reduce the workload, I'm not 100% convinced this is the best choice - so I've included these as the last two commits (4df079d and 4ece057). I'm happy to drop these commits if we feel that they aren't necessary.