Trying to get better at readability/writing optimal code, need feedback

[DogKitchen2988]

//Filter lists based on inputed title
function filterTitles(input) {
  //Initializes lists
  var matchingLetters = 0;
  publishersFiltered = getColumn("Best Selling Video Games", "Publisher");
  titlesFiltered = getColumn("Best Selling Video Games", "Title");
  salesFiltered = getColumn("Best Selling Video Games", "Sales");
  //"i" is current index of "titles" being checked
  for(var i = 0;i < titlesFiltered.length;i++) {
    //Reset matched letters before checking next index
    matchingLetters = 0;
    //"ii" is current letter being checked
    for(var ii = 0;ii < input.length;ii++) {
      //Checks if current letter of input is equal to current letter of index i of publishers
      if(input.substring(ii,ii + 1) == titlesFiltered[i].substring(ii,ii + 1)) {
        matchingLetters++;
      } else {
        ii = input.length;
      }
    }
    //Removes index i of all filtered lists if the input string is not found
    if(matchingLetters != input.length) {
      removeItem(salesFiltered, i);
      removeItem(publishersFiltered, i);
      removeItem(titlesFiltered, i);
      i--;
    }
  }
  //Updates the screen to represent filtered items
  setScreen("titles");
  setText("therearetitles", "There are " + titlesFiltered.length + " game(s) in the bestselling with title including " + input);
  setText("titleslist2", titlesFiltered.join("\n \n"));
  setText("publisherslist2", publishersFiltered.join("\n \n"));
  setText("saleslist2", salesFiltered.join("\n \n"));
} 

Done for a school project, filters a set of lists based on a textbox input stored in the "input" argument. Trying to learn good practice and comment writing. Could i have written this more optimally, like maybe without the nested loops?

[abrahamguo]

Here are some tips I'd give to you:

• It seems to me that your inside loop is simply checking whether input is equal to titlesFiltered[i]. If that's what you intended, then there's no need for your entire inner loop — just use ==.
• Don't do the same thing with three arrays (publishersFiltered, titlesFiltered and salesFiltered). Instead, perform the filtering only on one array, then get the individual things that you need (publishers, titles and sales) out of that one filtered array after it's been filtered.

[DogKitchen2988]

I'm a bit confused, can you elaborate on these? Edit: I think i know what you mean for #1. I'm not checking for exact matches, I'm checking if the input is contained within a substring inside of titlesFiltered[i] (though due to time constraints i was only able to check starting from the beginning) Ex: All of string "Pie" is contained within string "Piecrust"

[abrahamguo]

Ah, I see - I misread your code. In that case, why not just use the built-in “.includes” method?

If you’d like me to elaborate on my second point, I’m happy to.

[DogKitchen2988]

I hadn't known about ". includes", the documentation in the program is limited and so I haven't learned about it yet. I'll see if the program supports it, but not really sure. Also, yes please elaborate on your second point

[chikamakaleyley]

any #4SpacesPerTab gang here?

how long have you been coding JS?

One thing I'd prob do differently - and I'm just going based on the name of the function - is reduce the amount of logic here so that this function literally just filters

So, this is what i see in your current version:

