Cleaner structure for Battleships code

Question

The code provided is working fine, but I would like to see if there is room for improvement for the code provided:

    function Ship (size, direction) {
    this.coveredFields = [];
   this.place = function (sizeY, sizeX) { // sizeX & sizeY: size of fields in both dimensions

       // pick randomly within our limits
       var locationX;
       var locationY;
       if (direction) {
        locationX = Math.floor(Math.random() * (sizeX - 1 - size));
        locationY = Math.floor(Math.random() * (sizeY - 1));
       } else {
        locationX = Math.floor(Math.random() * (sizeX - 1));
        locationY = Math.floor(Math.random() * (sizeY - 1 - size));
       }

       // setting locations
       for (var i = 0 ; i < size ; i++) {
        if (direction) {
            this.coveredFields.push(locationY * 10 + locationX + i)
        } else {
            this.coveredFields.push((locationY + i) * 10 + locationX)
        }
       }
   }
  }

var ship1 = new Ship(3, true);
ship1.place(6,6);
ship1.coveredFields;

var ship2 = new Ship(3, false);
ship2.place(6,6);
ship2.coveredFields;

var ship3 = new Ship(3, true);
ship3.place(6,6);
ship3.coveredFields;


var guess;
var hits = 0;
var guesses = 0;
var isSunk = false;
var shipLength = 7;
var countSunkShip = 0;

var shipsArray = [];
shipsArray.push(ship1.coveredFields, ship2.coveredFields, ship3.coveredFields);
console.log(shipsArray);


 /*function collision(direction)
 {
for (var i = 0; i < shipsArray.length; i++) 
{
    for (var j = 0; j < direction.length; j++) 
    {
        if (shipsArray[i].direction.indexOf(direction[j]) != -1)
        {
            return true;
        }
    }
}
return false;
}*/

var createTable = function(cellsInARow, Rows){
var table = document.createElement('table');
for (var i = 0; i <= Rows; i++){
    var tr = document.createElement('tr');   

    for(var j=0; j<=cellsInARow; j++){
        var td = document.createElement('td');

        td.style.width = "50px";
        td.style.height = "50px";
        td.id = "td"+i+j;

        var text1 = document.createTextNode('');

        td.appendChild(text1);
        tr.appendChild(td);
    }
    table.appendChild(tr);
}
document.body.appendChild(table);

}
//call function to create grid(7,7)
createTable(6,6);


 //game's logic

// add event listeners to all the td's
var tags = document.getElementsByTagName("td");
console.log(tags);
for(var i=0; i<tags.length; i++){
//console.log(tags[i].id.replace('td',''));

matchTrue = 0;
tags[i].addEventListener("click", function clicked(d){
        guesses++;
        // get the id of the cliked item in a var
        var locationClicked = parseInt(this.id.replace('td',''));
        // var which indicates if the match has been made
        var matchTrue = 0;
        // looping through all locations, from 1-9
        for(var i=0; i<shipsArray.length && matchTrue==0; i++){
            for(var j=0; j<shipsArray.length && matchTrue==0; j++){
                //check if locations correspond to cliked items' ids
                if(locationClicked == shipsArray[i][j]){
                    matchTrue = 1;
                    hits++;
                    if (hits == 9) {
                        isSunk = true;
                        var div = document.createElement("div");
                        div.style.color = "#3dcabb;";
                        div.id = "congrats-message";
                        var stats = "You took " + guesses + " guesses to find all the objects, " +
                        "which means your total accuracy was " + (3/guesses);
                        div.innerHTML = "Congratulations, the game is over! You have overcome the challenge!"+ "<br/>"+ stats;
                        document.body.appendChild(div);

                        // assigning the final values to the final vars
                        document.getElementById("hitArea").innerHTML = "Hit accuracy: "+(3/guesses);
                        document.getElementById("guessArea").innerHTML = "Guesses: "+(guesses);
                    }
                    if(hits%3==0){
                        countSunkShip++;
                        document.getElementById("shipArea").innerHTML = "Ships sunk: "+ countSunkShip;
                    }
                }
                else
                {
                    matchTrue = 0;
                }// END if which checks locations == clicked items
            }
        }
    //check if the match was made
    if(matchTrue == 1){
            //insert ship image here
            var img = document.createElement('img');
            img.src = "ship3.png";
            img.style.backgroundPosition = "center center";
            this.appendChild(img);
        }
        else{
            //insert miss image here
            var img = document.createElement('img');
            img.src = "arrow.png";
            this.appendChild(img);
        }

        var node = d.target.parentNode.querySelector("td");
        console.log(node);
        d.target.removeEventListener("click", clicked, false);


    });

}

//resetBoard button
function reloadPage(){
window.location.reload();
}

