Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Look, ma, no loops!

Look, ma, no loops!

But seriously, if you were concerned about performance I don't think it's a problem of nested loops per se, $.grep in the loop concerns me much more. Especially since you're doing basically the same grep over and over!

After grep, neither item.parent_id nor i are used again. Looks like we can just cycle through all widgets in one $.grep, and that's exactly what we're going to do.

Also, what you're doing with item.widget_key begs for a switch statement - or, at least, having those radiobutton and dropdown typed twice bugs me and begs for a well-placed typo to cause lots of frustration.

I'm not sure I understand exactly what you do with default_next_step, so correctNextStep probably needs some refinement (like, what if next_step - 1 is not a valid forum section index either?) but the code is supposed to work exactly the same as yours. (or not work, hard to say without test cases:D)

const inArray = (needle, haystack) => (-1 != haystack.indexOf(needle));
const parseIntIfDefined = (item) => ('undefined' === typeof(item) ? null : parseInt(item));
const applyFormSectionIdsToControls = function(formSections) {
 if (!formSections.length) return;
 const formSectionIds = formSections.map((section) => parseInt(section.id));
 const correctNextStep = (nextStep) => {
 if (inArray(nextStep, formSectionIds)) return nextStep;
 return formSectionIds[nextStep - 1];
 };
 $.grep(application_widgets, (item) => {
 if (!inArray(parseInt(item.parent_id), formSectionIds)) return;
 switch (item.widget_key) {
 case 'nextbutton':
 const defaultNextStep = parseIntIfDefined(item.settings.default_next_step);
 if (defaultNextStep) {
 item.settings.default_next_step = correctNextStep(defaultNextStep);
 }
 break;
 case 'radiobuttons':
 case 'dropdown':
 item.settings.answers.map(
 (answer) => answer.next_step = correctNextStep(parseInt(answer.next_step))
 );
 break;
 }
 });
}

#Look, ma, no loops!

But seriously, if you were concerned about performance I don't think it's a problem of nested loops per se, $.grep in the loop concerns me much more. Especially since you're doing basically the same grep over and over!

After grep, neither item.parent_id nor i are used again. Looks like we can just cycle through all widgets in one $.grep, and that's exactly what we're going to do.

Also, what you're doing with item.widget_key begs for a switch statement - or, at least, having those radiobutton and dropdown typed twice bugs me and begs for a well-placed typo to cause lots of frustration.

I'm not sure I understand exactly what you do with default_next_step, so correctNextStep probably needs some refinement (like, what if next_step - 1 is not a valid forum section index either?) but the code is supposed to work exactly the same as yours. (or not work, hard to say without test cases:D)

const inArray = (needle, haystack) => (-1 != haystack.indexOf(needle));
const parseIntIfDefined = (item) => ('undefined' === typeof(item) ? null : parseInt(item));
const applyFormSectionIdsToControls = function(formSections) {
 if (!formSections.length) return;
 const formSectionIds = formSections.map((section) => parseInt(section.id));
 const correctNextStep = (nextStep) => {
 if (inArray(nextStep, formSectionIds)) return nextStep;
 return formSectionIds[nextStep - 1];
 };
 $.grep(application_widgets, (item) => {
 if (!inArray(parseInt(item.parent_id), formSectionIds)) return;
 switch (item.widget_key) {
 case 'nextbutton':
 const defaultNextStep = parseIntIfDefined(item.settings.default_next_step);
 if (defaultNextStep) {
 item.settings.default_next_step = correctNextStep(defaultNextStep);
 }
 break;
 case 'radiobuttons':
 case 'dropdown':
 item.settings.answers.map(
 (answer) => answer.next_step = correctNextStep(parseInt(answer.next_step))
 );
 break;
 }
 });
}

Look, ma, no loops!

But seriously, if you were concerned about performance I don't think it's a problem of nested loops per se, $.grep in the loop concerns me much more. Especially since you're doing basically the same grep over and over!

After grep, neither item.parent_id nor i are used again. Looks like we can just cycle through all widgets in one $.grep, and that's exactly what we're going to do.

Also, what you're doing with item.widget_key begs for a switch statement - or, at least, having those radiobutton and dropdown typed twice bugs me and begs for a well-placed typo to cause lots of frustration.

I'm not sure I understand exactly what you do with default_next_step, so correctNextStep probably needs some refinement (like, what if next_step - 1 is not a valid forum section index either?) but the code is supposed to work exactly the same as yours. (or not work, hard to say without test cases:D)

const inArray = (needle, haystack) => (-1 != haystack.indexOf(needle));
const parseIntIfDefined = (item) => ('undefined' === typeof(item) ? null : parseInt(item));
const applyFormSectionIdsToControls = function(formSections) {
 if (!formSections.length) return;
 const formSectionIds = formSections.map((section) => parseInt(section.id));
 const correctNextStep = (nextStep) => {
 if (inArray(nextStep, formSectionIds)) return nextStep;
 return formSectionIds[nextStep - 1];
 };
 $.grep(application_widgets, (item) => {
 if (!inArray(parseInt(item.parent_id), formSectionIds)) return;
 switch (item.widget_key) {
 case 'nextbutton':
 const defaultNextStep = parseIntIfDefined(item.settings.default_next_step);
 if (defaultNextStep) {
 item.settings.default_next_step = correctNextStep(defaultNextStep);
 }
 break;
 case 'radiobuttons':
 case 'dropdown':
 item.settings.answers.map(
 (answer) => answer.next_step = correctNextStep(parseInt(answer.next_step))
 );
 break;
 }
 });
}
Source Link

#Look, ma, no loops!

But seriously, if you were concerned about performance I don't think it's a problem of nested loops per se, $.grep in the loop concerns me much more. Especially since you're doing basically the same grep over and over!

After grep, neither item.parent_id nor i are used again. Looks like we can just cycle through all widgets in one $.grep, and that's exactly what we're going to do.

Also, what you're doing with item.widget_key begs for a switch statement - or, at least, having those radiobutton and dropdown typed twice bugs me and begs for a well-placed typo to cause lots of frustration.

I'm not sure I understand exactly what you do with default_next_step, so correctNextStep probably needs some refinement (like, what if next_step - 1 is not a valid forum section index either?) but the code is supposed to work exactly the same as yours. (or not work, hard to say without test cases:D)

const inArray = (needle, haystack) => (-1 != haystack.indexOf(needle));
const parseIntIfDefined = (item) => ('undefined' === typeof(item) ? null : parseInt(item));
const applyFormSectionIdsToControls = function(formSections) {
 if (!formSections.length) return;
 const formSectionIds = formSections.map((section) => parseInt(section.id));
 const correctNextStep = (nextStep) => {
 if (inArray(nextStep, formSectionIds)) return nextStep;
 return formSectionIds[nextStep - 1];
 };
 $.grep(application_widgets, (item) => {
 if (!inArray(parseInt(item.parent_id), formSectionIds)) return;
 switch (item.widget_key) {
 case 'nextbutton':
 const defaultNextStep = parseIntIfDefined(item.settings.default_next_step);
 if (defaultNextStep) {
 item.settings.default_next_step = correctNextStep(defaultNextStep);
 }
 break;
 case 'radiobuttons':
 case 'dropdown':
 item.settings.answers.map(
 (answer) => answer.next_step = correctNextStep(parseInt(answer.next_step))
 );
 break;
 }
 });
}
default

AltStyle によって変換されたページ (->オリジナル) /