4
\$\begingroup\$

I wrote this, and I would like to know if this code is clean.

It lets me know if a point is in the middle of an isometric cell.

const getMapPointFromCellId = cellId => {

    var {x, y} = getCoordinatesFromCellId(cellId)

    x = x * constants.cellWidth
    if (y % 2) x += constants.offsetWidth

    y = y * constants.offsetHeight 


    return {x, y}
}
const getCellIdFromMapPoint = (x, y) => {
   var cellId = 0
   while(cellId < constants.numberCells){

       const cellCoordinates = getMapPointFromCellId(cellId)

       const offsetX = cellCoordinates.x + constants.offsetWidth
       const offsetY = cellCoordinates.y + constants.offsetHeight

       const endX = cellCoordinates.x + constants.cellWidth
       const endY = cellCoordinates.y + constants.cellHeight


       if (cellCoordinates.x < x && x < cellCoordinates.x + constants.cellWidth)
          if (cellCoordinates.y < y && y < cellCoordinates.y + constants.cellHeight){

            if (x < offsetX && y < offsetY) 
                if ((offsetY - y) / (cellCoordinates.x - x) > (offsetY - cellCoordinates.y) / (cellCoordinates.x - offsetX)) return cellId
            if (x > offsetX && y < offsetY)
                if ((offsetY - y) / (endX - x) <= (offsetY - cellCoordinates.y) / (endX - offsetX)) return cellId
            if (x < offsetX && y > offsetY) 
                if ((offsetY - y) / (cellCoordinates.x - x) <= (offsetY - endY) / (cellCoordinates.x - offsetX)) return cellId
            if (x > offsetX && y > offsetY) 
                if((offsetY - y) / (endX - x) >= (offsetY - (constants.cellHeight - constants.offsetHeight)) / (endX - offsetX)) return cellId

        }

       cellId++
   }

  return -1
}
\$\endgroup\$
  • \$\begingroup\$ why the hate for for-loops? \$\endgroup\$ – le_m Jan 4 '18 at 22:51
  • \$\begingroup\$ @le_m the hate for for-loops? I just like while ^^ \$\endgroup\$ – ken Jan 4 '18 at 22:54
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to give it a different title if there is something more appropriate. Also, can you please include the code for getMapPointFromCellId? until then, this post is basically off-topic \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 4 '18 at 23:06
  • \$\begingroup\$ @SamOnela Thank you. Sorry for errors, I have added the missing code \$\endgroup\$ – ken Jan 4 '18 at 23:11
2
\$\begingroup\$

General javascript style notes

  • Clean javascript has semicolons in the right places.
// bad
const foo = 1

// good
const foo = 1;
  • Clean Javascript always uses delimited blocks
// worst
if(foo === bar) poo = 1

// bad
if(foo === bar) poo = 1;

// good
if (foo === bar) {
    poo = 1;
}
// or
if (foo === bar) { poo = 1; }
// or to satisfy some linters semicolons only at line end
if (foo === bar) { poo = 1 }
  • Clean Javascript does not repeat itself or have superfluous content

