Welcome to the Treehouse Community

Want to collaborate on code errors? Have bugs you need feedback on? Looking for an extra set of eyes on your latest project? Get support with fellow developers, designers, and programmers of all backgrounds and skill levels here with the Treehouse Community! While you're at it, check out some resources Treehouse students have shared here.

Looking to learn something new?

Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and join thousands of Treehouse students and alumni in the community today.

Start your free trial

JavaScript

Neil Bircumshaw
seal-mask
.a{fill-rule:evenodd;}techdegree
Neil Bircumshaw
Full Stack JavaScript Techdegree Student 14,597 Points
Posted by Neil Bircumshaw
.a{fill-rule:evenodd;}techdegree
Neil Bircumshaw
Full Stack JavaScript Techdegree Student 14,597 Points

tic tac toe - need a more elegant solution

So, in the game of tic-tac-toe the player wins if they get 3 of either their respective objects in a row. So I need to see if they have 3 of either their Xs or Os in a row. I've done this by looking if the list item (there is 9 in total representing the 9 boxed tic tac toe grid) has got either the class "box box-filled-1" (this is a O) or "box box-filled-2" (this is an X). If they have a O on the grid then the number of the boxes position in the list will be push into the OCombos array. If it is an X it will be pushed into the XCombos array. I will then later use the includes. method to see whether the array has a particular set of numbers in it that will be a winning row (like 0,1,2 (straight across first row), 0,4,8 (diagonally from the top left corner to the bottom right corner)).

The code I have to be honest looks messy. I'm sure I was suppose to use constructor functions and prototype in this challenge to make the code look neater. I understand how these work but I'm not sure how to implement them. Perhaps there is a more elegant way?

My JS looks like this:

const winningScreen = document.getElementById("finish");
const winningMessage = document.getElementsByClassName("message");
const board = document.getElementById("board");
const li = document.getElementsByClassName("box")
let XCombos = [];
let OCombos = [];

winningScreen.style.display = "none";


boxes.addEventListener("click", (e) => {



if (e.target.tagName == "LI"){
    if(li[8].className == "box box-filled-1"){
    OCombos.push(8)
    }

    else if(li[8].className == "box box-filled-2"){
    XCombos.push(8)
    }

    if(li[7].className == "box box-filled-1"){
    OCombos.push(7)
    }

    else if(li[7].className == "box box-filled-2"){
    XCombos.push(7)
    }

    if(li[6].className == "box box-filled-1"){
    OCombos.push(6)
    }

    else if(li[6].className == "box box-filled-2"){
    XCombos.push(6)
    }

    if(li[5].className == "box box-filled-1"){
    OCombos.push(5)
    }

    else if(li[5].className == "box box-filled-2"){
    XCombos.push(5)
    }

    if(li[4].className == "box box-filled-1"){
    OCombos.push(4)
    }

    else if(li[4].className == "box box-filled-2"){
    XCombos.push(4)
    }

    if(li[3].className == "box box-filled-1"){
    OCombos.push(3)
    }

    else if(li[3].className == "box box-filled-2"){
    XCombos.push(3)
    }

    if(li[2].className == "box box-filled-1"){
    OCombos.push(2)
    }

    else if(li[2].className == "box box-filled-2"){
    XCombos.push(2)
    }

    if(li[1].className == "box box-filled-1"){
    OCombos.push(1)
    }

    else if(li[1].className == "box box-filled-2"){
    XCombos.push(1)
    }

    if(li[0].className == "box box-filled-1"){
    OCombos.push(0)
    }

    else if(li[0].className == "box box-filled-2"){
    XCombos.push(0)
    }
}});

5 Answers

Steven Parker
Steven Parker
229,644 Points
Steven Parker
Steven Parker
229,644 Points

You can apply the DRY principle to compact the code.

You have a good solution, and now you can identify repeated areas and refactor them into functions to make the code more compact. It looks like this might be best done in two levels, one for the win patterns and one for testing each "3-in-a-row".

I might also use classList.contains() instead of direct comparisons to check for only the important class, and check for each player separately to issue different messages.

Doing all that might get you something like this:

function checkRow(a, b, c, cls) {
    return li[a].classList.contains(cls) &&
           li[b].classList.contains(cls) &&
           li[c].classList.contains(cls);
}

function checkWin(cls) {
    return checkRow(0, 1, 2, cls) || checkRow(0, 3, 6, cls) || checkRow(0, 4, 8, cls) ||
           checkRow(3, 4, 5, cls) || checkRow(6, 7, 8, cls) || checkRow(1, 4, 7, cls) ||
           checkRow(2, 5, 8, cls) || checkRow(2, 4, 6, cls);
}

boxes.addEventListener("click", e => {
        // do you need to add a class to the clicked item here before testing?
        if (checkWin('box-filled-1')) console.log("O wins!");
        if (checkWin('box-filled-2')) console.log("X wins!");
});
Martin Balon
Martin Balon
43,433 Points
Martin Balon
Martin Balon
43,433 Points

