5
\$\begingroup\$

I have some JavaScript code to manage a 2D map in a game. I have a function to create a new room in the map which is at map[x][y]. I need to create that property and also initialize the monsters property. The code I wrote looks like this, which works fine.

'use strict';

let map = {};

function newRoom(x, y) {
    if (map[x] === undefined)
        map[x] = {};
    map[x][y] = {};
    map[x][y].monsters = {};
}

newRoom(0, 0);

Is there a more concise way to write code like this?

\$\endgroup\$
1
  • \$\begingroup\$ It's already a great question, but I would love to see a new question with more of this code. \$\endgroup\$ – konijn Jan 3 at 13:24
3
\$\begingroup\$

Fundamentally, not really. When you need to create a nested structure, there are no built-in ways to get around explicitly testing if the intermediate object(s) exist, and creating them if they don't.

(If you just wanted to access a possibly-existing nested element, you could use optional chaining to achieve it concisely, but nested creation is much more verbose)

There are libraries that do nested creation for you, but they ultimately implement the same sort of logic under the hood:

// Just for demonstration, I do not recommend using a library for this:

let map = {};
function newRoom(x, y) {
  _.setWith(map, [x, y, 'monsters'], {}, Object);
}
newRoom(0, 0);
console.log(map);
<script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js"></script>

One shortcut you can make in your original code is to simply check map[x] instead of comparing against undefined. You can also declare the monsters object inline when assigning the room. You should also declare the map object with const unless you're reassigning it later.

Now that logical nullish assignment exists, you could use it as well, though given how new it is, you (or others who may read the code) may not be comfortable with it, and it may look a bit confusing regardless:

const map = {};

function newRoom(x, y) {
    map[x] ??= {};
    map[x][y] = {
        monsters: {}
    };
}

/*
or
function newRoom(x, y) {
    if (!map[x]) {
      map[x] = {};
    }
    map[x][y] = {
        monsters: {}
    };
}
*/

newRoom(0, 0);
console.log(map);

I do recommend using brackets for your if blocks. They'll make it a bit easier to read and will keep you from accidentally writing on the wrong indentation level assuming that something is inside the if block, when it actually isn't.

Note that both the above code and your original code will overwrite contents of the room if it already exists in the map. Hopefully that's desirable.

There are a few alternative methods to achieve the same result that require fewer characters of code, that code golfers or minifiers could use, but they sacrifice readability and therefore aren't worth it IMO, eg:

const map = {};

function newRoom(m,n){map[m]||(map[m]={}),map[m][n]={monsters:{}}}

newRoom(0, 0);
console.log(map);

Another thing to consider - as of ES2015, Maps are more suited to dynamic properties than objects. If you want a Map, use a Map instead of an object.

