3
\$\begingroup\$

My getExtend() function, which will be included via , has two inputs and one output:

  1. HTML file content
  2. config object which determine how to update the HTML, for example to create new script (as the first object in the array extendedHtmlObject ) or update existing script content such as attributes values)
  3. the output should be the extended HTML with all the modifications.

Here is an example config object. Please notice that that in the first element of the array (there is scenario which the next attribute is not provided which say to put the new script at the bottom) the next said put the script tag after script that have ID "ui-boot"

var extendedHtmlObject = [{
                    type: 'script',
                    action: 'new',
                    content: 'console.log(‘hello world’);',
                    next: "ui-boot"
                }, {
                    type: 'script',
                    id: "ui-boot",
                    action: 'upd',
                    innerElem: [{
                        type: 'attr',
                        id: 'data--ui-comersion',
                        content: '1.17'
                    }, {
                        type: 'attr',
                        id: 'src',
                        content: '/test/test2/-co.js'
                    }]
                }

This is the main function:

getExtend: function(htmlContent, extendedHtmlObject) {
    var self = this;
    if (extendedHtmlObject) {
        extendedHtmlObject.forEach(function(configs) {
            switch (configs.type) {
                case 'script':
                    htmlContent = self._handleScriptElement(htmlContent, configs);
                    break;
                case 'head':
                    break;
            }
        });
    }
    return htmlContent;
},

This method determines if I need to create a new script or update existing script attributes according to the input object

_handleScriptElement: function(htmlFilecontent, configEntry) {

    var oExtendedHTML = htmlFilecontent;
    switch (configEntry.action) {
        case 'new':
            oExtendedHTML = this._createNewScript(htmlFilecontent, configEntry);
            break;
        case 'upd':
            var sParsedHtml = this._htmlParser(oExtendedHTML);
            oExtendedHTML = this._updateScript(oExtendedHTML, configEntry, sParsedHtml);
            oExtendedHTML = this._convertHtmlBack(oExtendedHTML);
            break;
    }
    return oExtendedHTML;
},

This is the method for creating new script with two option

  1. the first fork need to parse the html
  2. the second for doesn't.
_createNewScript: function(htmlFilecontent, configEn) {
    var sScriptContent = this._createScript(configEntry.content);
    if (configEn.next != null) {
        var sParsedHtml = this._htmlParser(htmlFilecontent);
        $(sScriptContent).insertAfter(sParsedHtml.find('#' + configEn.next));
        htmlFilecontent = this._convertHtmlBack(sParsedHtml);
    } else {
     //when the script is at the end of file
        var iHeadEndTagPos = htmlFilecontent.search("(| )*</(| )*head(| )*>");
        htmlFilecontent = htmlFilecontent.substr(0, iHeadEndTagPos) + sNewScript + htmlFilecontent.substr(iHeadEndTagPos);
    }
    return htmlFilecontent;
},

This code is redundant(_htmlParser & _convertHtmlBack ) and not efficient(I'm fairly new to JS and use it inside the create and update), could I maybe improve it with JS prototype? I want to do the parse just once in the start and the parseBack at the end(of looping the input object) but the problem is that in the createNewScript the second fork doesn't need to use the parser.

update If I can improve this question and add more info please let me know how, Im really need the code review community help since the code should go to prod and I want it to be more efficient

\$\endgroup\$
3
  • \$\begingroup\$ I'm confused about the relationship between _handleScriptElement() and _changeScriptElement(). \$\endgroup\$ – 200_success Apr 8 '16 at 14:14
  • \$\begingroup\$ @200_success - ohh, sorry there is no _changeScriptElement just _handleScriptElement in this path...I copy it wrong. is it more clear now ? \$\endgroup\$ – Rayn D Apr 8 '16 at 14:35
  • \$\begingroup\$ @200_success - Is it more clear now? can you assist please,unfortunately the answer below is doesn't help much... \$\endgroup\$ – Rayn D Apr 8 '16 at 19:51
3
\$\begingroup\$

Personal preference, but I'd suggest 2-space indentation. You'll most likely nest a lot of stuff in JS, and using 4-space indentation can easily throw you over to the right side of your screen.

Another approach to reducing indentation overhead is to "return early" or offload the functionality to a separate function and use a ternary operation. This is especially when you have single if blocks with no else.

function foo(){
  var something = ''
  if(condition){
    something = doSomething();
  }
  return something;
}

// is the same as returning early:

function foo(){
  var something = ''
  if(!condition) return something;
  something = doSomething();
  return something;
}

// is the same as using a ternary:

function foo(){
  return condition ? doSomething() : '';
}

This is the input: please notice that that in the firstobj of the array (there is scenario which the next attribute is not provided which say to put the new script at the bottom) the next said put the script tag after script that have ID "ui-boot"

When you have to explain your code, that's a sign that you have done something bad. In this case, you had to explain what next in the first element is, and id on the second element.

Being consistent in your objects as well as good naming schemes are a good start:

var extendedHtmlObject = [{
  action: 'new',
  type: 'script',
  properties: null,
  content: 'console.log(‘hello world’);'
  position: {
    anchor: '#ui-boot',
    placement: 'after'
  }
}, {
  action: 'update',
  type: 'script',
  properties: {
    id: 'ui-boot',
    src: '/test/test2/-co.js',
    'data--ui-comersion': '1.17',
  },
  content: '',
  position: null
}];

While there are legitimate use cases for a switch, it's usually a sign that indicates that some function is trying to do too many things. Also, switch can bloat easily and forgetting break is a danger waiting to happen. Consider using key-value pairs instead:

function actions(key, args){
  switch(key){ 
    case: 'foo':
      return doFoo(args);
      break;
    case: 'bar':
      return doBar(args);
      break;
    case: 'baz':
      return doBaz(args);
      break
  }
}

var value = actions(keyargs);

// is the same as

var actions = {
  foo: function doFoo(){...},
  bar: function doBar(){...},
  baz: function doBaz(){...}
};

var value = actions[key](args);

could I maybe improve it with JS prototype?

If this is a singleton module, it doesn't make sense to use prototypes. Prototypes make sense when you create multiple instances that "inherit" the same methods. If you only create one instance, then it doesn't make sense to use a mechanism that was meant for building multiple instances of something.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much, I took your suggestion for config object and understand why not using prototype :) , one more thing, can you please provide the code example with my context since Im not sure 1. where to replace to ternary condition 2. the actions how to use it with the key and args. this will be very helpful for me sir. gratitude \$\endgroup\$ – Rayn D Apr 8 '16 at 12:59
  • \$\begingroup\$ Hi Again ,unfortunately I didnt able to understand your suggestion , can you please give the example in my context, please dont forget that Im using here loop on the object configuration (the first forEach).if you give a hint what should I change in the code with your suggestion it will be very helpful , gratitude. \$\endgroup\$ – Rayn D Apr 8 '16 at 19:54

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.