HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Dynamic UI generator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
generatordynamicstackoverflow

Problem

I am developing a query builder which consumes OData Web API (won't go into details on how it works in the back end).

I have this EDM/Metadata which I am using to generate UI components. I created a JSFiddle as well. Is there any way I can improve this code?



`function init() {
$container = $("#container");
$queryBuilderForm = $('Update Url');
$container.append($queryBuilderForm);
$entityFilters = $("");
$queryBuilderForm.append($entityFilters);
}

function processData() {
this.metadata = getMetadata();
for (var e in this.metadata.dataServices.schema) {
var schema = this.metadata.dataServices.schema[e];
if (schema.entityType) {
entities = schema.entityType;
break;
}
}
}

function addExpand(id, options) {

var generateOptions = constructOptions(options, true);
$entityFilters.append($([
'',
'', generateOptions, '',
'',
''
].join('')));

function constructOptions(options, addEmptySelect) {
var opt = [];
if (addEmptySelect) {
opt.push(' -- Select -- ');
}
$.each(options, function(index, e) {
opt.push('' + e.name + '');
});
return opt.join('');
}
}

function addEvents(id) {
var $entities = $('#entities_' + id);
$('body').on('change', $entities, function(event) {
var $e = $(event.target);
var entityName = singularize($e.val());
var entity = $.grep(entities, function(e) {
return e.name == entityName;
})[0];
var columns = entity.property; // if index is not -1

$e.parent().nextAll(".entity-filter").remove();
var $entities_cols = $e.next().empty();
$.each(columns, function(index, e) {
var selectColumnHtmlId = 'selectcolumn_' + e.name;
var $selectColumnLabel = $('', {
'for': selectColumnHtmlId,
text: e.name
});
$selectColumnLabel.appendTo($entities_cols);
$('', {
type: 'checkbox',
id: selectColumnHtmlId,
value: e.name
}).prependTo($selectColumnLabel);

Solution

Naming Things

One, two, very-few letters long variable names are a bad practice.
Even the things like indices can and should be given a real name.
If something is called an i, or e, or ie, it should be treated as if the thing does not have a reason to exist.
If a variable or a parameter exists only because it's enforced by a library/language (e.g. $.each will give us an index and an object args), it is possible to explicitly show which of them are unused by naming them _.
This is a common practice in functional programming.

A very nasty side effect of names like i, e, ci, cd is the added code complexity which appears out of nowhere.
The code is not self-explanatory anymore, and dealing with it requires constant on-the-fly translation, which in turn makes the reader think about unnecessary details rather then the intent of the code.

This is even applicable to the innocent looking id variable/parameter, because without looking around nobody can tell what does the id belong to...

DOM and JavaScript

Depending on the context, it may or may not be a good idea to manipulate HTML DOM directly via JavaScript/JQuery.
Say, in modern single page application frameworks (like Angular, and similar) it's considered a bad practice to deal with DOM in the way it's done in the sample code.
Even with these frameworks, there are exceptions of course, like the cases where one builds a widget library.

Reduce Mutable Things

Try to declare all your variables as const when possible.
Otherwise, make them be let.
Practice shows that the side effects and nuances of var are in the best case not obvious to the most of JavaScript developers, and in the worst case -- very confusing.

Terse Code

Assuming it's okay to use relatively fresh features of ECMAScript 6, certain things may be written more compactly without losing readability.
Syntax constructions like "fat arrow" (=>), spread operator (...), string interpolation, etc. are extremely powerful.
The fat arrow may be very effectively combined with $.each/.foreach, .map, .reduce, .filter, and many many other functions that can accept lambdas.

Here are a few examples of slightly improved function.
Notice that even this code may be further polished.

processData

function processData() {
  return getMetadata() // Notice that we return a value here -- caller has to save the result into a variable
    .dataServices
    .schema
    .find(schema => schema.entityType != null)
    .entityType;
}


Now the invocation changes to var entities = processData();

addExpand

function addExpand(id, options) {
  const generateOptions = constructOptions(options);
  $entityFilters.append($([
    ``,
    `${generateOptions}`,
    ``,
    ``
  ].join('')));

  function constructOptions(options) {
    return [
      ` -- Select -- `,
      ...$.map(options, option => `${option.name}`)
    ]
    .join('');
  }
}


Consider passing $entityFilters into this function as a parameter.
It will improve the code by not having a direct mutative access to global state which is a known source of bugs.

addEvents

function addEvents(id) {
  const $entities = $(`#entities_${id}`);
  $('body').on('change', $entities, event => {
    const $eventTarget = $(event.target);
    const entityName = singularize($eventTarget.val());
    const entity = $.grep(entities, candidate => candidate.name == entityName)[0];
    const columns = entity.property; // if index is not -1

    $eventTarget.parent().nextAll(".entity-filter").remove();
    const $entities_cols = $eventTarget.next().empty();
    $.each(columns, (index, column) => {
      const selectColumnHtmlId = `selectcolumn_${column.name}`;
      const $selectColumnLabel = $('', { 'for': selectColumnHtmlId, text: e.name });
      $selectColumnLabel.appendTo($entities_cols);
      $('', { type: 'checkbox', id: selectColumnHtmlId, value: column.name })
        .prependTo($selectColumnLabel);
    });

    if ($(".entity-filter").length  updateUrl());
}


updateUrl -- excerpt

+= and ternary (?:) operator can sometimes be efficiently combined too, but this kind of changes is really putting many developers on the fence.
I'm trying to stop myself with ternaries, as soon as I start thinking for too long whether it's readable or not...

$("#entityFilters select")
  .each((index, optionItem) => {
    temp_ents += index === 1 ? "?$expand=" : "/";
    temp_ents += index === 0 ? singularize($(optionItem).val(), true) : $(optionItem).val();

Code Snippets

function processData() {
  return getMetadata() // Notice that we return a value here -- caller has to save the result into a variable
    .dataServices
    .schema
    .find(schema => schema.entityType != null)
    .entityType;
}
function addExpand(id, options) {
  const generateOptions = constructOptions(options);
  $entityFilters.append($([
    `<div id="entityFilter_${id}" class="entity-filter">`,
    `<select id="entities_${id}">${generateOptions}</select>`,
    `<div id="entities_${id}_cols"></div>`,
    `</div>`
  ].join('')));

  function constructOptions(options) {
    return [
      `<option value=-1> -- Select -- </option>`,
      ...$.map(options, option => `<option value=${option.name}>${option.name}</option>`)
    ]
    .join('');
  }
}
function addEvents(id) {
  const $entities = $(`#entities_${id}`);
  $('body').on('change', $entities, event => {
    const $eventTarget = $(event.target);
    const entityName = singularize($eventTarget.val());
    const entity = $.grep(entities, candidate => candidate.name == entityName)[0];
    const columns = entity.property; // if index is not -1

    $eventTarget.parent().nextAll(".entity-filter").remove();
    const $entities_cols = $eventTarget.next().empty();
    $.each(columns, (index, column) => {
      const selectColumnHtmlId = `selectcolumn_${column.name}`;
      const $selectColumnLabel = $('<label />', { 'for': selectColumnHtmlId, text: e.name });
      $selectColumnLabel.appendTo($entities_cols);
      $('<input />', { type: 'checkbox', id: selectColumnHtmlId, value: column.name })
        .prependTo($selectColumnLabel);
    });

    if ($(".entity-filter").length < config.expandLimit)
      addExpand($(".entity-filter").length, entity.navigationProperty);
  });

  $("#btnUpdateUrl").on("click", () => updateUrl());
}
$("#entityFilters select")
  .each((index, optionItem) => {
    temp_ents += index === 1 ? "?$expand=" : "/";
    temp_ents += index === 0 ? singularize($(optionItem).val(), true) : $(optionItem).val();

Context

StackExchange Code Review Q#161752, answer score: 4

Revisions (0)

No revisions yet.