Segment builder

Question

I'm writing a pretty complicated piece of UI with angularjs. I've been using angular for about 2 weeks. I'm going to post the controllers I feel are the most confusing to read, and am wondering the following:

Could these be more "angular" (done in a more angular way?)
Is it normal to have this much logic in controllers? They are only modifying scope, and don't hit any external services (yet) or touch the HTML.
Should any of these controllers be directives?? Everything I've needed to do works completely fine in a controller so far.

If I need to include the HTML as well, let me know. Sorry if this is too much code, but you don't have to read all of it.

Edit: I'm creating a segment builder, which segments users in my application based on stuff they've done on our website. This is the UI for picking segments. For example, a segment could be 'Last visit' 'Greater than' 2 'days ago' and 'Last visit' 'Less than' 1 'weeks ago'. This is for building the dynamic UI for check boxes, lists, etc to choose them.

.controller('ConditionCtrl', ['$scope', function($scope) {
  $scope.$on('segment_deleted', function(bogus_event, event_key, segment_index) {
    if ($scope.event.key == event_key && $scope.segment_index == segment_index) $scope.condition_checked = false;
  });

  $scope.condition_checked = $scope.events[$scope.event.key] 
    && $scope.events[$scope.event.key].segments[$scope.segment_index].conditions[$scope.condition.key] != null;

  $scope.condition_check_changed = function() {
    if ($scope.condition_checked)
    {
      $scope.get_or_create_condition($scope.event.key, $scope.segment_index, $scope.condition)
    }
    else
    {
      $scope.delete_condition($scope.event.key, $scope.segment_index, $scope.condition.key)
    }
  }

  $scope.show_comparison_content = function() { 
    return $scope.condition_checked && $scope.condition["content_type_0"] == "comparison";
  }

  $scope.show_other_content = function() {
    return $scope.condition_checked && $scope.condition["content_type_0"] != "comparison";
  }

}])
.controller('ComparisonCtrl', ['$scope', '$rootScope', function($scope, $rootScope) {
  var get_content_value_sets = function()
  {
    return $scope.events[$scope.event.key].segments[$scope.segment_index].conditions[$scope.condition.key].content_value_sets;
  }

  var clear_empty_sets = function()
  {
    if (find_comparison_set_by_value(null).set != null)
    {
      $scope.events[$scope.event.key].segments[$scope.segment_index].conditions[$scope.condition.key].content_value_sets = [];
    }
  }
  var find_comparison_set_by_value = function(value)
  {
    var index = -1;
    var found_set = null;
    $.each(get_content_value_sets(), function(i, set)
    {
      if (set['0'] == value) {
        found_set = set;
        index = i;
      }
    });
    return {index: index, set: found_set};
  }
  clear_empty_sets();
  var set_result = find_comparison_set_by_value($scope.comparsion_value.id)

  $scope.comparison_checked = set_result.set != null;
  $scope.get_set_index = function() {
    return find_comparison_set_by_value($scope.comparsion_value.id).index;
  }
  $scope.$on('update_index_needed', function() {
    $scope.set_index = $scope.get_set_index();
  })
  $scope.comparison_check_changed = function() {
    if ($scope.comparison_checked)
    {
      var current_length = get_content_value_sets().length;
      $scope.set_index = current_length;
      get_content_value_sets().push({'0': $scope.comparsion_value.id, '1': null, '2': null})

    }
    else
    {
      var set_result = find_comparison_set_by_value($scope.comparsion_value.id);
      if (set_result.set != null)
      {
        get_content_value_sets().splice(set_result.index, 1)
        $rootScope.$broadcast('update_index_needed')
      }
    }
  }
}])
.controller('ListContentCtrl', ['$scope', function($scope){
  $scope.select2Options = {
    createSearchChoice: function(term, data) { 
      if ($(data)
        .filter(function() { 
          return this.text.localeCompare(term)===0; 
        }).length===0) 
      {
        return {id:term, text:term};
      }
    },
    placeholder: 'Select or enter a value',
    multiple: false,
    data: $scope.event.values,
    selectOnBlur: true,
    allowClear: true
  };
  var add_row = function() {
    if (!$scope.condition.allow_multi) return;
    if ($scope.selected_value != null && !$scope.added_row)
    {
      $scope.events[$scope.event.key].segments[$scope.segment_index].conditions[$scope.condition.key].content_value_sets.push({'1': null, '2': null, '3': null})
      $scope.added_row = true;
    }
  }
  add_row();
  $scope.update_value = function() {
    var new_val = ''
    if ($scope.selected_value != null && $scope.selected_value.text != null)
    {
      new_val = $scope.selected_value.text;
    }

    $scope.events[$scope.event.key].segments[$scope.segment_index].conditions[$scope.condition.key].content_value_sets[$scope.inner_set_index][$scope.position] = new_val; 
    add_row();
  }
}])
.controller('TextContentCtrl', ['$scope', function($scope){
  var add_row = function() {
    if (!$scope.condition.allow_multi) return;
    if ($scope.selected_value != null && $scope.selected_value != '' && !$scope.added_row)
    {
      $scope.events[$scope.event.key].segments[$scope.segment_index].conditions[$scope.condition.key].content_value_sets.push({'1': null, '2': null, '3': null})
      $scope.added_row = true;
    }
  }
  add_row();
  $scope.update_value = function() {
    var new_val = ''
    if ($scope.selected_value != null && $scope.selected_value != null)
    {
      new_val = $scope.selected_value;
    }

    $scope.events[$scope.event.key].segments[$scope.segment_index].conditions[$scope.condition.key].content_value_sets[$scope.inner_set_index][$scope.position] = new_val; 
    add_row();
  }
}])

