Adding genres and themes from work

Question

Codacy is telling me this method has a complexity of 13. I need some tips on how to reduce it, more specifically inside the forEach loop.

I've already changed it from an if/else` if hell, but it seems it wasn't enough.

function addGenresAndThemesFromWork(work){
 let workDetails = {workId: work.$.id, genres: [], themes: []};
 let workInfo = work.info;
 if (typeof workInfo === 'object') {
 Object.keys(workInfo).forEach(function (key) {
 let workfInfoType = workInfo[key].$.type;
 let workInfoValue = workInfo[key]._;
 switch (workfInfoType.toLowerCase()) {
 case 'genres':
 workDetails.genres.push(workInfoValue);
 if (Genres.findOne({name: workInfoValue}) === undefined) {
 Genres.insert({
 name: workInfoValue
 });
 }
 break;
 case 'themes':
 workDetails.themes.push(workInfoValue);
 if (Themes.findOne({name: workInfoValue.toLowerCase()}) === undefined) {
 Themes.insert({
 name: workInfoValue.toLowerCase()
 });
 }
 break;
 case 'main title':
 workDetails.name = workInfoValue;
 break;
 case 'plot summary':
 workDetails.plot = workInfoValue;
 break;
 case 'objectionable content':
 if (workInfoValue.toLowerCase() === 'ma') {
 workDetails.mature = true;
 }
 break;
 case 'picture':
 let workImg = workInfo[key].img[1];
 if (workImg !== undefined) {
 workDetails.picture = workImg.$;
 } else {
 if (workInfo[key].img[0] !== undefined) {
 workDetails.picture = workInfo[key].img[0].$;
 }
 }
 break;
 case 'alternative title':
 if (workInfo[key].$.lang.toLowerCase() === 'ja') {
 workDetails.alternativeTitle = workInfoValue;
 }
 break;
 }
 });
 }
 if (!_.has(workDetails, 'picture')) {
 workDetails.picture = {src: '/default.jpg', temporary: true};
 }
 persistWorkDetails(workDetails);
 }

Answer 1

I don't have a tool here to count, so I don't know how the numbers will play out. That said, I agree with the interpretation since there's a lot of things going on here and it makes sense to detangle the function into smaller bits.

The big if block immediately suggests that some kind of short-circuiting or decomposing into functions would be useful. E.g. if workInfo is not an object, call persistWOrkDetails and just return; the other branch will continue afterwards and also persists the object.

Then again, I'd rather split a function off in this case, though I have no idea what to call it.

function addGenresAndThemesFromWork(work) {
 let workDetails = {workId: work.$.id, genres: [], themes: []};
 if (typeof work.info === 'object') {
 fillInWorkDetails(work.info, workDetails);
 }
 if (!_.has(workDetails, 'picture')) {
 workDetails.picture = {src: '/default.jpg', temporary: true};
 }
 persistWorkDetails(workDetails);
}

Next up are the genres and themes, which exhibit a common pattern, so again, move that into a separate function. Sounds like that is a function that might already exist in a library anyway.

function pushNew(thing, name) {
 let value = {name: name};
 if (thing.findOne(value) === undefined) {
 thing.insert({name: name});
 }
}
...
case 'genres':
 workDetails.genres.push(workInfoValue);
 pushNew(Genres, workInfoValue);
 break;
case 'themes':
 workDetails.themes.push(workInfoValue);
 pushNew(Themes, workInfoValue.toLowerCase());
 break;

For "objectionable content" (I wonder) the if can be replaced with an or, that is:

workDetails.mature = workDetails.mature || workInfoValue.toLowerCase() === 'ma';

The syntax is a bit longer, but a temporary variable could be used to deal with that.

• Is there a particular reason to use Object.keys instead of for ... in on the workInfo object?
• The nested else { if ... } can just be written as else if in this case.
• workInfo[key] appears way too often, just assign it to a shorter name.
• Since you create workDetails yourself, you probably don't have to use _.has, but just plain in.
• The picture default value can be moved into the workDetails initialisation since the loop will always overwrite the existing value. That also takes care of any possible issues with undefined.
• If a function (like findOne) only returns a false value if couldn't find something, then the comparison can also simply be if (!findOne(...)) - I'm assuming that that's done for a reason though.

With all that in mind I'm arriving at the following, which, depending on the exact format and semantics of the objects could be further distilled, but I think this is good enough for now and for you to continue on it later.

I'd also suggest that the workDetails object is filled in with more defaults so it's actually clear that e.g. a missing mature field actually means the same as mature: false in the initialisation (or not, in which case the code below is slightly wrong).

function pushNew(thing, name) {
 let value = {name: name};
 if (thing.findOne(value) === undefined) {
 thing.insert(value);
 }
}
function pictureValue(img) {
 if (img[1] !== undefined) {
 return img[1].$;
 } else if (img[0] !== undefined) {
 return img[0].$;
 }
 return undefined;
}
function fillInWorkDetails(workInfo, workDetails) {
 for (let key in workInfo) {
 let value = workInfo[key];
 let workInfoValue = workInfo[key]._;
 switch (value.$.type.toLowerCase()) {
 case 'genres':
 workDetails.genres.push(workInfoValue);
 pushNew(Genres, workInfoValue);
 break;
 case 'themes':
 workDetails.themes.push(workInfoValue);
 pushNew(Themes, workInfoValue.toLowerCase());
 break;
 case 'main title':
 workDetails.name = workInfoValue;
 break;
 case 'plot summary':
 workDetails.plot = workInfoValue;
 break;
 case 'objectionable content':
 workDetails.mature = workDetails.mature || workInfoValue.toLowerCase() === 'ma';
 break;
 case 'picture':
 workDetails.picture = pictureValue(value.img) || workDetails.picture;
 break;
 case 'alternative title':
 if (value.$.lang.toLowerCase() === 'ja') {
 workDetails.alternativeTitle = workInfoValue;
 }
 break;
 }
 }
 return workDetails;
}
function addGenresAndThemesFromWork(work) {
 let workDetails = {
 workId: work.$.id,
 genres: [],
 themes: [],
 picture: {src: '/default.jpg', temporary: true}
 };
 if (typeof work.info === 'object') {
 fillInWorkDetails(work.info, workDetails);
 }
 persistWorkDetails(workDetails);
}