Bad

   const endX = cellCoordinates.x + constants.cellWidth
   const endY = cellCoordinates.y + constants.cellHeight


   if (cellCoordinates.x < x && x < cellCoordinates.x + constants.cellWidth)
      if (cellCoordinates.y < y && y < cellCoordinates.y + constants.cellHeight){

Good

    const endX = cellCoordinates.x + constants.cellWidth
    const endY = cellCoordinates.y + constants.cellHeight 

    // only one if statment needed
    if (cellCoordinates.x < x && x < endX && cellCoordinates.y < y && y < endY){
  • General rule is no calculations inside statements
// bad
if((offsetY - y) / (endX - x) >= (offsetY - (constants.cellHeight - constants.offsetHeight)) / (endX - offsetX)) return cellId


// good
const ySlope = (offsetY - y) / (endX - x);
const yOffsetSlope = 
   (offsetY - (constants.cellHeight - constants.offsetHeight)) /
   (endX - offsetX);

if (ySlope >= yOffsetSlope) {
    return cellId;
}

More points

It is nice to have descriptive names, it is not nice when descriptive names get in the way of code readability. It is even worse when names are in conflict.

// a map point is a cell coordinate???
const cellCoordinates = getMapPointFromCellId(cellId)

// but the function called gets the cell coordinates
const getMapPointFromCellId = cellId => {
    var {x, y} = getCoordinatesFromCellId(cellId)
    x = x * constants.cellWidth
    if (y % 2) x += constants.offsetWidth
    y = y * constants.offsetHeight 
    return {x, y}
}

Should the variable not be const cellMapPoint = getMapPointFromCellId(cellId)

And constants is meaningless out of context.

And speaking of context you prefixe some variable names with cell and others not.

// prefixed
var cellId = 0    
while(cellId < constants.numberCells){
    // prefixed
    const cellCoordinates = getMapPointFromCellId(cellId)
    // not prefixed
    const offsetX = cellCoordinates.x + constants.offsetWidth
    const offsetY = cellCoordinates.y + constants.offsetHeight
    // not prefixed
    const endX = cellCoordinates.x + constants.cellWidth
    const endY = cellCoordinates.y + constants.cellHeight

It is clear that the function is dealing with cells and that you don't need to prefix some names so why prefix others??

And from the code I can not workout if you are using an Id or an index. An id is different than an index and you would not test an id against a count while(cellId < constants.numberCells){ nor would you increment an id without testing if it exists so I think its an index.

const getCellIndexFromMapPoint = (x, y) => {
   var index = 0;
   const cells = constants; // ALIAS

   while(index < constants.numberCells){
       const mapPoint = getMapPointFromCellIndex(index);

       const offsetX = mapPoint.x + cells.offsetWidth;
       const offsetY = mapPoint.y + cells.offsetHeight;

       const endX = mapPoint.x + cells.cellWidth;
       const endY = mapPoint.y + cells.cellHeight;

       if (mapPoint.x < x && x < endX && mapPoint.y < y && y < endY){
           // and so on
       }
       index += 1;
   }

Axonometric projections

With all that said the whole function is a problem as is doing a search when a direct index can be found.

* note that the code below is off the top of my head and may contain typos, it is a suggestion and you should do some extra research on axonometric projections to create code more suited to your needs.*

All axonometric projections (Isometric is a particular type of axonometric projection) can be defined as 4 2d vectors that define the direction and scale of each of the 3D axis X,Y,Z and the location of the origin. If we ignore the Z as we are after a cell coordinate the problem can be simplified even further.

enter image description here

Consider the image above, The x axis and y axis are defined as a 2D vector each with an X and y magnitude that usually defines the size of a pixel. The origin defines where in screen space the coordinate 0,0 will appear.

You can define the axis as a matrix

const projection = [
    1, 0.5,  // direction and size of x axis
    -1, 0.5,  // direction and size of y axis
    0, 0     // the origin
];

From that matrix you can calculate a screen position for a pixel

function getScreenPos(x, y, screenPos = {}) {
    const P = projection; // alias
    screenPos.x = x * P[0] + y * P[2] + P[4];
    screenPos.y = x * P[1] + y * P[3] + P[5];
    return screenPos;
}

Create a projection

In that projection matrix is all you need to convert from screen space to world space (the reverse of the above function). So when you define the projection matrix you also define the inverse matrix

function defineProjection(origin, xAxis, yAxis){

    const m = [  // the projection matrix
        xAxis.x, xAxis.y,
        yAxis.x, yAxis.y,
        origin.x, origin.y,
    ];
    const im = [];  // the inverse matrix

    // get the determinate (I call it cross as it is a cross product of two vectors)
    const cross = m[0] * m[3] - m[1] * m[2];
    im[0] =  m[3] / cross;
    im[1] = -m[1] / cross;
    im[2] = -m[2] / cross;
    im[3] =  m[0] / cross;    

    // define the projection matrix object

    return {
        matrix : m,
        invMatrix : im,
        toScreen(worldPos, screenPos = {}) {
           const W = worldPos; // alias
           screenPos.x = W.x * m[0] + W.y * m[2] + m[4];
           screenPos.y = W.x * m[1] + W.y * m[3] + m[5];
           return screenPos;
        },
        toWorld(screenPos, worldPos = {}) {
           const x = screenPos.x - m[4];
           const y = screenPos.y - m[5];
           worldPos.x = x * im[0] + y * im[2];
           worldPos.y = x * im[1] + y * im[3];
           return worldPos;
        },  
    };
 }         

Create a map.

Now if you have a regular grid of cells that is defined as an array with a modulo. mapDesc defines the number of cells across and down. cellDesc defines the size of each cell. items are the cells as an array where a cell's position is related to the array index

var worldPos = {
    x : (index % cells.mapDesc.width) * cells.cellDesc.width,
    y : (index / cells.mapDesc.width | 0) * cells.cellDesc.height
}
// and screen pos is
var screenPos = cells.projection.toScreen(worldPos);

The object below is a bare bones simple version

 const cells = {
     items : [], // contains cells
     projection : defineProjection( 
         { x : 1, y : 0.5 },  // x axis
         { x : -0.5, y : 1 }, // y axis
         { x : 0, y : 0 },    // origin
     ),
     mapDesc : {
        width : 32,  // number cells 
        height : 32,
     },
     cellDesc : {
        width : 64,
        height : 64,
     },
     getCellAt(screenPos) {
         const worldPos = cells.projection.toWorld(screenPos);
         worldPos.x /= cellDesc.x;
         worldPos.y /= cellDesc.y;
         worldPos.x |= 0;  // floor
         worldPos.y |= 0;  // floor
         if (worldPos.x >= 0 && worldPos.x < cells.mapDesc.width &&
         worldPos.y >= 0 && worldPos.y < cells.mapDesc.height) {
              const cellIndex = worldPos.x + worldPos.y * mapDesc.width;
              return cells.items[cellIndex];
         }
     },
 }

The above object defines an array of cells (items) and a description of the projection used and details about a cell size and details about the cell layout.

You can call the function getCellAt({x : ?, y : ?}) and it returns a cell if you have given a coordinate over a cell. It does it very fast compared to a search.

Improved search

Even if the cells are randomly placed and you need to do a search, converting to world coordinates means you can work in axis aligned coordinates rather than the complex inversion you do for each cell.

// your search function need only do
const worldPos = projection.toWorld({x,y});
const endX = cell.x + constants.cellWidth
const endY = cell.y + constants.cellHeight
if (cell.x < worldPos.x && worldPos.x < endX && 
cell.y < worldPos.y && worldPos.y < endY){    
    return index;
}
\$\endgroup\$
  • \$\begingroup\$ Your solution seems much more adapter, but I do not understand it yet; I will need time. I corrected the first code you provided me, and thanks. But keeping this first function is correct. I do not understand why it's a problem \$\endgroup\$ – ken Jan 6 '18 at 11:07
  • \$\begingroup\$ @ken I don't know how the rest of your code is structured, but doing a search to find a map (world) location is slow. I provided a method of converting between map (world) coordinates and screen coordinates and back to make it a lot easier to test if a point is over a cell. \$\endgroup\$ – Blindman67 Jan 6 '18 at 11:18
  • \$\begingroup\$ I think it's not slow enough to realize slowness compared to your code? No? \$\endgroup\$ – ken Jan 6 '18 at 12:05
  • \$\begingroup\$ @ken It depends on the number of cells, how often you are searching and how they are arranged, all of which I don't know. \$\endgroup\$ – Blindman67 Jan 6 '18 at 12:26
2
\$\begingroup\$

I always like to document my functions using high-level doc-blocks, e.g.

/**
 * Return the ID of the cell whose center is at the given point or -1 if ...
 *
 * @param {number} x - The x coordinate of the given point.
 * @param {number} y - The y coordinate of the given point.
 * @return {number} cell ID or -1 if no cell found at given point.
 */
const getCellIdFromMapPoint = (x, y) => { ... }

Also, I use inline comments to help the reader and my future self to understand complex statements such as

// Do C if A and B or C:
if (x < offsetX && y < offsetY) 
   if ((offsetY - y) / (cellCoordinates.x - x) > (offsetY - cellCoordinates.y) / (cellCoordinates.x - offsetX)) return cellId

Better yet, take those complex statements as a hint to better refactor your code.

I advise to use available language constructs where appropriate. If you replace a simple for-loop with a while-loop for no apparent reason, a fellow developer will have a harder time to read and understand your code.

Also, I really recommend to use curly brackets with if-statements. The lack of those might be more pleasing to the eye by hiding the deep nesting, but makes reading and understanding as well as modifying your code much harder.

I suggest to combine if-clauses with identical statements. Instead of writing e.g.

if (A)
  if (B)
   return cellId;
if (C)
  if (D)
   return cellId

write e.g.

if ((A && B) || (C && D)) {
  return cellId;
}

Better yet, write helper functions to reduce the complex conditions, e.g.

if (isSomeCondition(A, B) || isSomeOtherCondition(C, D)) {
  return cellId;
}

Give those functions clear, self-documenting names to help the reader along. In your case, those could be e.g. isWithinRect(x, y, rect) or similar.

Your algorithm's runtime complexity is probably linear in the number of available cells. If you find that your application is running slow because of that, you can employ techniques such as spatial partitioning to improve runtime performance.

\$\endgroup\$
  • \$\begingroup\$ Nitpicks... If you're going to document functions you should use the proper syntax so your IDE can understand your functions. While loops, while less concise, are still just as common as for loops. If a dev can't understand a while loop they shouldn't be maintaining anybody's code. Other than that, great suggestions. \$\endgroup\$ – I wrestled a bear once. Jan 5 '18 at 14:35
  • \$\begingroup\$ @Iwrestledabearonce. Good advice regarding professional documentation. AFAIK the given syntax is proper though, so the comment should already pop up in an IDE / be included in autogenerated docs. Regarding while-loops: There are of course good use-cases for those, but here OP initializes a counter, loops till the counter reaches a certain limit and increments within the loop body - that's basically what for-loops have been made for. Seeing a for-loop there with all those loop counter related statements in one single place would have speed up my understanding of the code. \$\endgroup\$ – le_m Jan 5 '18 at 15:06
  • \$\begingroup\$ ok, fair enough. regarding commenting tho, the IDE will read your entire statement as a description rather than actually documenting the params and return types. i defend while loops because i frequently find myself in the opposite position, writing loops like for(;someCondition;) ...ignoring the first and the last statements when i really should be using a while loop instead. \$\endgroup\$ – I wrestled a bear once. Jan 5 '18 at 15:12
  • \$\begingroup\$ @Iwrestledabearonce. Yep, those teary eyed for (;;;) are equally bad... I included some more annotations so OP has a better impression of the power of documenting comments. \$\endgroup\$ – le_m Jan 5 '18 at 15:18
  • \$\begingroup\$ Though not your fault this does illustrate the problem with documentation. MapPoint as far as I can tell is a world coordinate, thus the documentation should clearly define that it is not just a coordinate, and it is completely unclear in the code as to what ID is, from the look of the code it is an index not an ID. Documentation should show the abstracted data meaning, and if not available it should not make guesses (again not your fault just pointing out the dangers). \$\endgroup\$ – Blindman67 Jan 5 '18 at 15:58
1
\$\begingroup\$
  • Combine your conditionals. especially this first one:

    var cellWidthFits = cellCoordinates.x < x && x < cellCoordinates.x + constants.cellWidth;
    var cellHeightFits = cellCoordinates.y < y && y < cellCoordinates.y + constants.cellHeight
    if(cellWidthFits && cellHeightFits)
    
  • If you can't fit the entire if statemtment on a single line, please use the curly brackets. Like the semicolons, they're not required, but your leaving yourself open to bugs. In fact Apple recently broke the internet because some dude thought it was a good idea to leave the braces out.

  • Be consistent with your operators. If you're gonna use += to add stuff then you should use *= to multiply stuff.
  • Be aware that your constants aren't constant if you store them in an object. Even if you use the const keyword to declare them. That's not how JS "constants" work. You should either rename your constants object to something more accurate (like globals or preferences) or you should store them as individual strings an numbers with the const keyword so they really will be constant and use UPPERCASE variable, as is the convention.
  • Semicolons. End your statements with them. If you don't add them then the JS interpreter will guess where to put them and that's a disaster waiting to happen. See this thread for more info.
  • Minor nitpick. You should, in my opinion, put a space between your functions to separate them a little bit.
  • Please be consistent with your white-space. I don't think you need any blank lines in the first function, but you surely don't need two blank lines before the return.

const getMapPointFromCellId = cellId => {
  var {x, y} = getCoordinatesFromCellId(cellId);
  x = x * constants.cellWidth;
  if (y % 2) x += constants.offsetWidth;
  y = y * constants.offsetHeight;
  return {x, y};
};

const getCellIdFromMapPoint = (x, y) => {
  var cellId = 0;
  while (cellId < constants.numberCells) {
    const cellCoordinates = getMapPointFromCellId(cellId)
    const offsetX = cellCoordinates.x + constants.offsetWidth;
    const offsetY = cellCoordinates.y + constants.offsetHeight;
    const endX = cellCoordinates.x + constants.cellWidth;
    const endY = cellCoordinates.y + constants.cellHeight;
    const inRange =
      cellCoordinates.x < x && x < cellCoordinates.x + constants.cellWidth &&
      cellCoordinates.y < y && y < cellCoordinates.y + constants.cellHeight &&
      (
        x < offsetX && y < offsetY &&
        (offsetY - y) / (cellCoordinates.x - x) > (offsetY - cellCoordinates.y) / (cellCoordinates.x - offsetX)
      ) || (
        x > offsetX && y < offsetY &&
        (offsetY - y) / (endX - x) <= (offsetY - cellCoordinates.y) / (endX - offsetX)
      ) || (
        x < offsetX && y > offsetY &&
        (offsetY - y) / (cellCoordinates.x - x) <= (offsetY - endY) / (cellCoordinates.x - offsetX)
      ) || (
        x > offsetX && y > offsetY &&
        (offsetY - y) / (endX - x) >= (offsetY - (constants.cellHeight - constants.offsetHeight)) / (endX - offsetX)
      );
    if (inRange) return cellId;
    cellId++
  }
  return -1;
};
\$\endgroup\$
  • \$\begingroup\$ I am seriously tempted to downvote for your (presumably supposed to be funny) comic. It is perfectly fine to write JS without semicolons unless your team's style guide tells you to use them. JS Standard style, Blog... I personally prefer no semicolons, but will use whatever the project I am working on currently uses. \$\endgroup\$ – Gerrit0 Jan 6 '18 at 23:06
  • \$\begingroup\$ yes obviously it was supposed to be funny, but it's also best practice for a number of reasons. if you don't like my answer feel free to downvote but please spare me the petty nonsense next time. \$\endgroup\$ – I wrestled a bear once. Jan 7 '18 at 0:58
  • \$\begingroup\$ Please help me understand the number of reasons then, so far as I can tell using semicolons or not is purely a matter of opinion. The linked post doesn't present compelling proof either for or against. As practically every project includes a linter, and linters can easily warn about the cases that might cause issues. \$\endgroup\$ – Gerrit0 Jan 7 '18 at 1:39
  • \$\begingroup\$ @Gerrit0 Your advice is to leave out the semicolons "because you can"? Did you read the resources on the linked thread? How about the linked PDF from ECMA international explicitly saying you should always use the semicolons? This question has been asked and answered. If you don't understand it ask for clarification on SO or open a bounty on the linked one. \$\endgroup\$ – I wrestled a bear once. Jan 7 '18 at 3:07
  • \$\begingroup\$ I'm sorry I am not willing to try to understand someone whose first response to a legitimate question is to insult the asker. I did read the linked resources on the top 3 answers. As for the PDF, nope. That's not what it says. I'm done here. \$\endgroup\$ – Gerrit0 Jan 7 '18 at 3:13

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.