Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interesting Submission 馃挱 #2

Open
chug2k opened this issue Mar 7, 2019 · 0 comments
Open

Interesting Submission 馃挱 #2

chug2k opened this issue Mar 7, 2019 · 0 comments

Comments

@chug2k
Copy link

chug2k commented Mar 7, 2019

Thanks for submitting your homework 馃懠 .

High Level Comments

Your documentation should be up to date! Please try to note what's been going on, and where the netlify link is at.

Your code style is really difficult for me to read. There are a few things that (as your teacher) will tell you not to do here in the next section. There are some style things, but really the structure of your code needs to be much clearer and simplified. I can see you are very bright. This is smart but messy code.

I know that sounds very judgmental, so let me try to explain. Let's look at this function, for example:

function supportMess(valueClk) {
    if (Math.abs(valueClk - ranNum) <= 10) {
        messBox.innerHTML = '<img class="conv" src="img/close.gif" alt="">';
        return;
    }
    if (valueClk < ranNum)
        messBox.innerHTML = '<img class="conv" src="img/low.gif" alt="">';
    else if (valueClk > ranNum)
        messBox.innerHTML = '<img class="conv" src="img/high.gif" alt="">';
}

This function is in control of close, low, and high. However the function that takes care of equality is called above, and called status. So if I try to follow the flow of the program, I have to jump around several places.

I think a clearer way to write this message would be to focus on just the supportMess part; namely working with the message box. That means it should also take care of the you win or you lose message as well, if you ask me.

function status(result = true) {
    if (result) {
        process.classList.add('d-none');
        document.querySelector('#start-screen').innerHTML = "<h1>WIN!!!!!!</h1><p>Press F5 to Restart</p>"
    } else {
        process.classList.add('d-none');
        document.querySelector('#start-screen').innerHTML = "<h1>Game Over!</h1><p>Press F5 to Restart</p>"
    }
}

This function is mostly confusing because result is a really bad variable name. In the previous method, you were doing straight comparisons on valueClk and ranNum. You should choose one way - either compute it outside the function, or inside the function, and do it the same way throughout your program.

Specific Code Suggestions

  • Do not use _i type variable names. JavaScript style discourages _ in variable names, but further this is called hungarian notation or something where you put the type in the variable name. It's an old practice and now is generally agreed upon to make code less readable.
  • if (aClk.classList[0] == 'x') { - this is an example of a "magic" value that is hard for me to read. Your class name of wrong is quite clear what to expect, but with x I have no idea. Also the use of [0] here is a bit fragile; the order of classes should not matter. Your code will break if the order of classes changes.
  • if you really want to use querySelectorAll, no need to do [0]. You can just call querySelector, and that will return the first one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant