3
\$\begingroup\$

First: I'm not really a programmer. Thus, this application may be started in a bad way, but here is my question.

It's not a specific one because I have no real clue about how to improve my code. Let me know how I can recenter this question if needed.

Anyway, here is what I do and my issues:

First, you will see on the code linked below, that I use cache. I'm pretty sure there is no interest to use it on my case, and even, that I use it badly.

From a SpreadSheet, I take all the lines and values, to format a visual render of what the lines should do on import with another software. The final goal is to use this to generate a pre-formatted menu card (for restoration). I started from a pre-formatted list of products that the sales gives us, to detect the product, and which categories, prices etc.., then I format lines to respond the needed format of the software.

The part of my script that need to be reviewed, is when I display the result the card would have if imported on the software, then create some options to modify it graphically.

This part is working, but modal's generation is very long. Besides, each time I switch in a different category, I have to generate again the modal (the solution is probably more JavaScript in my HTML ? ).

My code is divided in some files:

  • One for JS functions to format cards given by sales (auto-generation)

  • One for JS functions to graphically modify the card (part I'm asking code review)

  • One for generals JS functions (such as include() that permit to links different gscript files together, or spreadsheet's menu generation to use the script)

  • One for HTML main part ( that display a table with 49 buttons, and is regenerated when changing a category).

  • some others HTML different parts (at the moment there is only the form to edit/create a new category/product graphically ^^, I have no other needs at the moment)

Here is part of the code for the graphical part. The functions that are up-to-date are: generateManual, openCategory, and ordinateProductTabs. To do so, I have a menu on which I can launch the generation of the modal:

function generateManual(){  
  var cache=CacheService.getDocumentCache();
  //Default value for this variable, which will change once another 
  categorie is called
  cache.put("inProductTypeTab",(-1));
  var ui = SpreadsheetApp.getUi();
  var output=HtmlService.createTemplateFromFile('ZatyooCard');
  var product=JSON.parse(cache.get("product"));
  output.product=product;
  SpreadsheetApp.getUi().showModalDialog(output.evaluate().setWidth(620).setHeight(550), 'Carte Zatyoo - Visuel');  
}

The product's cache variable (I'm pretty sure is a bad use of cache, was to answer one of my first idea, when I change the category I'm on when I display the render) is generated on Spreadsheet Opening, and when a modification is done, by calling a function named regenProductCache. Here is the regenProductCache function:

function regenProductCache(){  
  var cache=CacheService.getDocumentCache();
  //dataFromResult is just a sheet.getValues(), no interest to cache it. To be rewritten
  var dataFromResult = JSON.parse(cache.get("datasheet"));
  var product=new Array();
  var tabEncountered=new Array();
  for(var i=0;i<dataFromResult.length;i++){
  //Stupid to do this like this, but kept it because it works 
  /* it is the reason why inProductTypeTab is set up at -1 as default value, to take into account the particular first case*/
    if(dataFromResult[i][7]=="PREMIER"){
      if(product[0] == null){
        product[0] = new Array();
      }
      product[0].push(JSON.parse(JSON.stringify(dataFromResult[i])));
    }
    else{
      if(tabEncountered.indexOf(dataFromResult[i][7])==-1){
        tabEncountered.push(dataFromResult[i][7]);
        product[tabEncountered.length] = new Array();
        product[tabEncountered.length].push(dataFromResult[i])
      }
      else{
        product[tabEncountered.indexOf(dataFromResult[i][7])+1].push(dataFromResult[i]);
      }
    }
  }
  //ordinateProductTabs is called In order to create an array formated to build my table.
  //Indeed, each screens has 49 buttons, Each buttons can call a new screen
  //So once i listed all products, and on which category these products are, 
  //I create an array that has 49 values ( that are potentially other array of 49 values, etc..)
  //I store the category's array order in the cache variable "tabEncountered"
  //which contains tab's name, in order to do an indexOf() and determines 
  //what part of the Big array i should check for data while clicking on a given button
  product=ordinateProductTabs(product);
  if(cache.get("product")!=null||cache.get("tabEncountered")!=null){
    cache.remove("product");
    cache.remove("tabEncountered");
    cache.put("tabEncountered",JSON.stringify(tabEncountered));
    cache.put("product",JSON.stringify(product));
  }else{
    cache.remove("product");
    cache.remove("tabEncountered");
    cache.put("tabEncountered",JSON.stringify(tabEncountered));
    cache.put("product",JSON.stringify(product));
  }

Because it may be part of what takes a long time to be executed, here is the ordinateProductTabs, but I dont really think I can improve that part without rethinking everything.

function ordinateProductTabs(product){
  var productOrdinated = new Array();
  for(var i=0;i<product.length;i++){
    var count=product[i].length;
    productOrdinated.push(new Array(count));
    while(count<49){
      product[i].push(null);
      productOrdinated[i].push(null)
      count++;
    }
    for(var j=0;j<product[i].length;j++){
      if(product[i][j]!=null){
        var place = fullTable.indexOf(Number(product[i][j][8]));
        productOrdinated[i][place]=product[i][j];
      }
    }
  }
  return productOrdinated;
}

Positions of buttons are determined by an integer in 9th column of the sheet. I created an array, fullTable, as global variable, to represent how is treated the position of buttons in the real software, that is also how I determine colors of buttons and text, by other static global arrays.

Here again, poor way to use cache.

Finally here is the HTML file I use as main graphical file:

<!DOCTYPE html>
<html>
  <head>
    <?!= include('mainCSS'); ?>
    <?   var cache=CacheService.getDocumentCache();?>
    <base target="_top">
    <script src="https://code.jquery.com/jquery-1.9.1.js"></script>
    <script src="https://code.jquery.com/ui/1.10.3/jquery-ui.js"></script>
  </head>
  <body>
    <table>
    <!-- Si  on est sur la liste des types de produit-->
      <!-- Génère les boutons de productType (nb ligne = nb_bouton_par_ligne / nb_bouton_sur_une_ligne) -->
      <? for(var i=0;i<(product[Number(cache.get("inProductTypeTab"))+1].length/5);i++ ){?>
        <tr>
          <!-- Chaque boucle, 1Boutons, jusqu'à 5boutons pour la ligne -->
          <? for(var j=0;j<5;j++){ ?>
            <!-- Si le bouton à créer est un bouton "produit"-->
            <? if(product[Number(cache.get("inProductTypeTab"))+1][i*5 + j]!=null){?>
              <td>
              <? Logger.log(product[Number(cache.get("inProductTypeTab"))+1][i*5 + j][13])?>
                <button ondblclick="google.script.run.openCategory(<?=product[Number(cache.get("inProductTypeTab"))+1][i*5 + j][13]?>)"
                  <?/* En fonction des valeurs du produit, changer le css du bouton */?>
                  <? switch(product[Number(cache.get("inProductTypeTab"))+1][i*5 + j][10]){
                  case 'N':var textc="tblack"; break;
                  case 'W':var textc="twhite"; break;
                  case 'R':var textc="tred"; break;
                  case 'V':var textc="tgreen"; break;
                  case 'J':var textc="tyellow"; break;
                  case 'B':var textc="tblue"; break;
                  }?>
                  <? switch(product[Number(cache.get("inProductTypeTab"))+1][i*5 + j][9]){
                  case 'R':?>class="red <?=textc?>" <? break;
                  case 'V':?>class="lightgreen <?=textc?>" <? break;
                  case 'B':?>class="blue <?=textc?>" <? break;
                  case 'J':?>class="yellow <?=textc?>"<? break;
                  case 'N':?>class="black <?=textc?>"<? break;
                  case 'W':?>class="white <?=textc?>"<? break;
                  case 'M':?>class="brown <?=textc?>"<? break;
                  case 'P':?>class="pink <?=textc?>"<? break;
                  case 'T':?>class="skyblue <?=textc?>"<? break;
                  case 'K':?>class="beige <?=textc?>"<? break;
                  case 'H':?>class="darkgreen <?=textc?>"<? break;
                  }?>
                  ><?= product[Number(cache.get("inProductTypeTab"))+1][i*5 + j][3] ?>
                  </button>
                <div class="dropdown-content">
                  <input type="button" class="mButton cButton" value="Modifier Catégorie" onclick="google.script.run.editCategory(<?=product[Number(cache.get("inProductTypeTab"))+1][i*5 + j]?>)"/>
                  <input type="button" class="mButton cButton" value="Supprimer Catégorie" onclick="google.script.run.deleteCategory(<?=product[Number(cache.get("inProductTypeTab"))+1][i*5 + j]?>)"/>
                </div>

              </td>
            <!-- Sinon on crée un bouton vide -->
            <?}else{?>

              <td>
              <button onclick="test()">
              </button>
                <div class="dropdown-content">
                  <input type="button" class="mButton cButton" value="Créer Nouvelle Catégorie" onclick="google.script.run.createCategory()"/>
                </div>
              </td>

            <?}?>
          <?}?>
        </tr>
      <?}?>
    </table>
    <? if(Number(cache.get("inProductTypeTab"))==-1){?>
      <input type="button" class="mButton" value="Sauvegarder Carte"  onclick="google.script.run.saveCard()" />
      <input type="button" class="mButton" value="Fermer Carte" onclick="google.script.host.close()" />
    <?} else {?>
      <input type="button" class="mButton" value="Retour" onclick="google.script.run.openCategory()" />
    <?}?>
    <script>
      $('tr td button').click( function(){
        $(this).parent().find('.dropdown-content').slideToggle(100);
      });

      // Close the dropdown menu if the user clicks outside of it
      window.onclick = function(event) {
        if (!event.target.matches('.dropbtn')) {
          var dropdowns = document.getElementsByClassName("dropdown-content");
          var i;
          for (i = 0; i < dropdowns.length; i++) {
            var openDropdown = dropdowns[i];
            if (openDropdown.classList.contains('show')) {
              openDropdown.classList.remove('show');
            }
          }
        }
      }
    </script>
    <? Logger.log("bijour")?>
  </body>
</html>

Let me know if you need a screenshot of the final rendering, or whatever could be needed

I have a lot of functions that may be used on this modal, that may be bugged, but that's not the goal of the question.

In the modal, there will be button to click on, to access a category, which is displayed on the same html file ( because it's an imbricated menu that I try to create).

I think I have a lot of JavaScript code i should use to upgrade switch between categories, once the modal appears. But I also think my way to generate my modal at first is not correct, maybe I shouldn't even use a modal for this ?

Thanks for taking time to read and advise me on this. Be sure to ask anything that may be needed for a better understanding of my issue, or my code.

\$\endgroup\$
  • \$\begingroup\$ because it’s an imbricated menu that I try to create” I am not familiar with that word “imbricated” what should that be or what does it mean? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 20 '18 at 12:59
  • \$\begingroup\$ You should see my spreadsheet (link under your answer) for an example. it's a "recursive menu" (to adapt to the differents restaurant's menu complexity) is probably another way to say it. \$\endgroup\$ – LMS Sep 20 '18 at 15:01
1
\$\begingroup\$

Redundant Code at the end of regenProductCache()

The following code appears at the end of that function:

if(cache.get("product")!=null||cache.get("tabEncountered")!=null){
    cache.remove("product");
    cache.remove("tabEncountered");
    cache.put("tabEncountered",JSON.stringify(tabEncountered));
    cache.put("product",JSON.stringify(product));
}else{
    cache.remove("product");
    cache.remove("tabEncountered");
    cache.put("tabEncountered",JSON.stringify(tabEncountered));
    cache.put("product",JSON.stringify(product));
}

However in both cases, the code blocks appear to be identical, which makes the conditional lines (i.e. if and else) pointless. That code can be simplified to just four lines:

cache.remove("product");
cache.remove("tabEncountered");
cache.put("tabEncountered",JSON.stringify(tabEncountered));
cache.put("product",JSON.stringify(product));

function ordinateProductTabs()

var productOrdinated = new Array();
for(var i=0;i<product.length;i++){
  var count=product[i].length;
  productOrdinated.push(new Array(count));
  while(count<49){
    product[i].push(null);
    productOrdinated[i].push(null)
    count++;
  }

I haven't tried this but you could explore using Array.prototype.fill() instead of the while loop. Something like:

var productOrdinated = new Array();
for(var i=0;i<product.length;i++){
  var count=product[i].length;
  productOrdinated.push(new Array(count));
  product[i].fill(null, count, 49);
  productOrdinated[i].fill(null, count, 49);

Do you want the new arrays pushed into productOrdinated to initially be filled with null, or undefined for the first count elements and then null from count to 49? If you want all elements to be null then you could just add a new array filled with null

productOrdinated.push(new Array(count).fill(null));
product[i].fill(null, count, 49);

Switch statements

Instead of using the large switch statements in the HTML file, consider defining a mapping, perhaps defined just after cache is defined at the top of the page:

var colorMapping = {
    'N': "tblack",
    'W': "twhite",
    'R': "tred",
    'V': "tgreen",
    'J': "tyellow",
    'B': "tblue"
};

Then use that value to assign to textc inside the loop:

var key = product[Number(cache.get("inProductTypeTab"))+1][i*5 + j][10];
var textc = ""; //default value - change accordingly.
if (key in colorMapping) {
    textc = colorMapping[key];
}

And similarly for the other large switch statement.

jQuery click handler

You have this in the click handler:

window.onclick = function(event) {
  if (!event.target.matches('.dropbtn')) {
    var dropdowns = document.getElementsByClassName("dropdown-content");
    var i;
    for (i = 0; i < dropdowns.length; i++) {
      var openDropdown = dropdowns[i];
      if (openDropdown.classList.contains('show')) {
        openDropdown.classList.remove('show');
      }
    }
  }
}

But since you are using jQuery, you can shorten this using a selector that combines the classes to select (i.e. ".dropdown-content.show") and the jQuery method .removeClass().

window.onclick = function(event) {
  if (!event.target.matches('.dropbtn')) {
    $(".dropdown-content.show").removeClass('show');
  }
}
\$\endgroup\$
  • \$\begingroup\$ Code at the end of regenProductCache: Indeed, dunno why I had to put this "IF" statement, but it's not anymore necessary. function ordinateProductTabs: It works well, thanks for that function :p. Switch statements: Have to think how to apply this, can't do that right now, but i will let you be informed. jQuery click handler : I don't really know jquery, had to adapt a code, using it on "id" not "class". The result=> I can toggle by clicking a button, but i can't switch off by clicking outside the button :S. just below, the link to spreadsheet if you want to check: \$\endgroup\$ – LMS Sep 20 '18 at 9:29
  • \$\begingroup\$ [docs.google.com/spreadsheets/d/… To display the table I talk about, ( be patient, very long xD), you have to go on "Menu Génération Zatyoo" on taskbar, then "modification manuel" \$\endgroup\$ – LMS Sep 20 '18 at 9:30

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.