Segment builder

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 &&

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 || '';

StackExchange Code Review Q#43430, answer score: 2