Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored code in src/notifications.js to reduce cognitive complexity #167

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactored code in src/notifications.js to reduce cognitive complexity #167

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a1b2d0
Initial fixes by removing nesting and using optional chaining
ajain-1 Jan 23, 2025
5d99b95
Removed optional chaining, not supported yet
ajain-1 Jan 23, 2025
b429475
Merge branch 'CMU-313:main' into main
ajain-1 Jan 23, 2025
abd62a6
Improved nesting in Notifications.merge
ajain-1 Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 30 additions & 26 deletions src/notifications.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,30 +85,32 @@ Notifications.getMultiple = async function (nids) {
const usersData = await User.getUsersFields(userKeys, ['username', 'userslug', 'picture']);

notifications.forEach((notification, index) => {
if (notification) {
intFields.forEach((field) => {
if (notification.hasOwnProperty(field)) {
notification[field] = parseInt(notification[field], 10) || 0;
}
});
if (notification.path && !notification.path.startsWith('http')) {
notification.path = nconf.get('relative_path') + notification.path;
}
notification.datetimeISO = utils.toISOString(notification.datetime);
if (!notification) {
return;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return reduces the complexity of this nested forEach by removing one branch.


if (notification.bodyLong) {
notification.bodyLong = utils.stripHTMLTags(notification.bodyLong, ['img', 'p', 'a']);
intFields.forEach((field) => {
if (notification.hasOwnProperty(field)) {
notification[field] = parseInt(notification[field], 10) || 0;
}
});
if (notification.path && !notification.path.startsWith('http')) {
notification.path = nconf.get('relative_path') + notification.path;
}
notification.datetimeISO = utils.toISOString(notification.datetime);

notification.user = usersData[index];
if (notification.user && notification.from) {
notification.image = notification.user.picture || null;
if (notification.user.username === '[[global:guest]]') {
notification.bodyShort = notification.bodyShort.replace(/([\s\S]*?),[\s\S]*?,([\s\S]*?)/, '$1, [[global:guest]], $2');
}
} else if (notification.image === 'brand:logo' || !notification.image) {
notification.image = meta.config['brand:logo'] || `${nconf.get('relative_path')}/logo.png`;
if (notification.bodyLong) {
notification.bodyLong = utils.stripHTMLTags(notification.bodyLong, ['img', 'p', 'a']);
}

notification.user = usersData[index];
if (notification.user && notification.from) {
notification.image = notification.user.picture || null;
if (notification.user.username === '[[global:guest]]') {
notification.bodyShort = notification.bodyShort.replace(/([\s\S]*?),[\s\S]*?,([\s\S]*?)/, '$1, [[global:guest]], $2');
}
} else if (notification.image === 'brand:logo' || !notification.image) {
notification.image = meta.config['brand:logo'] || `${nconf.get('relative_path')}/logo.png`;
}
});
return notifications;
Expand Down Expand Up @@ -412,7 +414,9 @@ Notifications.merge = async function (notifications) {
'post-queue',
];

notifications = mergeIds.reduce((notifications, mergeId) => {
notifications = mergeIds.reduce(notificationsReducer, notifications);

function notificationsReducer(notifications, mergeId) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduced nesting by making designated functions to reduce the notifications and also to iterate through the differentiators below.

const isolated = notifications.filter(n => n && n.hasOwnProperty('mergeId') && n.mergeId.split('|')[0] === mergeId);
if (isolated.length <= 1) {
return notifications; // Nothing to merge
Expand All @@ -428,7 +432,9 @@ Notifications.merge = async function (notifications) {
return cur;
}, []);

differentiators.forEach((differentiator) => {
differentiators.forEach(differentiatorsIterator);

function differentiatorsIterator(differentiator) {
function typeFromLength(items) {
if (items.length === 2) {
return 'dual';
Expand Down Expand Up @@ -487,7 +493,6 @@ Notifications.merge = async function (notifications) {
} else if (numUsers > 2) {
notifications[modifyIndex].bodyShort = `[[${mergeId}-${typeFromLength(usernames)}, ${usernames.slice(0, 2).join(', ')}, ${numUsers - 2}${titleEscaped}]]`;
}

notifications[modifyIndex].path = set[set.length - 1].path;
} break;

Expand All @@ -501,13 +506,12 @@ Notifications.merge = async function (notifications) {
if (!notifObj || !notifObj.mergeId) {
return true;
}

return !(notifObj.mergeId === (mergeId + (differentiator ? `|${differentiator}` : '')) && idx !== modifyIndex);
});
});
}

return notifications;
}, notifications);
}

const data = await plugins.hooks.fire('filter:notifications.merge', {
notifications: notifications,
Expand Down
Loading