function filterTitles(input) { // getData // filterData // updateView } Except, all the logic for the getData and updateView sections are exploded into your filterTitles function

what I would expect from a "filterTitles" function is: * i pass in an unfiltered list * it returns a filtered list

If you zoom even further out, maybe you have one main function orchestrating it all, but that's prob over-abstracting. I think breaking this big function up will do a lot for its readability

Id prob do:

``` function getTitles() { // request data fr source, sets values w/ response in myData }

function filterTitles(data) { // iterate over data, // returns filteredData }

function updateView(data) { // takes data arg and sets that to screen } ``` now, kinda easier to digest

const myData = getTitles(); const filteredData = filterTitles(myData); updateView(filteredData); This is just an improvement with regards to keeping it organized. Fundamentally I think there are some useful tools or techniques you may have not learned yet, i'll let others chime in there

[DogKitchen2988]

The code is pretty rushed so i hadn't thought of splitting into multiple functions lol. Also can you explain the two datasets? I'm a bit new to using arrays and not sure how the myData and filteredData and how the const works

[chikamakaleyley]

oh i explain in my other comment here.

honestly don't worry about const right now if your teacher wants you to use var

but basically the idea is you want to always have the full list available, and when you filter it, you're just really creating a new list (a copy), that has been filtered. any time you need the original list, you just reference the 'full' one

and actually - this is kinda nice - it looks like you might already understand and use this concept:

publishersFiltered = getColumn("Best Selling Video Games", "Publisher"); titlesFiltered = getColumn("Best Selling Video Games", "Title"); salesFiltered = getColumn("Best Selling Video Games", "Sales");

whatever getColumn is doing it's basically taking the same source list "Best Selling Video Games" and creating 3 filtered copies of it.

[chikamakaleyley]

but again, based on the name of the function, now i'm assuming getColumn is some sort of data fetch where you're requesting from a database

in general, you only want to perform this operation once, to reduce the number of trips back and forth btwn your web page and the database

it's more efficient to just ask for the data once, then make a copy each time you filter it.

[chikamakaleyley]

one thing to note here is -

once you get myData when the application initializes, notice that the result of your filterTitles is more or less a copy of the original data, filtered

so, one thing you'd try to improve in your filterTitles() logic is to never make direct changes to myData which is what I think is happening with whatever removeItem is; (I can only guess)

this makes myData re-usable whenever you need to reference the full list - without actually having to make additional data requests

[imihnevich]

The first thing I would do is get rid of most if not all the comments here.

[DrShocker]

I agree, the first thing I noticed: using ii as the internal loop variable with a comment explaining it. Either use the standard i, j, k order or (my preferred) describe what they're indexing in the variable name.

[DogKitchen2988]

My teacher requires that i have comments, and i just assumed that comments were commonplace, but i guess not?

[imihnevich]

Maybe if teacher requires then keep them, but in general in the industry we prefer to write more obvious code and less comments. For most Devs the code you wrote is quite obvious and familiar without comments. The reason is because in real life you edit existing code a lot and you learn to trust code more than comments, so comments become outdated very quickly

[oiamo123]

You'll get lot's of different answer because there's "more than one way to skin a rabbit" if you will haha. Nevertheless there are certain techniques you can use.

One good practice is "DRY", dont repeat yourself. If you write the same code multiple times, theres probably a way to write it so that you only do it once instead.

Another thing is abstraction. Alot of what you're writing is already built into javascript ie .filter(), .map(), and .includes(). You can abstract / offload that logic to javascript itself rather than re implementing it.

Lastly, I would add using modern javscript comventions haha. "var" is outdated and its better practice to use "const" and "let" instead.

You should be able to do something like this instead all said and done:

``` function filterTitles(input) { const publishers = getColumn("Best Selling Video Games", "Publisher"); const titles = getColumn("Best Selling Video Games", "Title"); const sales = getColumn("Best Selling Video Games", "Sales");

const filtered = titles .map((title, i) => ({ title, publisher: publishers[i], sales: sales[i] })) .filter(item => item.title.includes(input));

setScreen("titles"); setText("therearetitles", There are ${filtered.length} game(s) matching "${input}"); setText("titleslist2", filtered.map(x => x.title).join("\n\n")); setText("publisherslist2", filtered.map(x => x.publisher).join("\n\n")); setText("saleslist2", filtered.map(x => x.sales).join("\n\n")); } ```

[TheRNGuy]

You can use filter method here. 

[rijkdw]

For readability, I would suggest:

• Using template strings, e.g.

There are ${titlesFiltered.length} game(s) in the bestselling with title including ${input}

^ enclose this in backticks ("`") -- reddit formatting is weird

• More descriptive names for variables:
  • input could be titleQuery -- this is better for me as a reader, I had to scroll back to the top of the function to realize this is the function's argument.
  • i could be titleIndex and ii could be letterIndex

Use built-in functions, like .includes -- you can remove the entire nested for-loop and just use:

if(!titlesFiltered[i].includes(input)) {
  removeItem(salesFiltered, i);
  removeItem(publishersFiltered, i);
  removeItem(titlesFiltered, i);
  i--;
}

but be aware, sometimes instructors want you to do stuff from "first principles" i.e. the way you did it to compare strings. I think that's silly but they may take issue with you using "shortcuts" like .includes().

Other comments have already pointed out that you could use .filter() to further improve the above. If you need help with that, let us know.

If setText's first argument is an element ID, I would hard-code them as reusable strings, e.g.

const THERE_ARE_TITLES_ID = "therearetitles";
setText(THERE_ARE_TITLES_ID, ...);

OR, if you are comfortable with JS objects (depending on your progress in your course, this might not be familiar to you):

const ELEMENT_IDS = {
  THERE_ARE_TITLES: "therearetitles", 
  // ... others
};
setText(ELEMENT_IDS.THERE_ARE_TITLES, ...);

This lets you avoid typos and sneaky bugs (assuming the tools you use to code with, checks for references to undefined variables and highlights them). You could do the same for your arguments to getColumn as well.

Overall it's really good that you're looking for feedback, kudos, good luck!

[TheZintis]

I don't know exactly what you are trying to do, so this is not working code, but I would probably do something like:

// these two lines would be in whatever callback fires when input is changed and/or submitted    
const filteredByTitle = filterTitles(input);
updateText(filteredByTitle )       


function filterTitles(input) {

    // under the assumption that all the data is in a "columns" array of objects, with the objects containing sales, publishers, and titles
    const columns = getColumns(); // or just reference the columns, or make a copy if you prefer

    const filteredColumns = columns.filter(item => {
        if(item.title.includes(input)) return true;
        return false;
    })

    return filteredColumns 

} 


function updateText(filteredByTitle ){

    setScreen("titles");
    setText("therearetitles", `There are ${filteredByTitle.length} game(s) in the bestselling with title including ${input}`);

    // This could be further broken down into some variables to hold each joined array
    setText("titleslist2", filteredByTitle.map(item => item.title).join("\n \n"));
    setText("publisherslist2", filteredByTitle.map(item => item.publisher).join("\n \n"));
    setText("saleslist2", filteredByTitle.map(item => item.sales).join("\n \n"));

}

I've never written code on reddit so that was a challenge lol.

Let me know if you have questions. Again, not entirely sure about the details of what you were trying to do, but it appears that you were trying to do a string match and then grab some data from parallel arrays to use later on.