5
\$\begingroup\$

I wrote a server-side Battleship game simulator in nodeJs/Javascript:

  1. A random player who begins is chosen.
  2. Both players place their ships. Software recognizes if they are placed correctly.
  3. One of the players shoots at a position on the 10×10 field. If it's not the current players turn, the program recognizes it; if it's the player's turn the program checks if the player hit a ship of the opponent. If he/she hit a ship, he/she can shoot again. If a ship is destroyed, the software recognizes that...

Its source code is available at: https://github.com/ndsvw/battleship

I also wrote 36 test cases (npm install, npm run test) and it works.

I would like to improve the code and want to make sure that it does always work (all imaginable scenarios).

An example: the file src/field.js contains checkShipArray(arr) that takes an array like [67, 77, 31, 41, 51, 61, 71, 0, 1, 2, 16, 17, 18, 96, 97, 98, 99] and returns an object with a status 'success'/'fail' and an array of ships:

{
    status: 'success',
    ships:
    [
        [ 31, 41, 51, 61, 71 ],
        [ 67, 77 ],
        [ 0, 1, 2 ],
        [ 16, 17, 18 ],
        [ 96, 97, 98, 99 ]
    ]
 }

But the code that makes this is ~200 lines long. My main problem: The 2 functions getCollisionPosOfHorizontalShip and getCollisionPosOfVerticalShip do almost look the same but I can't figure out how to write it shorter.

This is the whole field.js file:

const PositionSet = require('./positionset');
const RandomFieldGenerator = require('./random-field-generator');