What are your code doing ? We would need a bit more of context. — Marc-Andre
– Marc-Andre, Commented Mar 4, 2014 at 18:51
Sorry! I guess that would help. I'm creating a segment builder, which segments users in my application based on stuff they've done on our website. This is the UI for picking segments. For example, a segment could be 'Last visit' 'Greater than' 2 'days ago' and 'Last visit' 'Less than' 1 'weeks ago'. This is for building the dynamic UI for check boxes, lists, etc to choose them — Shawn
– Shawn, Commented Mar 4, 2014 at 18:53
Well thanks for the explanation, but you should add it to the question! It will be more visible for everyone! — Marc-Andre
– Marc-Andre, Commented Mar 4, 2014 at 18:54

konijn · Accepted Answer · 2014-03-06 13:49:49Z

That code is hard too grok, I find it stretches too much horizontally. From that perspective :

Don't skip newlines for if blocks, it is okay to skip the curly braces
No:

if ($scope.event.key == event_key && $scope.segment_index == segment_index) $scope.condition_checked = false;

Yes:

if ($scope.event.key == event_key && $scope.segment_index == segment_index)
    $scope.condition_checked = false;

If all parameters are coming from the same object, consider just passing the object
Hard to read:

$scope.get_or_create_condition($scope.event.key, $scope.segment_index, $scope.condition);

Easier to read:

$scope.get_or_create_condition($scope);

Easiest to read ( assuming you would use this in $scope )

$scope.get_or_create_condition();

lowerCamelCase is good for you: get_or_create_condition -> getOrCreateCondition, personally I would not put the fact that that the function either creates or gets in the function name (TMI), I would simply call it getCondition
It is better to always stick to dot notation so $scope.condition["content_type_0"] -> $scope.condition.content_type_0

I have reviewed a number of angular submissions, I've never seen anything like this:

$scope.events[$scope.event.key].segments[$scope.segment_index].conditions[$scope.condition.key].content_value_sets[$scope.inner_set_index][$scope.position] = new_val;

You need to think about simplifying your object/data structure!

Furtermore, not related to horizontal challenges:

You should compare to null with !== not with != as you do, or, if you can get away with it, use falsey comparisons like if (set_result.set)
You are missing a boatload of semicolons, please use a site like jshint.com to fix those

This:

var new_val = ''
if ($scope.selected_value != null && $scope.selected_value != null)
{
  new_val = $scope.selected_value;
}

could be:

var new_val = $scope.selected_value || '';

thank you however i feel like the majority of this response is stylistic issues which don't actually affect anything and are very objective. the only useful help here was to simplify my object/data structure, but i can't because that is the object/data structure. it's a significantly complicated query builder tree. however, thank you for taking the time to read my submission — Shawn
– Shawn, Commented Apr 4, 2014 at 21:26
A large part of code review is to make sure that other people can maintain your code. Your code is too hard to read. If you were to address the style issues, then maybe we could find issues that are not related to style. — konijn
– konijn, Commented Apr 4, 2014 at 23:02
sort of, i think mostly those are personally preferences. camel case vs underscore, semicolons or no semi colons, triple equals vs double. it's the same shit — Shawn
– Shawn, Commented Apr 7, 2014 at 23:07
right, that is the object structure, which i thought was a valid criticism. you still have not explained any way to reduce the complexity of the object structure. however, i will split that in to two lines with a temp variable for making it more readable. — Shawn
– Shawn, Commented Apr 9, 2014 at 0:42

Stack Exchange Network

Segment builder

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Segment builder

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions