Programma contatore semplice

Aug 29 2020

È il mio primo programma in Javascript. C'è qualche mancanza o qualcosa da migliorare? Semplicemente aumenta, diminuisce o azzera il contatore tramite pulsanti o tasti freccia.

HTML,

<h1 id="number"></h1>
<button class="btn" id="incr">Increase</button>
<button class="btn" id="reset">Reset</button>
<button class="btn" id="decr">Decrease</button>

JS,

let number = 0

const getNumber = document.getElementById(`number`)
const incrButton = document.getElementById(`incr`)
const decrButton = document.getElementById(`decr`)
const resetButton = document.getElementById(`reset`)

getNumber.textContent = number

const incrFunc = function () {
  ++number
  getNumber.textContent = number
  if (number > 0) {
    getNumber.style.color = `green`
  }
}

const resetFunc = function () {
  number = 0
  getNumber.textContent = number
  getNumber.style.color = `gray`
}

const decrFunc = function () {
  --number
  getNumber.textContent = number
  if (number < 0) {
    getNumber.style.color = `red`
  }
}

incrButton.addEventListener(`keyup`, function (e) {
  e.stopPropagation()
  //console.log(e.target === document.body)
  if (e.code === `ArrowUp`) {
    incrFunc()
  }
})

document.addEventListener(`keyup`, function (e) {
  //console.log(e.target === document.body)
  if (e.code === `ArrowUp`) {
    incrFunc()
  }
})

document.addEventListener(`keyup`, function (e) {
  if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
    resetFunc()
  }
})

resetButton.addEventListener(`keyup`, function (e) {
  if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
    resetFunc()
  }
})

document.addEventListener(`keyup`, function (e) {
  if (e.code === `ArrowDown`) {
    decrFunc()
  }
})

decr.addEventListener(`keyup`, function (e) {
  if (e.code === `ArrowDown`) {
    decrFunc()
  }
})

incrButton.addEventListener(`click`, incrFunc)

resetButton.addEventListener(`click`, resetFunc)

decrButton.addEventListener(`click`, decrFunc)

Risposte

3 GirkovArpa Aug 30 2020 at 03:27

Il mio primo consiglio sarebbe quello di riformattare il codice con un linter. Non entrerò in questo in modo più dettagliato perché presumo che tu stia principalmente cercando consigli per l'implementazione, ma poiché è piuttosto stridente da leggere, simile al testo scritto nella tua lingua ma con maiuscole e punteggiatura straniere.

Alcune delle tue funzioni potrebbero usare la destrutturazione per rendere i loro corpi più concisi.

Ad esempio, questo:

document.addEventListener(`keyup`, function (e) {
  if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
    resetFunc()
  }
})

Potrebbe essere riscritto in questo modo, poiché stai usando solo la codeproprietà dell'argomento:

document.addEventListener(`keyup`, function ({ code }) {
  if (code === `ArrowRight` || code === `ArrowLeft`) {
    resetFunc()
  }
})

Un consiglio più controverso potrebbe essere quello di scrivere questo nella parte superiore del tuo script:

const $ = document.querySelector.bind(document);

Questo ti permette di sostituire questo:

const getNumber = document.getElementById(`number`)
const incrButton = document.getElementById(`incr`)
const decrButton = document.getElementById(`decr`)
const resetButton = document.getElementById(`reset`)

Con qualcosa di molto meno dettagliato:

const getNumber = $(`#number`)
const incrButton = $(`#incr`) const decrButton = $(`#decr`)
const resetButton = $(`#reset`)
3 Kruga Aug 31 2020 at 18:49

Dovresti usare un punto e virgola alla fine delle righe.

Non è necessario avere un evento keyup sui pulsanti, il documento catturerà l'evento. Hai anche bisogno di un solo listener di eventi sul documento.

document.addEventListener(`keyup`, function (e) {
  if (e.code === `ArrowUp`) {
    incrFunc();
  }
  else if (e.code === `ArrowDown`) {
    decrFunc();
  }
  else if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
    resetFunc();
  }
})

Le tue funzioni per aumentare, diminuire e ripristinare sono abbastanza simili da poter essere combinate, quindi non devi ripetere lo stesso codice. In questo modo hai anche un modo per impostare il numero su qualsiasi valore o cambiarlo di qualsiasi importo se ne hai bisogno in seguito.

function setNumber(value) {
  number = value;
  getNumber.textContent = number
  if (number < 0) {
    getNumber.style.color = `red`
  }
  else if (number > 0) {
    getNumber.style.color = `green`
  }
  else {
    getNumber.style.color = `gray`
  }
}

function changeNumber(change) {
    setNumber(number + change);
}

È possibile impostare i listener di eventi con argomenti preimpostati con bind . Il primo argomento è impostare la thisparola chiave, che non usiamo, quindi possiamo semplicemente impostarla su null.

incr.addEventListener(`click`, changeNumber.bind(null, 1));
reset.addEventListener(`click`, setNumber.bind(null, 0));
decr.addEventListener(`click`, changeNumber.bind(null, -1));

Sebbene il salvataggio di un elemento in una variabile quando fa riferimento più volte sia una buona pratica, non è necessario quando viene utilizzato solo una volta.

Infine, getNumbernon è un buon nome descrittivo.

Il codice finale.

const numberDisplay = document.getElementById(`number`);
let number = 0;
setNumber(number);

function setNumber(value) {
    number = value;
    numberDisplay.textContent = number;
    if (number < 0) {
      numberDisplay.style.color = `red`;
    }
    else if (number > 0) {
      numberDisplay.style.color = `green`;
    }
    else {
      numberDisplay.style.color = `gray`;
    }
}

function changeNumber(change) {
    setNumber(number + change);
}

document.addEventListener(`keyup`, function (e) {
  if (e.code === `ArrowUp`) {
    changeNumber(1);
  }
  else if (e.code === `ArrowDown`) {
    changeNumber(-1);
  }
  else if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
    setNumber(0);
  }
})

document.getElementById(`incr`).addEventListener(`click`, changeNumber.bind(null, 1));
document.getElementById(`reset`).addEventListener(`click`, setNumber.bind(null, 0));
document.getElementById(`decr`).addEventListener(`click`, changeNumber.bind(null, -1));
<h1 id="number"></h1>
<button class="btn" id="incr">Increase</button>
<button class="btn" id="reset">Reset</button>
<button class="btn" id="decr">Decrease</button>

1 LucasWauke Sep 03 2020 at 09:19

Forse potresti mappare ogni codice chiave con una funzione ed evitare di controllare il codice chiave più volte

const map = {
  'ArrowUp': incrFunc,
  'ArrowRight': resetFunc,
  'ArrowLeft': resetFunc,
  'ArrowDown': decrFunc,
}

document.addEventListener(`keyup`, function (e) {
  if(map[e.code]) {
    map[e.code]();
  }
})

1 RoToRa Sep 04 2020 at 15:14

Non utilizzare stringhe di modelli ( `number`) se non stai effettivamente utilizzando modelli. Usa virgolette singole o doppie normali: 'number'o "number".