module.exports = class Feld {
    constructor(options) {
        options = options || {};
        this.SAMEPLAYERSTURNAFTERHIT = typeof options.SAMEPLAYERSTURNAFTERHIT === 'undefined' ? true : options.SAMEPLAYERSTURNAFTERHIT;
        this.REQUIREDSHIPS = options.REQUIREDSHIPS || [0, 1, 2, 1, 1]; // default: 0x 1er, 1x 2er, 2x 3er, 1x 4er, 1x 5er
        this.FIELD_HEIGHT = options.FIELD_HEIGHT || 10;
        this.FIELD_WIDTH = options.FIELD_WIDTH || 10;
        this.COLLISION_RULES = options.COLLISION_RULES || {
            ALLOW_CORNER_COLLISIONS: true // in the default field: [0,1,2,3,4,15,16] for example
        };
        this.SHIPCOUNTER = 0;
        this.SHIPPOSCOUNTER = 0;
        for (let i = 0; i < this.REQUIREDSHIPS.length; i++) {
            if (this.REQUIREDSHIPS[i] > 0) {
                this.SHIPCOUNTER += this.REQUIREDSHIPS[i];
            }
            this.SHIPPOSCOUNTER += this.REQUIREDSHIPS[i] * (i + 1);
        }
        this.ships = [];
        this.hits = [];
        this.misses = [];

        if (this.REQUIREDSHIPS.length > this.FIELD_WIDTH && this.REQUIREDSHIPS.length > this.FIELD_WIDTH) {
            throw new Error('At least 1 ship seems to be larger than the field.');
        }
        if (this.SHIPPOSCOUNTER > this.FIELD_WIDTH * this.FIELD_HEIGHT) {
            throw new Error('The field is not large enough for all ships.');
        }
    }

    isShipAt(pos) {
        return this.ships.some((s) => s.includes(pos));
    }

    hasAlreadyBeenHit(pos) {
        return this.hits.includes(pos);
    }

    hasAlreadyBeenMissed(pos) {
        return this.misses.includes(pos);
    }

    isShipDestroyedAt(pos, opponentFeld) {
        const ship = this.ships.find((s) => s.includes(pos)) || null;
        return (ship !== null && ship.every((p) => opponentFeld.hasAlreadyBeenHit(p)));
    }

    setShips(arr) {
        const data = this.checkShipArray(arr);
        if (data.status === 'success') {
            this.ships = data.ships;
            return {
                status: 'success'
            };
        }
        return {
            status: data.status,
            reason: data.reason
        };
    }

    setRandomShips() {
        // only works for the default field so far
        const rfg = new RandomFieldGenerator();
        return this.setShips(rfg.generateField());
    }

    checkShipArray(arr) {
        // eliminate duplicates
        arr = Array.from(new Set(arr));

        // sort ascending
        arr.sort((a, b) => a - b);

        // check whether all ships are placed
        if (arr.length !== this.SHIPPOSCOUNTER) {
            return {
                status: 'fail',
                reason: 'A problem occured. The following ships need to be placed: ' + this.getRequiredShipsListAsText()
            };
        }

        // Check whether all ships are placed within the field
        if (arr.some((s) => s < 0 || s > this.FIELD_HEIGHT * this.FIELD_WIDTH - 1)) {
            return {
                status: 'fail',
                reason: 'A problem occured. Ships need to be placed within the field.'
            };
        }

        // getting an array with all ships
        const data = this.getShipsOfArray(arr);
        const ships = data.shipArray;
        const shipsH = data.shipArrayH;
        const shipsV = data.shipArrayV;

        // check whether the number of ships and their sized are correct
        if (ships.length === this.SHIPCOUNTER) {
            // deep copy the requirements; for each ship of length x: decrement the value of the index x.
            // after that: check if all values of the array are 0.
            const reqCheckArr = JSON.parse(JSON.stringify(this.REQUIREDSHIPS));
            for (const s of ships) {
                reqCheckArr[s.length - 1]--;
            }
            if (reqCheckArr.some((x) => x !== 0)) {
                return {
                    status: 'fail',
                    reason: 'A problem occured. The following ships need to be placed: ' + this.getRequiredShipsListAsText()
                };
            }
        } else {
            return {
                status: 'fail',
                reason: 'A problem occured. The following ships need to be placed: ' + this.getRequiredShipsListAsText()
            };
        }

        // Check whether all parts of the horizontal ships are in the same row (don't accept [8,9,10,11,12] in the default match)
        for (const s of shipsH) {
            const row = Math.floor(s[0] / this.FIELD_WIDTH);
            for (let i = 1; i < s.length; i++) {
                if (Math.floor(s[i] / this.FIELD_WIDTH) !== row) {
                    return {
                        status: 'fail',
                        reason: 'A problem occured. The following ships need to be placed: ' + this.getRequiredShipsListAsText()
                    };
                }
            }
        }


        // iterate over all ships and check whether they are at forbidden positions
        const forbiddenPositions = this.getCollisionPos(shipsH, shipsV);
        for (const s of ships) {
            if (s.some((pos) => forbiddenPositions.hasPos(pos))) {
                return {
                    status: 'fail',
                    reason: 'A problem occured. Ships must not collide!'
                };
            }
        }

        return {
            status: 'success',
            ships
        };
    }

    getShipsOfArray(arr) {
        const shipArray = [];
        const shipArrayH = [];
        const shipArrayV = [];
        const arrH = []; // Array, that contains all the position of the horizontal ships.

        // find vertical ships.
        for (const s of arr) {
            // if the position is already part of a ship, continue
            if (shipArray.some((sh) => sh.includes(s))) {
                continue;
            }

            let i = 0;
            while (arr.includes(s + (i + 1) * this.FIELD_WIDTH)) {
                i++;
            }
            if (i === 0) {
                arrH.push(s);
            } else {
                const newShip = [];
                for (let j = s; j < s + (i + 1) * this.FIELD_WIDTH; j += this.FIELD_WIDTH) {
                    newShip.push(j);
                }
                shipArray.push(newShip);
                shipArrayV.push(newShip);
            }
        }

        // find horizontal ships.
        for (const s of arrH) {
            // if the position is already part of a ship, continue
            if (shipArray.some((sh) => sh.includes(s))) {
                continue;
            }

            let i = 0;
            const currentRow = Math.floor(s / this.FIELD_WIDTH);
            // as long as the current position is in arr && if we are still in the same row => increment i
            while (arr.includes(s + i + 1) && Math.floor((s + i + 1) / this.FIELD_WIDTH) === currentRow) {
                i++;
            }
            if (i !== 0) {
                const newShip = [];
                for (let j = s; j < s + i + 1; j++) {
                    newShip.push(j);
                }
                shipArray.push(newShip);
                shipArrayH.push(newShip);
            }
        }

        return {
            shipArray,
            shipArrayH,
            shipArrayV
        };
    }

    getCollisionPos(shipsH, shipsV) {
        const collisionPos = new PositionSet(this.FIELD_HEIGHT, this.FIELD_WIDTH);
        for (const s of shipsH) {
            collisionPos.union(this.getCollisionPosOfHorizontalShip(s));
        }
        for (const s of shipsV) {
            collisionPos.union(this.getCollisionPosOfVerticalShip(s));
        }
        return collisionPos;
    }

    getCollisionPosOfHorizontalShip(s) {
        const collisionPos = new PositionSet(this.FIELD_HEIGHT, this.FIELD_WIDTH);

        // position in front of the ship and behind the ship are forbidden.
        if (s[0] % this.FIELD_WIDTH > 0) {
            collisionPos.add(s[0] - 1);
        }
        if ((s[s.length - 1] + 1) % this.FIELD_WIDTH > 0) {
            collisionPos.add(s[s.length - 1] + 1);
        }

        // rows next to the ship and in parallel to the ship are forbidden
        for (let i = 0; i < s.length; i++) {
            collisionPos.add(s[i] - this.FIELD_WIDTH);
            collisionPos.add(s[i] + this.FIELD_WIDTH);
        }

        // positions at the corners are (maybe) forbidden
        if (!this.COLLISION_RULES.ALLOW_CORNER_COLLISIONS) {
            if (s[0] % this.FIELD_WIDTH > 0) {
                collisionPos.add(s[0] - (this.FIELD_WIDTH + 1));
                collisionPos.add(s[0] + (this.FIELD_WIDTH - 1));
            }
            if ((s[0] + 1) % this.FIELD_WIDTH > 0) {
                collisionPos.add(s[s.length - 1] - (this.FIELD_WIDTH - 1));
                collisionPos.add(s[s.length - 1] + (this.FIELD_WIDTH + 1));
            }
        }
        return collisionPos;
    }

    getCollisionPosOfVerticalShip(s) {
        const collisionPos = new PositionSet(this.FIELD_HEIGHT, this.FIELD_WIDTH);

        // position in front of the ship and behind the ship are forbidden.
        collisionPos.add(s[0] - this.FIELD_WIDTH);
        collisionPos.add(s[s.length - 1] + this.FIELD_WIDTH);

        // rows next to the ship and in parallel to the ship are forbidden
        for (let i = 0; i < s.length; i++) {
            if (s[i] % this.FIELD_WIDTH > 0) {
                collisionPos.add(s[i] - 1);
            }
            if ((s[i] + 1) % this.FIELD_WIDTH > 0) {
                collisionPos.add(s[i] + 1);
            }
        }

        // positions at the corners are (maybe) forbidden
        if (!this.COLLISION_RULES.ALLOW_CORNER_COLLISIONS) {
            if (s[0] % this.FIELD_WIDTH > 0) {
                collisionPos.add(s[0] - (this.FIELD_WIDTH + 1));
                collisionPos.add(s[s.length - 1] + (this.FIELD_WIDTH - 1));
            }
            if ((s[0] + 1) % this.FIELD_WIDTH > 0) {
                collisionPos.add(s[0] - (this.FIELD_WIDTH - 1));
                collisionPos.add(s[s.length - 1] + (this.FIELD_WIDTH + 1));
            }
        }
        return collisionPos;
    }

    getRequiredShipsListAsText() {
        const reqShips = [];
        for (let i = 0; i < this.REQUIREDSHIPS.length; i++) {
            if (this.REQUIREDSHIPS[i] > 0) {
                reqShips.push(this.REQUIREDSHIPS[i] + 'x ' + (i + 1) + 'er');
            }
        }
        return reqShips.join(', ');
    }
};
\$\endgroup\$
  • \$\begingroup\$ The code that is included in my question works absolutely fine. And all the tests of the project are successful. But it was -->ME<-- who wrote the test cases. So I'm not 100% sure whether there is MAYBE a scenario that I didn't think about that does not work as it should. (placing 2 2x1 ships on a 2x2 field with special options or something crazy....) :) \$\endgroup\$ – Alpha Mar 5 at 22:26
  • \$\begingroup\$ all in all: It works absolutely fine but one can almost never say that there are 100% no bugs. \$\endgroup\$ – Alpha Mar 5 at 22:30
  • \$\begingroup\$ OK Looks good. Have you tried pasting whole file? Does it cut off? Would be easier for reviewers if you can do it. :) \$\endgroup\$ – 422_unprocessable_entity Mar 5 at 22:37
  • \$\begingroup\$ ok. Was not sure about that because the file has requirements and I though, seeing it on GitHub would be easier. \$\endgroup\$ – Alpha Mar 5 at 22:47
  • \$\begingroup\$ my bad. I thought you wanted the whole file reviewed. Sorry about it \$\endgroup\$ – 422_unprocessable_entity Mar 5 at 23:17
3
\$\begingroup\$

General feedback

This code looks okay, though it could utilize more features of , like for...of loops and default parameters (see below for more detail). Nothing jumps out as far as a way to consolidate the code in those two methods to check the collision positions but I wonder if you could consider flipping values in one of those instances in order to find similarities, though maybe that would make it even more complex.

Targeted feedback

Default parameters could be used to avoid lines like this in the constructors:

options = options || {};

Instead of using Array.from() in checkShipArray():

arr = Array.from(new Set(arr));

the spread syntax can be used to avoid the function call:

arr = [...new Set(arr)];

The call to sort the array in checkShipArray() could be moved after the validation checks to avoid excess processing.


Many of the for loops could be transformed into for...of loops, to simplify access of array elements. I wish that syntax supported access to the index but it doesn't seem that is the case, and thus the loop in the class Feld (typo?) would need to manually increment such a variable if that was converted.

The functional method Array.filter() could be used to rewrite the for loop in getRequiredShipsListAsText()- it does allow access to the iterator counter (i.e. i).

\$\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.