2
\$\begingroup\$

I am working on a login/register system and want the register and log in process as smooth as possible. One way of making it smoother is in my opinion to make a multi-step login/register.

What I mean: If a user want's to log in, he first have to type in his username and if this username exists, he can type in a password and log in. The user has more feedback, especially in the register process. Since I started to learn OOP and know that es6 was introduces a while ago, I want to improve in JS aswell.

I only have one button that does everything. That's why I have a switch case in my button event listener that compares the value of it to select the right method in my form class.

The code at the bottom is what I have came up with. It works but I don't think it is how it's done normally.

Is this code 'advanced' or does it look like it's from a noob.

I am aware that doing a login like that leaks information, but I have already setup a robust protection against it. So that attackers don't have a lot of tries, while users can login wihtout a DoS.

class Form {
    content = document.getElementById('content');
    username = document.getElementById('username');
    password = document.getElementById('password');
    submit_btn = document.getElementById('right');
    loader = document.getElementById('loader');
    request;

    constructor () {
        this.request = new XMLHttpRequest();
    }

    execute(type, path, Data) {
        this.loading();
        this.request.open(type, path);
        this.request.setRequestHeader('Content-type', 'application/x-www-form-urlencoded');
        this.request.send(Data);
    }

    loading() {
        this.loader.style.opacity = 1;
    }

    loaded() {
        this.loader.style.opacity = 0;
    }

    submit_username () {
        this.request.onload = () => {
            this.loaded();

            //If response value is true than
            //Switch Input field to password
            //Change button value to login_two
            //else show error messages

        }

        let Data = `username=${this.username.value}`

        this.execute('post', '/login/step_one', Data);
    }

    submit_login() {
        this.request.onload = () => {
            this.loaded();

            //If response value is true than
            //Redirect the user to home
            //else show error messages

        }

        let Data = `password=${this.password.value}`

        this.execute('post', '/login/step_two', Data);

    }

}

const form = new Form();

form.submit_btn.addEventListener("click", () => {
    let method = form.submit_btn.value;
    switch (method) {
        case 'login_one':
            form.submit_username();
        break;
        case 'login_two':
            form.submit_login();
        break;
    }
});

Maybe someone can provide an example of a better way.

Codepen

\$\endgroup\$
1
\$\begingroup\$
  • Declaring a class makes sense if it can be reused, but less so if the logic and element ids are hardcoded to just one case, it only adds the unnecessary this. to variables.
  • It's unclear why the click listener is handled outside of the form class as it's logically and conceptually a part of the form.
  • JS naming convention is camelCase, not snake_case.
  • Consider URLSearchParams API so the HTTP headers are set automatically and values are properly escaped.
  • Consider making the login routes more declarative e.g. routes in the code below.
  • If your goal was to use a class as a namespace then it might be better to switch to modules. For example using the built-in ES modules the inner contents of the function will be the entire contents of the module.
function handleLogin() {
  const request = new XMLHttpRequest();
  const loader = document.getElementById('loader');
  const btn = document.getElementById('right');
  let step = 0;
  const routes = [{
    path: 'one',
    field: 'username',
    onload() {
      step++;
      getDataElement().focus();
    },
    onerror(e) { /* show error */ },
  }, {
    path: 'two',
    field: 'password',
    onload() { /* show home page */ },
    onerror(e) { /* show error */ },
  }];

  btn.addEventListener('click', () => {
    loader.style.opacity = 1;
    const {path, field} = routes[step];
    request.open('post', `/login/step_${path}`);
    request.send(new URLSearchParams({[field]: getDataElement().value}));
  });

  request.addEventListener('loadend', e => {
    loader.style.opacity = 0;
    routes[step][request.response === 'true' ? 'onload' : 'onerror'](e);
  });

  function getDataElement() {
    return document.getElementById(routes[step].field);
  }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer! No, my goal was not to use the class as a namcespace. I just wanted my code to be more modern and scaleable. Since I dont need module I dont need it as function. However I am trying to expand it, because the registering process works the same way as the login. \$\endgroup\$ – Mointy Nov 4 '19 at 13:01
  • \$\begingroup\$ Well without seeing the full workflow I don't see what can be scaled here as it's too simple. Like I already said, with the info present so far I see no benefits in using a class here, quite the opposite. \$\endgroup\$ – wOxxOm Nov 4 '19 at 13:04
  • \$\begingroup\$ I added a codepen link where you will see what I mean. The code you posted is fine, I dont need to have everything in a function (handleLogin), since it should execute when the file loads. But I also want the same functionality for registering a user without the need of copying everything. \$\endgroup\$ – Mointy Nov 4 '19 at 13:57
  • \$\begingroup\$ FWIW you can call a function just fine or you can make it an IIFE (() => {...})() As for copying, there's not much to copy so I'm against adding superficial complexity of classes and inheritance. \$\endgroup\$ – wOxxOm Nov 4 '19 at 14:37

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.