Dodaj nazwę klasy do elementu DOM

Question

Mam funkcję, która dodaje nazwę klasy do elementu DOM jest spełniony. Dostarczony skrypt działa.

function rm_row(){ 

  var chk_inherent =  arcapi.dataResult().columnNameToIndex('Inherent Risk');
  var chk_residual = arcapi.dataResult().columnNameToIndex('Residual Risk')
  var chk_perf = arcapi.dataResult().columnNameToIndex(arcapi.getSetting('Column name'));

  d3.selectAll("#" + arcapi.chartId() + " .dataTables_scrollBody tbody tr").classed('removerow', function() {

    const perf = this.children[chk_perf].innerHTML
    const inherent = this.children[chk_inherent].innerHTML
    const residue = this.children[chk_residual].innerHTML
    const cf_regex = perf.replace(/[`~%]/gi, '');

    //  **** Check *****
    const inh_isEmpty = inherent === '' || inherent === 'Null'
    const rsk_isEmpty = residue === '' || residue === 'Null'
    const perf_isInvalid = cf_regex < 0 || cf_regex > 100

    /** Logic [if inherent is empty AND  Residual risk is empty AND is perf value is incorrect = True] **/
    return inh_isEmpty && rsk_isEmpty && perf_isInvalid === true
  });
}

Jednak czuję, że jest to rozwlekłe. Czuję, że można to jeszcze bardziej zoptymalizować. Jakie jest najlepsze podejście / najlepsze praktyki do dalszego doskonalenia się i optymalizacji mojego skryptu, tak aby był łatwy w utrzymaniu?

Myślałem o utworzeniu tablicy i przechowywaniu wszystkich moich zmiennych w tablicy, ale uważam, że takie podejście będzie kosztowne.

Answer 1

Jednak czuję, że jest to rozwlekłe.

Aby zmniejszyć ilość powtarzanego kodu i ułatwić dostosowanie się do różnych kolumn bez ponownego pisania tego samego rodzaju 2 lub 3 razy, poszukaj podobieństw w różnych kolumnach. Dla każdej kolumny, którą wywołujesz arcapi.dataResult().columnNameToIndex, możesz więc utworzyć funkcję, która, mając nazwę kolumny, będzie ją wywoływać. Uzyskujesz również dostęp this.children[index].innerHTMLdo każdego pobranego indeksu.

Jedną z opcji jest użycie obiektów zamiast wielu niezależnych zmiennych. Możesz indeksować obiekt według nazw zmiennych, których wartości są wskazaniami kolumn. Następnie w selectAllwywołaniu zwrotnym możesz skonstruować obiekt z tymi samymi właściwościami, ale z wartościami tekstowymi, które Cię interesują, używając mapowania wpisów obiektu i przekazywania do Object.fromEntries:

function rm_row() {
  const getIndex = columnName => arcapi.dataResult().columnNameToIndex(columnName);
  const columnIndiciesByName = {
    inherent: getIndex('Inherent Risk'),
    residual: getIndex('Residual Risk'),
    perf: getIndex(arcapi.getSetting('Column name')),
  };

  d3.selectAll("#" + arcapi.chartId() + " .dataTables_scrollBody tbody tr").classed('removerow', function() {
    const columnTextsByName = Object.fromEntries(
      Object.entries(columnIndicies).map(
        ([key, columnIndex]) => ([key, this.children[columnIndex].innerHTML])
      )
    );
    columnTextsByName.perf = columnTextsByName.perf.replace(/[`~%]/gi, '');
    // Then refer to columnTextsByName.inherent, etc

Powyższe jest tylko opcją do rozważenia. Chociaż sprawia to, że kod jest mniej powtarzalny i bardziej elastyczny dla przyszłych dodawanych lub usuwanych kolumn, które możesz chcieć sprawdzić, nie jestem do końca przekonany, że jest to solidna poprawa w stosunku do twojego obecnego kodu, jeśli nie spodziewasz się zmiany bieżącego kodu. YMMV; możesz uznać, że Twój obecny kod jest bardziej czytelny lub nie.

Inne możliwe ulepszenia:

Nie używaj varw ES6: Jeśli masz zamiar pisać w składni ES6 - co powinieneś, jest świetny - nie ma powodu, aby używać var, ma zbyt wiele pułapek (takich jak podnoszenie zakresu funkcji i automatyczne przypisywanie do globalnej obiekt na najwyższym poziomie), z którego warto skorzystać. Zawsze używaj const(lub, gdy musisz ponownie przypisać zmienną, let).

Konwencje nazewnictwa JS Zdecydowana większość profesjonalnych aplikacji JavaScript wykorzystuje camelCasefunkcje i zwykłe zmienne. snake_casejest dość rzadkie (a mieszanie przypadku węża z camelCase w tej samej nazwie zmiennej jest jeszcze dziwniejsze). Rozważ użycie camelCasewszędzie tam, gdzie jest to właściwe.

cf_regexPodobnie nazwij swoje zmienne odpowiednio: w Twoim oryginalnym kodzie cf_regexnie jest wyrażeniem regularnym, jest to po prostu ciąg znaków, w którym zastąpiono pewne znaki. Lepiej nazwać to czymś dokładniejszym i dłuższym (nie mam pojęcia, co to ma być, podobnie jak inni czytelnicy kodu podając tylko ten kontekst)

Coś dziwnego w wyrażeniu regularnym - używasz flagi inieuwzględniającej wielkości liter , ale nie dopasowujesz żadnych liter, więc flaga nic nie robi. Równie dobrze mógłbym go usunąć. Lub, aby wyjaśnić, co się dzieje, jeśli spodziewasz się, że wynik będzie zawierał tylko liczby, zamiast tego dopasuj cyfry:

columnTextsByName.perf = Number(columnTextsByName.perf.match(/\d+/)[0]);

(ponieważ będziesz później porównywać z liczbami, lepiej jest rzutować zmienną, która jest porównywana z liczbą; ma to większy sens i ułatwi debugowanie, jeśli później pojawią się problemy)

innerHTMLczy textContent? O ile celowo nie pobierasz znaczników HTML, które nie wyglądają tutaj na obecne, jeśli zależy ci tylko na tekście komórki, bardziej odpowiednie byłoby użycie .textContentniż .innerHTML. To też jest szybsze.

Średniki używasz kilku średników, ale również brakuje sporo. Jeśli nie jesteś ekspertem i nie używasz wszędzie średników, możesz w końcu potknąć się o automatyczne wstawianie średników . Rozważ użycie lintera .

Wydajność, którą mówisz

Myślałem o utworzeniu tablicy i przechowywaniu wszystkich moich zmiennych w tablicy, ale uważam, że takie podejście będzie kosztowne.

Na nowoczesnych komputerach obciążenie związane z tworzeniem tablicy lub obiektu zamiast wielu niezależnych zmiennych jest całkowicie nieobecne. Jeśli użycie określonej struktury danych sprawia, że ​​kod jest łatwiejszy w utrzymaniu (biorąc pod uwagę preferowany styl), zrób to. Jeśli później stwierdzisz, że występuje problem z wydajnością, możesz wrócić i debugować, aby znaleźć dokładnie wąskie gardło, abyś mógł wymyślić bardziej wydajną metodę - ale użycie obiektu lub tablicy prawie na pewno nie będzie wąskim gardłem. Staraj się unikać przedwczesnej optymalizacji lub przynajmniej nie poświęcaj dla niej przejrzystości kodu, chyba że musisz.

Answer 2

Refaktoryzuj powtarzalny kod na funkcje wielokrotnego użytku.

Pierwszą rzeczą, która przyszła mi do głowy, była ta para powtarzających się linii:

const inh_isEmpty = inherent === '' || inherent === 'Null'
const rsk_isEmpty = residue === '' || residue === 'Null'

Zamiast tego zamieńmy to w funkcję:

function isEmpty(string) {
  return string === '' || string === 'Null'
}

Teraz możemy się pozbyć inh_isEmptyi rsk_isEmptyzmiennych w całości, a tylko pisać isEmpty(inherent)i isEmpty(residue)zamiast. To może nie być ogromna oszczędność pod względem liczby linii, ale z pewnością wygląda czysto. Możesz także użyć tej isEmptyfunkcji w innym miejscu w kodzie.

Ale to jeszcze nie koniec. Te wiersze również wyglądają na bardzo powtarzalne:

var chk_inherent =  arcapi.dataResult().columnNameToIndex('Inherent Risk');
var chk_residual = arcapi.dataResult().columnNameToIndex('Residual Risk')
var chk_perf = arcapi.dataResult().columnNameToIndex(arcapi.getSetting('Column name'));

^{(Przy okazji, dlaczego używasz vartutaj, ale const/ letgdzie indziej? Mieszanie tych dwóch stylów deklaracji zmiennych nie ma większego sensu. Bądź konsekwentny! W tym przypadku użycie średnika jest również trochę przypadkowe.)}

… Podobnie jak te wiersze poniżej:

const perf = this.children[chk_perf].innerHTML
const inherent = this.children[chk_inherent].innerHTML
const residue = this.children[chk_residual].innerHTML

Jedną z opcji byłoby refaktoryzacja powtarzających się części tych linii na funkcję taką jak ta:

function getColumnHTML(row, columnName) {
  const index = arcapi.dataResult().columnNameToIndex(columnName)
  return row.children[index].innerHTML
}

… I używaj go w ten sposób np .:

const perf = getColumnHTML(this, arcapi.getSetting('Column name'))
const inherent = getColumnHTML(this, 'Inherent Risk')
const residual = getColumnHTML(this, 'Residual Risk')

^{(Swoją drogą , zmieniłem nazwę twojej residuezmiennej residualna pasującą do nazwy kolumny.)}

Skoro już o tym mowa, zdefiniujmy również funkcję do sprawdzenia poprawności:

function isValidPercentage(string) {
  const percent = Number(string.replace(/[`~%]/g, ''))
  return percent >= 0 && percent <= 100
}

^{( Porównanie ciągów z liczbami w JavaScript może być trudne, więc wolałbym jawnie Number()przepuścić przefiltrowany ciąg . W szczególności zapewnia to, że wszelkie nierozdzielne dane wejściowe zostaną zamienione na NaN, co następnie zakończy się niepowodzeniem. Ponadto /iprzełącznik jest bezużyteczne dla tego wyrażenia regularnego, więc je usunąłem).}

Po tych wszystkich zmianach Twój refaktoryzowany kod wyglądałby mniej więcej tak:

function getColumnHTML(row, columnName) {
  const index = arcapi.dataResult().columnNameToIndex(columnName)
  return row.children[index].innerHTML
}

function isEmpty(string) {
  return string === '' || string === 'Null'
}

function isValidPercentage(string) {
  const percent = Number(string.replace(/[`~%]/gi, ''))
  return percent >= 0 && percent <= 100
}

function removeBadRows() { 
  d3.selectAll("#" + arcapi.chartId() + " .dataTables_scrollBody tbody tr").classed('removerow', function() {
    const perf = getColumnHTML(this, arcapi.getSetting('Column name'))
    const inherent = getColumnHTML(this, 'Inherent Risk')
    const residual = getColumnHTML(this, 'Residual Risk')

    return isEmpty(inherent) && isEmpty(residual) && !isValidPercentage(perf)
  })
}

Oczywiście możesz również przenieść funkcje pomocnicze w inne miejsce - np. Wewnątrz removeBadRowsfunkcji, jeśli nie chcesz, aby były widoczne poza nią, lub alternatywnie do jakiejś kolekcji funkcji użytkowych wielokrotnego użytku.

Ps. Kilka innych zmian, które również zasugerowałbym:

• Jeśli nie potrzebujesz surowego kodu HTML (i wygląda na to, że nie), użyj textContent(lub jego efektywnego synonimu innerText) zamiast innerHTMLuzyskać dostęp do tekstu wewnątrz elementu DOM. Jest to zarówno łatwiejsze (nie trzeba martwić się o analizę kodu HTML ani dekodowanie jednostek), jak i bezpieczniejsze (mniej okazji do przypadkowych błędów wstrzyknięcia HTML).

• Użyj ściślejszego dopasowania wyrażenia regularnego, aby zweryfikować wartości procentowe. Nie mogę zasugerować konkretnego wyrażenia regularnego, ponieważ nie wiem dokładnie, jak wyglądają twoje dane, ale jako przykład, jeśli kiedykolwiek miałeś tylko liczbę bez znaku (całkowitą lub dziesiętną), po której następuje %znak, możesz zrobić coś takiego :

  function isValidPercentage(string) {
    const match = /^([0-9]+)(\.[0-9]+)?%$/.exec(string)
    if (!match) return false
    const percent = Number(match[1] + match[2])
    return percent >= 0 && percent <= 100
  }
  

• Budowanie selektorów DOM poprzez konkatenację ciągów jest brzydkie i podatne na błędy. Unikaj tego, jeśli możesz, zminimalizuj, jeśli nie możesz. Rozważ np. Zrobienie czegoś takiego:

  const chart = d3.select('#' + arcapi.chartId())
  chart.selectAll('.dataTables_scrollBody tbody tr').classed('removerow', // ...
  

  lub nawet:

  const chart = d3.select(document.getElementById(arcapi.chartId()))
  // ...
  

  (i rozważ zapisanie chartzmiennej jako stałej globalnej lub zawiń kod do obliczania go w jeszcze jedną funkcję pomocniczą, aby uniknąć jej powtórzenia).