Code Clean Up Suggestions

I suck at JavaScript. I’m more of a backend dev (PHP, Ruby). I’ve tried looking into OOP JavaScript, but something just isn’t clicking or I don’t understand. Anyway, here is some JS I recently made and I’m wondering if I could get some suggestions, tips, or examples on how I can clean this up and make it more elegant.

    /**
     * Conditional Form Fields
     *
     * Show/Hide form fields based on the selected source field
     */
    (function( $ ) {
      'use strict';
      // Project Field
      selectConditional(
        '#dpm_calendar_entry_for_cmb',
        ['proj_schedule', 'proj_milestone'],
        '#dpm_calendar_project_cmb',
        '#dpm_calendar_task_cmb'
      );
      // Task Field
      selectConditional(
        '#dpm_calendar_entry_for_cmb',
        'task_due_date',
        '#dpm_calendar_task_cmb',
        '#dpm_calendar_project_cmb'
      );
    })( jQuery );
    /**
     * Clear the value of the field if another source option is selected
     * 
     * @param  string/array target the target field(s) to clear
     * @return void
     */
    function clearValue(target){
      var target = $(target+' select');
      // Do nothing if target doesn't have a value
      if (typeof target === "undefined" || target === null) {
        return;
      }
      // Check if is an array
      if($.isArray(target)) {
        // Iterate through array
        $.each(target, function(index, value) {
          // Clear each old target
          value.val('');
        });
      } else {
        // Clear old target
        target.val('');
      }; 
    }
    /**
     * Initiate the conditional checks
     * 
     * @param  string source The source field
     * @param  string/array condition The condition(s) for displaying the target field(s)
     * @param  string/array target The field(s) to target
     * @param  string/array clear The fields to clear when sorce is switched
     * @return void
     */
    function selectConditional(source, condition, target, clear) {
      'use strict';
      var source = $(source+' select');
      // Run showFields function
      showFields(source, condition, target, clear);
      // Watch for change in the source
      source.change( function(){
        // Run showFields function
        showFields(source, condition, target, clear)
        // Run the clearValue function
        clearValue(clear);
      });
    }
    /**
     * Show/Hide form fields based on the selected source field
     * 
     * @param  string source The source field
     * @param  string/array condition The condition(s) for displaying the target field(s)
     * @param  string/array target The field(s) to target
     * @param  string/array clear The fields to clear when sorce is switched
     * @return void
     */
    function showFields(source, condition, target, clear) {
      'use strict';
      var target = $(target);
      // Check if is an array
      if($.isArray(condition)) {
        // Iterate through array
        $.each(condition, function(index, value) {
          // Check if source value equeals each of the targets
          if(source.val() === value) {
            // Check if is an array
            if($.isArray(target)) {
              // Iterate through array
              $.each(target, function(index, value) {
                // Show the form field
                target.css({'display': 'block'});
              });
            } else {
              // Show the form field
              target.css({'display': 'block'});
            }
          }
        });
      } else {
        // Check if source value equeals the target
        if(source.val() === condition) {
          // Check if is an array
          if($.isArray(target)) {
            // Iterate through array
            $.each(target, function(index, value) {
              // Show the form field
              target.css({'display': 'block'});
            });
          } else {
            // Show the form field
            target.css({'display': 'block'});
          }
        }
      }
    }
2 Likes

Hi there,

Whilst I can’t offer you any immediate suggestions, as I read your query, I did think it may be helpful to understand what you are expecting the code to do for you, as you may get a more considered response on ways of improving it. What do feel is inelegant about it?

1 Like

The first thing you could do is to get rid of those comments that just repeat the obvious and make the script harder to read.

For example:

// Run showFields function
showFields(source, condition, target, clear)

That comment is worse than useless as it contributes nothing to telling you what the next line does (the line itself tells you more about what it does than the comment does) and it slows down the loading and execution of the script and makes the code harder to read.

In general keep comments to the top of functions and use them to explain how/why - the code itself should explain what (if it doesn’t then your names are not descriptive enough).

1 Like

Definitely for production, but those comments are there just for me right now during development. They make it easier for me to quickly scan the code to find what I need.

2 Likes

What the code actually does is really irrelevant. I just want to break away from the generic procedural code model. Like I said, I looked onto JS OOP, but I’m just missing something.

I’m all about OOP in my backend code. I know there are a few different ways to do that in JS, but I don’t work with it enough or have the time to really dig into to it like I should. I’m just looking for an example with this code to help get better at my JS.

I tried building it with an object model, but I just can’t get it to work.

As a fellow-backend developer I can empathize with your difficulty. Javascript was not born as an Object Oriented language. It has evolved into one. In many cases, it feels like it came along kicking-and-screaming.


I agree the comments have less value after the code is working.
A habit I have developed is to write the comments first. In other words, when I need to construct a function, I will write a list of steps (as comments) that I expect to be executed. A bit more ‘human’ and descriptive than pseudo-code but “code-like” enough to guide me. It helps me to not forget important elements.
Then, by following my “notes” I can flesh-out the function.

1 Like

I do the same, but I call them “outlines”

As a simple contrived example

/*
get input name field
make uniform by changing everything to lowercase
make first letter of each name uppercase
handle apostrophe names eg. O’Reilly
handle suffixes eg. Richard III
return styled name
*/