var resetButton = document.getElementById('resetBtn');
console.log(resetButton);
resetButton.onclick = reloadPage;

Answer 1

I would like to touch on a few points, some of which are minor and others that are big picture. I will start with the minor issues first.

I am not sure if you simply made a mistake when copying the code onto this site, but you have some indentation issues here:

    function Ship (size, direction) {
    this.coveredFields = [];
   this.place = function (sizeY, sizeX) { // sizeX & sizeY: size of fields in both dimensions

And here:

   for (var i = 0 ; i < size ; i++) {
    if (direction) {
        this.coveredFields.push(locationY * 10 + locationX + i)
    } else {
        this.coveredFields.push((locationY + i) * 10 + locationX)
    }
   }
}

As well as some other places. You also have inconsistent brackets after else statements here:

            else
            {
                matchTrue = 0;
            }// END if which checks locations == clicked items

And here:

    else{
        //insert miss image here
        var img = document.createElement('img');
        img.src = "arrow.png";
        this.appendChild(img);
    }

Finally there is not enough white space in some of your if statements such as in these two examples:

if(hits%3==0){
if(matchTrue == 1){

It is important to follow the language conventions for whatever language you are using, and it is also important to have proper indentation and white space. It might be easy for you to read and understand the code in its current form and at this moment in time, but it is hard for anyone else to read it with these kinds of problems, and you will be left wondering what is going on when you come back to the code at a later time.

I would recommend that you put the { bracket on the same line as the if statement, and have a tab on the next line. Whether you decide to follow this guideline, or some other guideline, it is important to be consistent throughout your code. I would also recommend more white space, like this if (hits % 3 == 0) {. It is simply more readable this way.

Now on to a big design issue. I see you declaring variables like this:

var ship1 = new Ship(3, true);
ship1.place(6,6);
ship1.coveredFields;

var ship2 = new Ship(3, false);
ship2.place(6,6);
ship2.coveredFields;

var ship3 = new Ship(3, true);
ship3.place(6,6);
ship3.coveredFields;

Any time that you have a variable name followed by a number, it is easy to tell that there are problems with the code. It would be much better to have the game designed in such a way that it can take a number of ships as input rather than hard coding them. This is a sign that you should change the organization of the code (possibly dramatically).

In addition to this, you have a large number of magic numbers in the code, such as these:

locationX = Math.floor(Math.random() * (sizeX - 1));
locationY = Math.floor(Math.random() * (sizeY - 1 - size));

this.coveredFields.push(locationY * 10 + locationX + i)
this.coveredFields.push((locationY + i) * 10 + locationX)

It might be obvious to you, or maybe even to someone else, what the 1 and the 10 are supposed to represent, but I for one am not totally sure. It is a good idea to eliminate all magic numbers in your code, and replace them with things like this:

td.style.width = "50px";
td.style.height = "50px";
var shipLength = 7;

This way it is immediately clear to you or to anyone else exactly what the numbers are supposed to be.

//call function to create grid(7,7)
createTable(6,6);

The above snippet is a good example of why your comments should explain the Why of your code, rather than the What. In this case, it looks like you changed the size of the grid from 7 to 6, so the comment is not accurate. If the comment is accurate, then it should instead say why a 7 is going to make a grid of size 6.

There are lots more issues related to the overall flow of the code. I had a hard time following the logic of the game with the way that you have it organized. I would recommend structuring it differently, though I do not have a specific suggestion in this regard. The bottom line is that the different pieces of code are located in places that makes it hard to immediately tell what is going on, and when.

Answer 2

I'm just going to talk about your first function. I'll leave the rest to the experts.

       // pick randomly within our limits
       var locationX;
       var locationY;
       if (direction) {
        locationX = Math.floor(Math.random() * (sizeX - 1 - size));
        locationY = Math.floor(Math.random() * (sizeY - 1));
       } else {
        locationX = Math.floor(Math.random() * (sizeX - 1));
        locationY = Math.floor(Math.random() * (sizeY - 1 - }
       }

I understand that there are only two choices for direction, so it must seem like a Boolean is the right choice. What does if (direction) mean though? Doesn't that read oddly? If you stick with a boolean value, I would go with vertical or horizontal instead. I think If (horizontal) reads much more naturally. (Please note that I didn't take the time to figure out if it should be horizontal or vertical.)

I would also extract the logic that returns the random coordinate into a function to remove the duplication. You should end up with something that looks like this:

       if (horizontal) {
        locationX = getRandomCoord(sizeX - size);
        locationY = getRandomCoord(sizeY));
       } else {
        locationX = getRandomCoord(sizeX);
        locationY = getRandomCoord(sizeY - size);
       }