Hi Neil, I haven't tested it but it should work(I was a way too lazy to create html file :D ).

const winningScreen = document.getElementById("finish");
const winningMessage = document.getElementsByClassName("message");
const board = document.getElementById("board");
const li = document.getElementsByClassName("box");
let XCombos = [];
let OCombos = [];

winningScreen.style.display = "none";

boxes.addEventListener("click", (e) => {

  if (e.target.tagName == "LI"){

    for (let i = 0; i <= 8; i++) {

      if (li[i].className == 'box box-filled-1') {
        OCombos.push(i);
      } else if (li[i].className == 'box box-filled-2') {
        XCombos.push(i);
      }

    }

  }

});
Alexander Acker
Alexander Acker
18,123 Points
Alexander Acker
Alexander Acker
18,123 Points

Yep agree that this is the simplest approach without changing too much of your current built in functionality.

Steven Parker
Steven Parker
229,644 Points
Steven Parker
Steven Parker
229,644 Points

These arrays might get bigger than you expect.

It looks like you push every marked square into the arrays on every move, so there will be many duplicates of the early moves in the array by the end.

I'm not sure you need these "combos" arrays anyway. There's only 8 ways to win, so you could possibly just check for a winning pattern directly on the board after each move. Then you don't need any storage other than the board itself.

Neil Bircumshaw
seal-mask
.a{fill-rule:evenodd;}techdegree
Neil Bircumshaw
Full Stack JavaScript Techdegree Student 14,597 Points
Neil Bircumshaw
.a{fill-rule:evenodd;}techdegree
Neil Bircumshaw
Full Stack JavaScript Techdegree Student 14,597 Points

Thanks for your answers guys. Yeah I tried a loop before and it didn't seem to work, must have made a syntax error! Thanks for that solution though Martin :)

Yeah I noticed that also Steven, I guess I don't want to end up with a huge array with lots of redundant numbers in it. So would you say do it like this?

JS

boxes.addEventListener("click", (e) => {
if (li[0].className == 'box box-filled-1' && li[1].className == 'box box-filled-1' && li[2].className == 'box box-filled-1'
|| li[0].className == 'box box-filled-2' && li[1].className == 'box box-filled-2' && li[2].className == 'box box-filled-2'  ) {
console.log("It worked!")}

else if (li[0].className == 'box box-filled-1' && li[3].className == 'box box-filled-1' && li[6].className == 'box box-filled-1'
|| li[0].className == 'box box-filled-2' && li[3].className == 'box box-filled-2' && li[6].className == 'box box-filled-2'  ) {
console.log("It worked!")}

else if (li[0].className == 'box box-filled-1' && li[4].className == 'box box-filled-1' && li[8].className == 'box box-filled-1'
|| li[0].className == 'box box-filled-2' && li[4].className == 'box box-filled-2' && li[8].className == 'box box-filled-2'  ) {
console.log("It worked!")}

else if (li[3].className == 'box box-filled-1' && li[4].className == 'box box-filled-1' && li[5].className == 'box box-filled-1'
|| li[3].className == 'box box-filled-2' && li[4].className == 'box box-filled-2' && li[5].className == 'box box-filled-2'  ) {
console.log("It worked!")}

else if (li[6].className == 'box box-filled-1' && li[7].className == 'box box-filled-1' && li[8].className == 'box box-filled-1'
|| li[6].className == 'box box-filled-2' && li[7].className == 'box box-filled-2' && li[8].className == 'box box-filled-2'  ) {
console.log("It worked!")}

else if (li[1].className == 'box box-filled-1' && li[4].className == 'box box-filled-1' && li[7].className == 'box box-filled-1'
|| li[1].className == 'box box-filled-2' && li[4].className == 'box box-filled-2' && li[7].className == 'box box-filled-2'  ) {
console.log("It worked!")}

else if (li[2].className == 'box box-filled-1' && li[5].className == 'box box-filled-1' && li[8].className == 'box box-filled-1'
|| li[2].className == 'box box-filled-2' && li[5].className == 'box box-filled-2' && li[8].className == 'box box-filled-2'  ) {
console.log("It worked!")}

else if (li[2].className == 'box box-filled-1' && li[4].className == 'box box-filled-1' && li[6].className == 'box box-filled-1'
|| li[2].className == 'box box-filled-2' && li[4].className == 'box box-filled-2' && li[6].className == 'box box-filled-2'  ) {
console.log("It worked!")}
});

Would there be anyway to neaten this up at all though and make the code more compact?

Neil Bircumshaw
seal-mask
.a{fill-rule:evenodd;}techdegree
Neil Bircumshaw
Full Stack JavaScript Techdegree Student 14,597 Points
Neil Bircumshaw
.a{fill-rule:evenodd;}techdegree
Neil Bircumshaw
Full Stack JavaScript Techdegree Student 14,597 Points

Took me a while to understand that but after reading it a few times I understood it :) very well written code, hopefully that comes naturally to me one day. I think I tend to shy away from passing arguments too much is my problem, need to get more use to doing it to make my code more compact.

Thank you as always Steven.