-1
\$\begingroup\$

I am working on a background project that is a little interactive. Now I am using some collision detection to draw some lines and stuff.

The code I have works great but it is very bulky and hard to read.

if (background.allPixels[i].location.x - drawLineBetweenPixelRange < background.allPixels[j].location.x + background.allPixels[j].width &&
    background.allPixels[i].location.x + background.allPixels[i].width + drawLineBetweenPixelRange > background.allPixels[j].location.x &&
    background.allPixels[i].location.y - drawLineBetweenPixelRange < background.allPixels[j].location.y + background.allPixels[j].height &&
    background.allPixels[i].location.y + background.allPixels[i].height + drawLineBetweenPixelRange > background.allPixels[j].location.y)

It comes down to running this for the x and y coordinates:

var result = (x - a < y + b) && (x + b + a > y)

Can this be simplified?

I whas hoping if the check itself could be shortened by taking stuff out that might counter eachother.

\$\endgroup\$
0
\$\begingroup\$

first accessing those properties all over again are really confusing

let bgI = background.allPixels[i]
let locI = bgI.location
let line = drawLineBetweenPixelRange
let bgJ = background.allPixels[j]
let locJ = bgJ.location


if (locI.x - line < locJ.x + bgJ.width &&
    locI.x + bgI.width + line > locJ.x &&
    locI.y - line < locJ.y + bgJ.height &&
    so on and so on...

second redefining all those variables all over again really hurts efficiency

function detectCollision(background, line) {
    let bgI = background.allPixels[i]
    let bgJ = background.allPixels[j]
    let locI = bgI.location
    let locJ = bgJ.location

    if (locI.x - line < locJ.x + bgJ.width &&
        locI.x + bgI.width + line > locJ.x &&
        locI.y - line < locJ.y + bgJ.height &&
        so on and so on....
}

detectCollision(background, drawLineBetweenPixelRange)

hope it helps

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Please go into more detail how it "hurts efficiency". Also note that your function uses global variables i and j and therefore will be a a nightmare to debug. \$\endgroup\$ – Zeta Nov 10 '18 at 8:21
  • \$\begingroup\$ Inefficient because: t gives space for human error, is not DRY, responsibilty of methods are not well separated. i and j could be anythig, global var, index of iteration, etc. So I'll leave the real implementation of i and j to OP. \$\endgroup\$ – Nikko Khresna Nov 10 '18 at 11:20
  • \$\begingroup\$ Thanks for the feedback. Since this code does not run again on any other place i hadn't thought about putting it in its own function. I will think about applying this but is not the answer i am looking for. I whas hoping if the check itself could be shortened by taking stuff out that might counter eachother. \$\endgroup\$ – JanWillem Huising Nov 10 '18 at 14:29
  • \$\begingroup\$ @NikkoKhresna Additional information should be put into your post. Please edit accordingly. Comments may get deleted when they aren't needed anymore and don't have a history, whereas posts do. \$\endgroup\$ – Zeta Nov 11 '18 at 12:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.