Hi, I’ve been working on this beat mix project and I wanted some help reviewing my code. First, I’ll show what I originally typed and then what I simplified it to.
I’m much happier with the simplified version, but is there any way to shorten the first if statement so I don’t have to have so many ‘or’ statements? And don’t have to keep typing the string name of the variables?
I’ve also read that using ‘eval’ is a bad idea, any thoughts on alternatives?
Link to problem here
The result is supposed to change when I click on the drum buttons for toggleDrum. The arrays each have 16 values of false, when clicked they should change to true.
The function is supposed to take in a string, ‘kicks’, ‘snares’, etc. and then toggle the kicks, snares, etc. array at the given indexNumber.
I used eval, because this
eval('kicks')[indexNumber] = ...
translates (or so I think) to
kicks[indexNumber] = ...
and alters the array in the way I want, at the given index.
Now, I’ve passed all the tests, but it doesn’t seem that the mixer is working, so I guess this is wrong.
Answers to your questions:
The result is supposed to toggle every time I click on a box.
The if statement is there because kicks[indexNumber] is changing to the opposite of whatever it is (if true then it changes to false, if false then it changes to true).
It’s not wrong (not that part) but it’s overkill to execute a string as javascript code. you should probably be writing code that does what you mean instead.
so instead of executing strings, how about treating them like keys, so that, if you have a string, you can use it to look up an array. Sound familiar?
{
'kicks': kicks
}
If your function is supposed to have an effect, then don’t return a value as well. It gets really difficult to tell what the purpose of a function is when it does a bit of everything.
And, if you have true but want false, or vice versa, the relation there is NOT:
So you’d make a mapping from the string identifiers to the arrays… then look it up and make the adjustment.
You might want to skip the condition altogether, because all it’s really doing is suppressing an error that shouldn’t happen, and if something shouldn’t happen then you want an error.
Test the length instead of having magic numbers in there
(and I still don’t think it should be tested at all anything else is invalid input, is the callers fault, should make the program scream and protest)
doing something to each element is something you’d usually use map for:
[0, 3, 4].map((x) => x+1) // [1, 4, 5]
and again, don’t agree with the condition
same thing, map over it
this could be a more generic function:
given a value and an amount, return an array containing that value repeated that many times, you’d probably name this function replicate
When something’s wrong one has to be willing to make observations right? Otherwise it all devolves into wild guesswork, or something like: busted fuse -> replace the whole car
So “tests failed” isn’t the last observation that can be made. One would look at which test, maybe pick the first one that failed, and then, what particular action was it that the test couldn’t carry out, or what was different from what the test expected.
Not saying you particularly need to get that code to fit with the rest, but that’s always going to be how to fix things, to dig in.