Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions app/components/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@ const Router = ReactRouter.BrowserRouter;
const Route = ReactRouter.Route;
const Switch = ReactRouter.Switch;
const Game = require('./Game');
const pathFind = require('../utils/BFS');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import findPath from '../utils/BFS';

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I've been googling on the difference between require() and import and got into a wall of not understanding the difference between 'dynamic' and 'static' loading. And that's besides the fact that most places say that import is being transpiled by Babel to require() anyway because there is no native support yet... :/

  1. Any chance for a link to clarify that difference between dynamic and static loading?

  2. you marked only the findPath to use with import. Is there any reason not to use import everywhere instead of the require() I've been using to for the other classes of the app? Or React for that matter?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It's a complex topic and I can write you the basics in chat.
  2. I only commented in this PR on changes, not on everything. If I'd comment on everything, we'd never finish. Gotta limit the scope, otherwise it's unmanageable. You should use import everywhere you can.

// const Settings = require('./Settings');
// const Results = require('./Results');

function boardReset(lengthX, lengthY) {

console.log('creating board')
console.log('path?');
console.log(pathFind);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't one console.log enough?

const board = [];
for (let i=0; i<lengthY; i++) {
board[i] = [];
Expand Down Expand Up @@ -44,18 +48,23 @@ class App extends React.Component {
this.handleClickOnBoard = this.handleClickOnBoard.bind(this);
this.handleStartClick = this.handleStartClick.bind(this);
this.handleEndClick = this.handleEndClick.bind(this);
this.handleGoClick = this.handleGoClick.bind(this);
this.updateStartLocation = this.updateStartLocation.bind(this);
this.updateEndLocation = this.updateEndLocation.bind(this);
this.updatePath = this.updatePath.bind(this);
}

handleClickOnBoard(x, y) {
console.log('trying to update state');
const board = this.state.board;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use destructuring, everywhere: const { board, } = this.state;

if (this.state.startFlag) {
this.updateStartLocation(x, y);
} else if (this.state.endFlag) {
this.updateEndLocation(x, y);
} else {
const tempBoard = this.state.board;
const tempBoard = board.map((row) =>{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't name things "temp". Even life is temporary, we all know that. Even "new" is better than "temp".
Name your mapping functions. This helps debugging and stack traces. Naming things in general helps you write better code.

return row.map((cell)=>Object.assign({}, cell));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shorter:

return row.map(cell => ({ ...cell, }));

But why clone the cell at all?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because I want to make changes to state only from this.setState(), and since board is a nested array of objects, if i make a change to the cell's content it will change the state without invoking the setState function.

})
tempBoard[y][x].allowed = this.state.board[y][x].allowed ? false : true;
console.log(tempBoard)
this.setState(() => {
Expand All @@ -82,6 +91,28 @@ class App extends React.Component {
})
}

handleGoClick() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle/calculate/do/perform/compute are all meaningless. Your functions must start with a verb which is clear and end with a subject: "drawPath", "drawPathStep", "killBill", "brewCoffee".
You can later assign those well named methods/functions as even handlers, or call them from event handlers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what this function does without reading it. I only know when someone was planning to use it.

// console.log('go pressed!')
const {start, end, board} = this.state;
const boardCopy = board.map((row) =>{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second time this function appears here. Extract to function or method to DRY it.

return row.map((cell)=>Object.assign({}, cell));
})
const path = pathFind(boardCopy, start, end);
const step = -1;
const addSteps = (path, step) => {
if (path.length === 0) {
return
}
this.updatePath(path[0],step+1)
const timeoutForStepAddition = window.setTimeout(addSteps.bind(null, path.slice(1), step+1), 500)
}
addSteps(path, step);
// async path.map( (step, index) => {
// console.log('update');
// await window.setTimeout(this.updatePath.bind(null,step,index), 500)
// })
}

updateStartLocation(x, y) {
this.setState(() => {
return {
Expand All @@ -100,6 +131,21 @@ class App extends React.Component {
})
}

updatePath(step, stepNum) {
const board = this.state.board;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Destructure

const tempBoard = board.map((row) =>{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3rd time same function

return row.map((cell)=>Object.assign({}, cell));
})
const [x, y] = step;
tempBoard[y][x].stepVisited = stepNum;

this.setState(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrow functions are good for brevity:

this.setState(() => ({ board: tempBoard, }));

Better: name your temp board 'board' and use ES2015 shorthand object literal

this.setState(() => ({ board, }));

Even better, specifically for React's setState, which can accept an object literal to merge with current state:

this.setState({ board, });

return {
board: tempBoard
}
})
}

render() {
return (
<Router>
Expand All @@ -112,7 +158,8 @@ class App extends React.Component {
handleClicks = {{
handleClickOnBoard: this.handleClickOnBoard,
handleStartClick: this.handleStartClick,
handleEndClick: this.handleEndClick
handleEndClick: this.handleEndClick,
handleGoClick: this.handleGoClick
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Destructuring FTW:

const { handleFoo, handleBar, } = this;
handleClicks = { handleFoo, handleBar, }

}}
updateLocations = {{
updateStartLocation: this.updateStartLocation,
Expand Down
12 changes: 8 additions & 4 deletions app/components/Game.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ function Cell(props) {

const {x, y} = props.cellData;
props = props.props;
const {board, colorA, colorB, start, end} = props.state;
const {board, colorA, colorB, colorC, start, end} = props.state;
const lengthY = board.length;
// const styles = {
// cell: {
Expand All @@ -20,7 +20,10 @@ function Cell(props) {
<div className = 'cellShell'
style = {{'height': (40/lengthY) + 'vh', 'width': (40/lengthY) + 'vh'}}>
<div className = 'cell'
style = {{'backgroundColor': (board[y][x].allowed ? colorA : colorB)}}
style = {{'backgroundColor':
(board[y][x].stepVisited >= 0) ? colorC : (board[y][x].allowed ? colorA : colorB)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreadable beyond even trying. You need to name things.


}}
onClick = {props.handleClickOnBoard.bind(null, x, y)}
>
{(start[0] === x) && (start[1] === y) && <div>start</div>}
Expand Down Expand Up @@ -155,7 +158,7 @@ function GameButtonsPanel(props) {
classes: 'btn success',
activated: () => {return false},
disabled: () => {return (!disabledCheck(start) || !disabledCheck(end))},
onClick: () => {},
onClick: props.handleClickOnButtons.handleGoClick,
}
];
console.log('buttons ok')
Expand Down Expand Up @@ -184,7 +187,8 @@ function GameScreen(props) {
<GameButtonsPanel state = {props.state}
handleClickOnButtons = {{
handleStartClick: props.handleClicks.handleStartClick,
handleEndClick: props.handleClicks.handleEndClick
handleEndClick: props.handleClicks.handleEndClick,
handleGoClick: props.handleClicks.handleGoClick
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Destructuring like in comment above

}}
/>
</div>
Expand Down
122 changes: 122 additions & 0 deletions app/utils/BFS.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
const pathFind = (field, startP, endP) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. functions should start with a verb: findPath
  2. Don't abbreviate: startPoint, endPoint or pointStart/End. I'm assuming "P" is for "point", but don't know. Could be "path" or "properties" or "pop corn". Don't make the reader guess, ever.
  3. Don't use arrow functions unless you need their features: lexical binding and implicit return.
  4. I follow the rule that from the third parameter and on, the function should be refactored to a single object argument with destructuring.
    Summarizing:
function findPath({ field, startPoint, endPoint, }){

const path = [];

// tests if the point is inside the tested field
var inField = function (point) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't var
A boolean or a function returning one should start with a predicate: "isInField". Predicates: is, has, did, does, can, will, should, etc.

if (point[0]<field.length && point[0] >=0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use destructuring to name all those unreadable indexes

if (point[1]<field[0].length && point[1] >=0) {
return true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your function can return undefined here.

}
return false;
}

// tests if the point was already
var visited = function (point) {
if (field[point[0]][point[1]].stepVisited<0) {
return false;
}
return true;
}

// tests if a certain step is allowed
var allowedStep = function (point) {
if (field[point[1]][point[0]].allowed) {
return true;
} else if (!field[point[0]][point[1]].allowed) {
return false;
}
}

// tests if the tested point is in fact the ending point of the search
var isEndPoint = function(point) {
if (point === endP) {
return true;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't else after return

return false;
}
}

// returns the neighbours of a certain point
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments don't help. Name your identifiers so you won't need comments: getPointNeighbours

var getNeighbours = function (point) {
var neighbours = [];
var movementX = [1, -1, 0, 0];
var movementY = [0, 0, 1, -1];
for (var i=0; i<4; i++) {
let neighbour = [];
neighbour[0] = point[0] + movementY[i];
neighbour[1] = point[1] + movementX[i];
neighbours.push(neighbour);
}
return neighbours;
}

// tests if a certain point can be used (inside the board && allowed to be stepped in && was not visited)
var legalPoint = function (point) {
if (inField(point)) {
if (allowedStep(point)) {
if (!visited(point)) {
return true;
}
}
}
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return isInField(point) && isAllowedStep(point) && !hasBeenVisited(point);

}

// returns the neighbours that are valid points to be used
var legalNeighbours = function (neighbours) {
var legal = [];
neighbours.forEach(function(neighbour) {
if (legalPoint(neighbour)) {
legal.push(neighbour);
}
})
return legal;
}

// the BFS algorithm that searches for a path between the 'start' and 'end' points
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A link to an explanation is better

// returns 'true' if a path was found and 'false' if not
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant for a well named boolean function

var pathExists = function() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you copy/pasted if from somewhere, add a link.

if (legalPoint(startP) && legalPoint(endP)) {
var testingQue = [startP];
field[testingQue[0][0]][testingQue[0][1]].stepVisited = 0;
while(testingQue.length>0) {
let currStep = testingQue[0];
if (currStep[0] === endP[0] && currStep[1] === endP[1]) return true;

let currStepNum = field[currStep[0]][currStep[1]].stepVisited;
let allowedSteps = legalNeighbours(getNeighbours(currStep));
allowedSteps.forEach(function(nextStep){
field[nextStep[0]][nextStep[1]].stepVisited = currStepNum+1;
field[nextStep[0]][nextStep[1]].previous = currStep;
testingQue.push(nextStep);
})
testingQue.shift();
}
}

return false;
}

// a function that filles the 'path' object with pairs of key = step number : value = the tile location
var buildPath = function() {
var stepNum = field[endP[0]][endP[1]].stepVisited;
var currStep = endP;
path[0] = startP;
while (stepNum > 0) {
path[stepNum] = currStep;
stepNum--;
currStep=field[currStep[0]][currStep[1]].previous;
}
}

if (pathExists()) {
buildPath();
// gameService.pathExistsFlag = true;
}

return path;

}

module.exports = pathFind;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export default findPath