But I rarely, if ever, leave such comments there for long, preferring good naming to take it’s place.

As far as cleaning up the code, I’m wondering if it might be better to break down the function into smaller functions instead of having so many conditionals. eg. borrowing from PHP
function showFields(source, [mixed] condition, [mixed] target, clear) {
where mixed in this case means it could be an array or maybe not.

2 Likes

I always thought that JavaScript WAS born as an OO language. If not then OO had been added by the time the ECMAscript version 1 standard was released in 1997. So if it wasn’t OO from the start it had become so within the first 18 months.

I also do that sometimes. I don’t insert the code into the middle of the comments though as the OP has done. The code would get added after the comments and then once all of the code is added then the comment block would be removed.

Having code and comments that basically just say the same thing as the code does jumbled together as the OP has just makes it a lot harder to see what the code actually does. If I was looking at how to improve the code as the OP is then the first thing I’d do would be to get rid of all the unnecessary comments so as to make it easier to read what the code does.

1 Like

Hi jawittdesigns,

There are a few changes you could make to the code which would simplify it quite a bit. Starting off with the clearValue function, here is the current code:

function clearValue(target){
  var target = $(target+' select');

  if (typeof target === "undefined" || target === null) {
    return;
  }
  
  if($.isArray(target)) {
    $.each(target, function(index, value) {
      value.val('');
    });
  } else {
    target.val('');
  }; 
}

The first thing you could do is to remove the second IF statement by taking advantage of the way jQuery works. Some methods (such as .val()) can be called on both single elements, or on a collection of elements. When called on a collection, jQuery will apply the method to each element in turn, so we can call target.val('') and not have to worry about how many elements matched the selector.

Another result of the way jQuery works is that it will return an empty collection if no matching elements are found. Calling .val('') on an empty collection will just fail silently, which means we can also remove the first IF statement, reducing the function to:

function clearValue(target) {
  $(target+' select').val('');
}

You could just now remove the function and inline this code into selectConditional, although some would argue it’s still worth keeping as a separate method.

Looking at showFields, there’s some duplication caused by having to check if condition and target are arrays. To simplify things you could ensure that condition is always an array, even if it only contains a single string. That way, you can change the code to check if the value of source exists in the array, like this:

if (condition.indexOf(source.val()) > -1) {
  //...
}

The second check (for target) can be removed in the same way that we did for clearValue, by letting jQuery worry about if it’s a single element or a collection. The refactored function ends up looking like this:

function showFields(source, condition, target) {
  'use strict';
  
  if (condition.indexOf(source.val()) > -1) {
    $(target).css({'display': 'block'});
  }
}
5 Likes

Awesome, I’ll try these out. Thanks just the kind of advice I was looking for.

I second that. just because JS OOP is different from C++ OOP doesn’t mean it’s not OOP at all.

Have you considered using typescript to write your javascript code? As it offers an OOP way of programming that compiles into native Javascript.

Typescript

Hey dragonlord, you can actually do OOP in native JavaScript. TypeScript’s main difference from JavaScript is that it adds static typing.

As I understand it, it depends on what version of JS you’re using as to how “supported” OOP is. Also it’s only with EMCA 6.0 that first class support for OOP is being introduced rather than needing to use work arounds that simulate OOP.

JavaScript has been an OO object from day one AFAIK. Where people usually get confused is that JavaScript’s OO features are prototype-based rather than class-based. ES6 introduced some ‘syntactic sugar’ to make using objects and inheritance feel more familiar to developers used to class-based OO languages, but it’s still prototypes all the way down under the hood.

It was OO before it was handed over to ECMA in 1997 as the first ECMAScript standard makes lots of references to it being OO.

The “class” support introduced in ES6 is merely a wrapper around one of the many OO constructs that JavaScript supports to make it look more like the far more primitive classical OO that is the only type of OO that many other OO languages support. JavaScript also continues to support the far more powerful prototypal OO so that you don’t have to dumb down your OO if you don’t want to.

I would also suggest changing the parameters for selectCondition to accept a config object:

      selectConditional({
        source: '#dpm_calendar_entry_for_cmb',
        condition: 'task_due_date',
        target: '#dpm_calendar_task_cmb',
        clear: '#dpm_calendar_project_cmb'
      });

which will make it clearer at the calling site what each thing does. Then just validate the passed in params at the top of the function.

“classes” !== “OOP”

More often than not I use isolated closures with a single high level object literal for each feature.

./*
  * Some type of comment about what this feature is suppose to do.
  */
(function() {

  var handler = {
    init: function() {
      window.addEventListener('load',handler.onWindowLoad)
    },
    onWindowLoad: function(evt) {
      // Do not use this because this would refer to the window, safer to just use handler.
      handler.doSomethingElse();
    },
    doSomethingElse: function() {
       ...
    }
  };

  handler.init();

})();

As a boilerplate example rules I tend to follow are below.

1.) All functions associated with the feature go in the handler.
2.) I never use this. Instead I reference handler directly to avoid loosing object context.

This pattern works well for much or the JavaScript I have been doing lately which is primarily adding JS behavior to an existing HTML/traditional website. I find it simple yet effective in not cluttering the global namespace and keeping separate features independent of one another (unless there needs to be cross pollination). I also typically use jQuery and pass that to the closure not resorting to the in browser event listening work flow. I’m not even sure if that is correct in my example.

1 Like

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.