function newRoom(x, y) {
    if (!map.has(x)) {
        map.set(x, new Map());
    }
    map.get(x).set('y', { monsters: {} });
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Good answer. I just wanted to add the fact that while there might be ways of achieving what OP asked, none of the above is as readable and easier to understand as OP’s \$\endgroup\$ – Grajdeanu Alex Jan 3 at 7:29
  • \$\begingroup\$ Not even the or section? I'm not sold on the ??= either, but I think using just a truthy test for the if, using const instead of let, and declaring the monsters sub-object inline all to be improvements over the original? \$\endgroup\$ – CertainPerformance Jan 3 at 14:47
  • \$\begingroup\$ Don’t get me wrong, they’re all improvements definitely! I was strictly talking about the readability of the code :p \$\endgroup\$ – Grajdeanu Alex Jan 3 at 14:49
2
\$\begingroup\$

TL;DR

This code snippet is rather taken out of context and is likely symptomatic of bigger issues in the codebase as a whole. You may have bigger problems than trying to improve these few lines of object initialization.


A few minor points...

For starters:

'use strict';

Good start!


let map = {};

Use const. Using let says "I'm going to re-assign this value later" which is almost never what you want to do except for loop counters.


    if (map[x] === undefined)
        map[x] = {};

Always use block braces {} on conditions.

If your "contract" behind map is that it only ever has two values, undefined and {} then if (map[x]) seems clearest to me and robust to changes like swapping undefined out for null. This is a minor point--the === undefined is not bad.


Respect scope and consider encapsulation

newRoom has a serious problem: it breaks scope and relies on a hardcoded global, map. It can't be re-used with arbitrary maps. If you change the name of the global map to bigMap or something, the function breaks for no good reason. At best, a call to newRoom has no obvious connection to map.

With this in mind, a better function header is newRoom(map, x, y). This communicates explicitly which map is being initialized, it works on any map-type object we pass in and it won't break if you do something simple like change a variable name elsewhere in your application.

Now, even if you pass map as a parameter, the function is still impure because it mutates the object. It's looking like C-style OOP with the object passed in as the first argument to the function. If you need to mutate a property on an object repeatedly like this or have many functions like foo(map, ...), bar(map, ...), baz(map, ...), consider attaching a prototype function to it (or using the class syntactic sugar) so you can use map.newRoom(x, y). A benefit is that the function newRoom "belongs" to the map object (or its prototype/class) rather than drifting out in the global scope with no obvious connection to anything. When map.newRoom(x, y) mutates data on this, it's easy to reason about what's going on because the state is encapsulated in an instance.

Another approach is to allocate and return a totally new map with the added room, keeping the function entirely free of side effects. This is the safest option.

Making this design call relies on context that isn't available here and I'm not advocating jumping straight to OOP or 100% pure programming without good motivation to do so.

The important thing is to get map into newRoom's scope.


More design thoughts

On a related note, why isn't map preallocated with empty objects for rooms? Checking undefined strikes me as a bit of a code smell--if the rest of the codebase has functions that repeatedly check for undefined before taking action on map, there's likely a better way to design this, given more context about the application.

It's a bit odd that the function has to act as a coalescing operation to both initialize and re-initialize a room. It seems overburdened. It may be better to have a newRoom function that handles setting the map[x] = {}; and an initializeMonstersInRoom function that sets map[x][y] = {monsters: {}}. This is still pretty unusual but at least there's no condition and the caller can use newRoom and initializeMonstersInRoom at separate points in the application rather than basically using a branch in newRoom to figure out what to do. If you're calling this function in a loop that initializes rooms for many x and y values, there may be a cleaner way to arrange that that solves the coalescing problem.


Prefer arrays for numeric, sequential indexes

It's a little surprising to me that map is a nested object with stringified integer keys and hash lookups. The only reason I'd do this is if the x/y coordinates are sparse or numbered non-sequentially.

I also prefer using row-major order: map[y][x] rather than map[x][y]. The standard approach is to iterate rows, then columns. This is typically how memory is laid out and compilers often rearrange column-major matrix accesses to row-major to improve locality. I'm not sure if JS JIT compilers do this or not, but it's only icing on the cake of following standards. There are some notable exceptions like FORTRAN and R that use column-major, but these languages also use 1-indexing so neither has much credibility as a reference for writing modern code in a language like JS.


A rewrite suggestion taking the original at face value

As for the original logic, it isn't particularly terse or elegant and seems suggestive of larger design flaws, but on the other hand, there's nothing especially wrong with the function block logic in and of itself. Since the function is so small, there's no real readability problem here. Your attention might be oversensitive to a minor readability micro-optimization that would solve itself naturally with the correct design (such as prepopulating map indices).

Having said that, I'd write it as

'use strict';

const newRoom = (map, x, y) => {
  map[x] = map[x] || {};
  map[x][y] = {monsters: {}};
};

const map = {};
newRoom(map, 0, 0);
console.log(map);

Or, with arrays, assuming your rooms use sequential integers:

'use strict';

const newRoom = (map, x, y) => {
  map[x] = map[x] || [];
  map[x][y] = {monsters: {}};
};

const map = [];
newRoom(map, 0, 0);
console.log(map);

\$\endgroup\$
2
\$\begingroup\$

Intent

Reading only the code

The code gives hints at some higher level use, but what that could be is unclear. I will use intent to fill the blanks

Good code provides more than just an understanding of the logic, but conveys intent.

The strongest guide to intent is naming, I find this more true for less experienced coders.

Deducing intent

From the naming map, x, y, monsters, newRoom

  • map hints at spacial layout, though it is also common (and bad practice) to use it to represent key / value store. In this case I think you are building a geographic like map.

  • x, y This further enforces that your intent is a geographic coordinate on the map

  • 'monsters' This is a term one would seldom use outside a game. Which is the clincher that make me sure that up are creating some type of level map. Adding a room at coordinates x, y that contains monsters. As monsters is plural it suggests many monsters

  • newRoom this name does not match the logic of the code. The code is conditional creating a column on the map, then creates a room, directly added to the map, then adds monsters I suspect that your intent is more closely matched to the name newRoom That you are forced to add the column due to the fact that the map is incomplete.

On the assumption I just made

Use a 2D Array

With that I would suggest that map be a 2D array. To work effectively as an array it would be best that all items in an array be created before adding items at random indexes. This is to avoid the array being marked as sparse array (sparse arrays are slow)

I will also suggest that you swap the coordinates, convention addresses coordinates, x then y, so it is best to maintain that when ever you access a coordinate. This will save you lots of head ache.

To populate the array would require some information on the max size of the map.

The next example is one method of creating a 2D array that is filled with undefined to ensure that indexing into the array is as fast as possible.

The 2D array is an array of columns.

const WIDTH = 10;
const HEIGHT = 10;
const map = new Array(WIDTH).fill().map(()=>new Array(HEIGHT).fill());

Create a room

A room is independent of a map location. I suggest that you create a room then add it to the map

The monsters as a plural would also be better as an array, unless you give each monster a unique name. But I think you are not familiar with arrays and that you would name monsters 1, 2, 3

Thus creating a room would be

const createRoom = (x, y) => ({monsters: [], x, y});

Note that I added the coordinates of the room to the room. This is so code that deals with the room can easily know where the room is.

Adding a room to the map

As the room is now independently created the newRoomToMap function can get a better name. addRoomToMap that as the name implies adds a room to the map.

Because the room holds its coordinates you need only pass the room to the function.

I will also assume that your code knows the correct coordinates for each room and that there is no need to check if a room already exists

// with map already defined as 2D array
const addRoomToMap = (room, map) => { map[room.x][room.y] = room }

Putting it all together.

Thus with all the assumptions you code is somewhat transformed to

"use strict";
const WIDTH = 10, HEIGHT = 10;
const map = new Array(WIDTH).fill().map(()=>new Array(HEIGHT).fill());

const createRoom = (x, y) => ({monsters: [], x, y});
const addRoomToMap = (room, map) => { map[room.x][room.y] = room }

addRoomToMap(createRoom(5, 0), map); 

Assumptions.

I have based this answer on assumptions derived completely on your code alone. This is how I always approach a review, copy code to an IDE write the review, then read the question, and refine the review. The reason is that in the real world code is all we see, a description seldom comes with code, and when it does, it is not read at the same time or in the same document as the code.

Reading the full question I see that your intent is truly embedded in the naming, but not well defined in the logic.

Arrays

If you are unfamiliar with arrays MDN Array will get you started

\$\endgroup\$

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.