Too many if statements?

[DontRevealMe]

Hi, This block of code is sampling 1 cell in a 2d array and checking whether or not it borders the x1, y1, x2, and y2 coordinates. It’s in a for loop and if you need more context, I’m working off of this and I was trying to remake it so it can suit my needs. Looking at the block code, it feels like there are way too many if statements and feels like it could be improved on.

Improvements I already know of:

• Use “elseif x==x1 or y==y1 …” to stop repetition of table.insert()

if x~=x1 and y~=y1 and x~=x2 and y~=y2 then
    cell:DestroyWall("N")
    cell:DestroyWall("S")
    cell:DestroyWall("E")
    cell:DestroyWall("W")
elseif x==x1 and y==y1 then
    cell:DestroyWall("N")
    cell:DestroyWall("E")
    table.insert(self.BorderingCells, cell)
elseif x==x2 and y==y1 then 
    cell:DestroyWall("N")
    cell:DestroyWall("W")
    table.insert(self.BorderingCells, cell)
elseif x==x1 and y==y2 then
    cell:DestroyWall("S")
    cell:DestroyWall("E")
    table.insert(self.BorderingCells, cell)
elseif x==x2 and y==y2 then 
    cell:DestroyWall("S")
    cell:DestroyWall("W")
    table.insert(self.BorderingCells, cell)
elseif x==x1 or x==x2 then 
    cell:DestroyWall("S")
    cell:DestroyWall("N")
    table.insert(self.BorderingCells, cell)
elseif y==y1 or y==y2 then
    cell:DestroyWall("E")
    cell:DestroyWall("W")
    table.insert(self.BorderingCells, cell)
end

[DoNotEatSoggyWafflez]

I wish Lua used Switch statements, but since they don’t I feel like this is the only way

[TheEdgyDev]

Hey there! I always avoid against premature optimizations, but luckily it seems this question is more about the readability of your code. Much different than my opinion on optimizations, I believe code readability and maintainability is, by far, the most important–even more than functionality. Seeing posts like these make me rather happy when others are looking to improve their code in those aspects particularly.

Here’s what I could concoct:

local Direction = {
    North = "N",
    East = "E",
    South = "S",
    West = "W",
};

--- Returns the cell at the given column and row, if it exists.
--- @template T
--- @param {T[][]} matrix
--- @param {number} column
--- @param {number} row
--- @returns {T|nil}
local function getCell(matrix, column, row)
    assert(math.floor(column) == column);
    assert(math.floor(row) == row);

    return (
        (
            matrix
            and matrix[column]
            and matrix[column][row]
        )
        or nil
    );
end

--- Returns the cells that border the given cell.
--- @template T
--- @param {T[][]} matrix
--- @param {number} column
--- @param {number} row
--- @returns {Object.<Direction, T|nil>}
local function getBorderingCells(matrix, column, row)
    assert(math.floor(column) == column);
    assert(math.floor(row) == row);

    local borderingCells = {
        [Direction.East] = getCell(matrix, column + 1, row),
        [Direction.North] = getCell(matrix, column, row + 1),
        [Direction.South] = getCell(matrix, column, row - 1),
        [Direction.West] = getCell(matrix, column - 1, row),
    };

    return borderingCells;
end

--- Gets the cell at the current `column` and `row`, destroying any walls with
--- a bordering wall. Returns the bordering cells for developer ergonomics.
--- @param {Cell[][]} matrix
--- @param {number} column
--- @param {number} row
--- @returns {Object.<Direction, Cell>}
local function destroyBorderingWalls(matrix, column, row)
    assert(math.floor(column) == column);
    assert(math.floor(row) == row);

    local cell = getCell(matrix, column, row);
    assert(cell)

    local borderingCells = getBorderingCells(matrix, column, row);

    for direction in pairs(borderingCells) do
        cell:DestroyWall(direction);
    end

    return borderingCells;
end

I tested it in the playground, just to be sure, and it worked as I believed it should. And, as always, this code is not a perscriptive solution, but a reference to help you better inform your own design and style. If you have any questions feel free to ask!

(I assume you’ve made most of these functions or their equivalents, but I’ve included my own renditions of these just in case)

[DontRevealMe]

Ah I think you might’ve read it wrong or I’m reading your post wrong, but you seem to be trying to destroy cells that are directly bordering the cell instead of cells that border (x1, y1) and (x2, y2). The cells are held within a 2d array and the 2 coordinates are indicating 2 points on that 2d array. If you were aiming to show me another perspective of how I could make these functions, then thank you! This will help me out when I have to refactor it!
@DoNotEatSoggyWafflez probably had it right where I can’t find a way to make it look prettier. Maybe I could shorten it by adding a :DestroyAllWalls() function and allowing :DestroyWall() to take in more than 1 direction.

[TheEdgyDev]

I certainly misread it then! In that case, get the bordering cells of x1, y1 and the bordering cells of x2, y2; reduce it to be a listing of only shared sides, and then programmatically remove those colliding walls.

You caught me right after I closed up my office, but if that doesn’t make much sense, let me know and I can make a standalone example in the next day or two.

[DontRevealMe]

Take a look at this:

@iiau

This is what I aimed for, minus the door creation part. I think what I have going right now is probably gonna gonna be the best I’m gonna get. I’m going through the cell array between x1, y1 and x2, y2 and checking whether or not their X and Y value line up with the given coordinates.