mhomami / Tank

0 stars 0 forks source link

Consider making repeated code a function #3

Open andyg0808 opened 2 years ago

andyg0808 commented 2 years ago

https://github.com/mhomami/Tank/blob/74e115e25ae04a530c524b3d3756b83eb56cb745/js/script.js#L93-L95

You might consider defining a new function at the bottom of your script, something like

function randomY() {
  return Math.floor(Math.random() * _h);
}

Then you can write these lines as

 const y1 = randomY(); 
 const y2 = randomY();
 const y3 = randomY();

(incorporating the use of const at initialization point as mentioned in #2)

mhomami commented 2 years ago

I'm going to put some serious work into this. Right now I've done as you suggested but for some reason it prevents the later if...else statements from randomising the y axis entry point once the fish are reset. Regardless, I would like to create some functions to reduce this and everything after line 100 (maybe even from line 47?) down to single line statements executing a function call. TBC.

andyg0808 commented 2 years ago

Huh, that seems pretty strange. Just to double-check: you're only trying to change these lines? https://github.com/mhomami/Tank/blob/3d3d337ffd4504df40b43f608093b72b450c924a/js/script.js#L93-L95 and maybe this one https://github.com/mhomami/Tank/blob/3d3d337ffd4504df40b43f608093b72b450c924a/js/script.js#L99

If so, I'm not quite sure what's going wrong. You might try adding either console.log statements to log out the values in the places you're using them, or using the debugger in your browser to let you watch the code execute. If you add a breakpoint in one of the if statements, you can have it stop the code only when that if tries to run. (An intro to the debugger, in case you've not tried it before: https://developer.chrome.com/docs/devtools/javascript/)

Re. reducing everything to single-line statements: sounds like a worthwhile experiment. So you know, what constitutes a worthwhile function is (for me at least) a skill that's developed by practice and trying things out to see what is easier to work with and what's harder. So I'd say, take a shot at it, but don't be afraid to leave some code inline if you feel like the function version is messier or doesn't have a good name available. These are the kinds of questions we devs debate in code review; mine is just one opinion.

mhomami commented 2 years ago

I've only changed line 93 which changes driver1's behaviour (the driver with the ifixit logo). Line 99 should be randomising the reentry point even when line 93 is changed as it overrides the initial placement. I'll do as you suggest, I'll run it through the debugger line by line.

Noted about worthwhile reductions. I'll certainly keep that in mind if I can get my functions to work! I swear python allowed you to create variables inside functions but maybe I'm losing it. Regardless, this is not Python!

andyg0808 commented 2 years ago

Are you sure it used to work? It looks to me like https://github.com/mhomami/Tank/blob/3d3d337ffd4504df40b43f608093b72b450c924a/js/script.js#L101 would be overwriting whatever value had gotten set to driver1sprite.y once the conditional on the if stops being true: https://github.com/mhomami/Tank/blob/3d3d337ffd4504df40b43f608093b72b450c924a/js/script.js#L108-L112 (also, I think that conditional is just jimmy1sprite.x < -600 because _w - (_w + 600) = _w - _w - 600 = -600)

And yup, both Python and JS allow variable creation inside of functions. In Python, you just assign to a variable to create it:

def demo():
  myVar = 42
  return myVar

In JS, the best way to declare a variable in a function is with let or const inside the function:

function demo() {
  const myVar = 42;
  return myVar;
}
mhomami commented 2 years ago

Success on compressing some of the code (thank you for the tip in your last comment!).

I didn't spend too much time on the y-axis issue. It was working but is definitely out of action now. I need to figure out how to create a y randomiser that "ticks" over like the animation function. This will eliminate 3 of 4 variables and simplify the code, likely will do away with the if...else statements too.

For tomorrow though. Feeling burnt out, going to do some light reading on tcp/ip protocols this evening.

mhomami commented 2 years ago

r.e: (also, I think that conditional is just jimmy1sprite.x < -600 because _w - (_w + 600) = _w - _w - 600 = -600)

This is my thought process, let me know if I misunderstood or if there is a better (and shorter!) way of writing the code. I've got a screen, say 1800px wide, and if I call j.x -600 what I seem to get is j.x = 1200. It takes the current width of the screen and subtracts my value. By subtracting _w from itself I can get the left most edge of the screen and subtract 600 giving me the -600 I'm looking for.

andyg0808 commented 2 years ago

If I'm following what you're saying, you're looking for "six hundred pixels off the left edge of the screen." The index of the left edge of the screen is just 0, as far as I can tell. Off the screen to the left seems to be negative. So we shouldn't need to care about the width of the screen at all, because we only care about the left edge. Thus,

if (jimmy1sprite.x < -600)

would be true when (probably the middle of) jimmy1sprite is 600 pixels offscreen to the left (although this would depend on which part of the sprite its position is measured from).

I think you already figured this out, just in a more obscured form. What I was pointing out is that the conditional here https://github.com/mhomami/Tank/blob/f511f204760a124abe93d721e8d52efadf64f177/js/script.js#L104 is precisely the same as if you wrote

  if (jimmy1sprite.x < -600){

because the two _ws cancel each other out and disappear, and you're left with just a plain -600 (you can work through the right-hand side of the inequality with algebra to show this). It seems like the right conditional, since jimmy1sprite and driver1sprite are moving the opposite direction from the rest of the fish.

mhomami commented 2 years ago

You are spot on, I rewrote those lines and all is well.

I fixed the y-axis issue but I'm still really confused as to why it wouldnt work. Take a look at lines 123-146:

https://github.com/mhomami/Tank/blob/6083e481ff263961aa3d2b40ada9cbf0ce13f056/js/script.js#L123-L146

I rewrote the code for the fish along the same lines and removed mention of them further up. I even console logged this, the y values in the if statement would execute and provide a different y value but for some reason the else statement would then reset the y-axis position of the fish. Current solution works, I'm just unhappy that I cant figure out why my previous code wouldnt (and why the solution works for the bits?). This took the better part of 3 hours and I dont feel good about that but sacrifice must be paid to the learning gods.