Is it possible to decrease / refactor this if statement?

1

Is it possible to decrease / refactor this if statement

if (params.name) {
  let data = this.store.peekAll('departed').filter(departed => {
    Ember.Logger.log(departed.get('name'));
    return departed.get('name') == params.name;
  });
  if(params.departed != 'undefined') {
    Ember.Logger.log(data);
    
    Ember.Logger.log('departed',params.departed);
    return data.filter(list => {
      return list.get('departed') == params.departed;
    });
  }
  if(params.birth != 'undefined') {
    Ember.Logger.log('birth',params.birth);
    return data.filter(list => {
      return list.get('birth') == params.birth;
    });
  }
  if(params.registry != 'undefined') {
    Ember.Logger.log('registry',params.registry);
    return data.filter(list => {
      return list.get('registry') == params.registry;
    });
  }
  return data;
}
2

This is not necessarily an ember specific question, but there’s a bunch of ways you could do this.

I’ll take a stab at this by showing some of the steps I might take to refactor this.

To start, since you’re repeatedly checking for if the property is defined on params, we could move all of that logic into a separate function (let’s call it maybeFilterByParam):

function maybeFilterByParam(list, params, key) {
  if (params[key]) {
    return list.filterBy(key, params[key]);
  } else {
    return list;
  }
}

let data = this.store.peekAll('departed');
data = maybeFilterByParam(data, params, 'name');
data = maybeFilterByParam(data, params, 'departed');
data = maybeFilterByParam(data, params, 'birth');
return maybeFilterByParam(data, params, 'registry');

I’m still not totally happy at this point — it still feels fairly repetitive since I’m having to pass the same arguments over and over. So, we could make this a factory function. Something like:

const maybeFilterByParamFactory = (list, params) => key => {
  if (params[key]) {
    return list.filterBy(key, params[key]);
  } else {
    return list;
  }
}

let data = this.store.peekAll('departed');
let maybeFilterByParam = maybeFilterByParamFactory(data, params);

data = maybeFilterByParam('name');
data = maybeFilterByParam('departed');
data = maybeFilterByParam('birth');
return maybeFilterByParam('registry');

I think that constantly re-assigning data looks a little funky at this point. So, maybe there’s a way we can express a list of maybeFilterBys and use a .reduce:

function maybeFilterByParams(list, params, ...keys) {
  return keys.reduce((filteredList) => {
    if (params[key]) {
      return filteredList.filterBy(key, params[key]);
    } 

    return filteredList;
  }, list);
}

let data = this.store.peekAll('departed');

return maybeFilterByParams(data, params, 'name', 'departed', 'birth', 'registry');

I’m starting to like this more — but, I I think that maybeFilterByParams is doing too much. Maybe we can make it a teeny bit better by moving out the logic that determines which keys and values from params we should use (then we don’t need to pass it as an argument to our maybeFilterByParams.

function nonEmptyEntriesBy(params, ...keys) {
  return keys
    .filter(key => params[key] != 'undefined')
    .map(key => [key, params[key]]);
}

function filterByParamEntries(list, entries) {
  return entries.reduce((filteredList, [key, value]) => {
    return filteredList.filterBy(key, value);
  }, list);
}

let data = this.store.peekAll('departed');
let paramEntries = nonEmptyEntriesBy(params, 'name', 'departed', 'birth', 'registry');

return filterByParamEntries(this.store.peekAll('departed'), paramEntries);

This last one has no if statement at all. o_O. That might be a little bit too abstract, but maybe not.

Hopefully there’s something here that helps. Let me know if anything I threw out here needs any clarification!

5 Likes
3

Your solution is nice and explamation even better, but it’s not equivalent.

  1. Note that first if checks truthfulness. So that disparity would need to be normalized before your code.

  2. More importantly, each if after first one returns data, ending code flow. That could be modeled by taking first two values of params instead of them all.

4

You can do it in a single functional chain, that acts over the params instead of over the result.

First you list the interesting keys:

return ['name', 'departed', 'birth', 'registry'];

Then you only keep those that are present:

let isPresent = (key, params) => key == 'name' ? params[key] : params[key] != 'undefined';

return ['name', 'departed', 'birth', 'registry']
  .filter(key => isPresent(key, params));

And finally reduce applying the corresponding filter at each step, passing the initial query as starting value for the reduction.:

let isPresent = (key, params) => key == 'name' ? params[key] : params[key] != 'undefined';

return ['name', 'departed', 'birth', 'registry']
  .filter(key => isPresent(key, params))
  .reduce(
    (list, key) => list.filter(item => item.get(key) == params[key]),
    this.store.peekAll('departed')
  );

Maybe you could make better comparisons if you use ‘blankness’ and ‘noneness’, and a static configuration for which check to use:

const checks = {
  name: Ember.isBlank,
  default: Ember.isNone
};

return ['name', 'departed', 'birth', 'registry']
  .filter(key => !(checks[key] || checks.default)(params[key]))
  .reduce(
    (list, key) => list.filter(item => item.get(key) == params[key]),
    this.store.peekAll('departed')
  );
1 Like
5

I’m a little late to the party but couldn’t resist a bit of code golf. My solution would be something like :

 if (params.name) {
   let data = this.store.peekAll('departed');

   let keys = ['name', 'departed', 'birth', 'registry'];
   keys.forEach(p => {
     data = params[p] ? data.filter(d => d.get(p) == params[p] : data;
   });
 }

Or if you knew those where going to be all the props in the params object you could use Object.keys() like so

let data = this.store.peekAll('departed');

Object.keys(params).forEach(p => {
  data = params[p] ? data.filter(d => d.get(p) == params[p] : data;
});
1 Like