3 ms·
> You're missing likely missing one or more techniques that make this work well: I know how to do it, I just don't always think it's worth it. > Do you happen
by ajuc 5y ago
> You're missing likely missing one or more techniques that make this work well:
I know how to do it, I just don't always think it's worth it.
> Do you happen to have any 100 lines of code that you could provide that would show this as a challenge to compare to the refactored code?
Not 100 lines, just 34, but it's a good example of a function I wouldn't split even if it get to 300 lines.
function getFullParameters() {
const result = {
"gridType": { defaultValue: 1, randomFn: null, redraw: onlyOneRedraw("grid"), },
"gridSize": { defaultValue: 32, randomFn: null, redraw: onlyOneRedraw("grid"), },
"gridOpacity": { defaultValue: 40, randomFn: null, redraw: onlyOneRedraw("grid"), },
"width": { defaultValue: 1024, randomFn: null, redraw: allRedraws(), },
"height": { defaultValue: 1024, randomFn: null, redraw: allRedraws(), },
"seed": { defaultValue: 1, randomFn: () => Math.round(Math.random() * 65536), redraw: allRedraws(), },
"treeDensity": { defaultValue: 40, randomFn: () => Math.round(Math.random() * 100), redraw: onlyOneRedraw("trees"), },
"stoneDensity": { defaultValue: 40, randomFn: () => Math.round(Math.random() * 20 * Math.random() * 5), redraw: onlyOneRedraw("stones"), },
"twigsDensity": { defaultValue: 40, randomFn: () => Math.round(Math.random() * 20 * Math.random() * 5), redraw: onlyOneRedraw("twigs"), },
"riverSize": { defaultValue: 3, randomFn: () => Math.random() > 0.5 ? Math.round(Math.random() * 10) : 0, redraw: onlyRedrawsAfter("river"), },
"roadSize": { defaultValue: 0, randomFn: () => Math.random() > 0.5 ? Math.round(Math.random() * 10) : 0, redraw: onlyRedrawsAfter("river"), },
"centerRandomness": { defaultValue: 20, randomFn: () => Math.round(30), redraw: onlyOneRedraw("trees"), },
"leavedTreeProportion": { defaultValue: 95, randomFn: () => Math.round(Math.random() * 100), redraw: onlyOneRedraw("trees"), },
"treeSize": { defaultValue: 50, randomFn: () => Math.round(30) + Math.round(Math.random() * 40), redraw: onlyOneRedraw("trees"), },
"treeColor": { defaultValue: 120, randomFn: () => Math.round(Math.random() * 65536), redraw: onlyOneRedraw("trees"), },
"treeSeparation": { defaultValue: 40, randomFn: () => Math.round(80 + Math.random() * 20), redraw: onlyOneRedraw("trees"), },
"serrationAmplitude": { defaultValue: 130, randomFn: () => Math.round(80 + Math.random() * 40), redraw: onlyOneRedraw("trees"), },
"serrationFrequency": { defaultValue: 30, randomFn: () => Math.round(80 + Math.random() * 40), redraw: onlyOneRedraw("trees"), },
"serrationRandomness": { defaultValue: 250, randomFn: () => Math.round(100), redraw: onlyOneRedraw("trees"), },
"colorRandomness": { defaultValue: 30, randomFn: () => Math.round(20), redraw: onlyOneRedraw("trees"), },
"clearings": { defaultValue: 9, randomFn: () => Math.round(3 + Math.random() * 10), redraw: onlyRedrawsAfter("clearings"), },
"clearingSize": { defaultValue: 30, randomFn: () => Math.round(30 + Math.random() * 20), redraw: onlyRedrawsAfter("clearings"), },
"treeSteps": { defaultValue: 2, randomFn: () => Math.round(3 + Math.random() * 2), redraw: onlyOneRedraw("trees"), },
"backgroundNo": { defaultValue: 1, randomFn: null, redraw: onlyTheseRedraws(["background", "backgroundCover"]), },
"showColliders": { defaultValue: 0, randomFn: null, redraw: onlyOneRedraw("colliders"), },
"grassLength": { defaultValue: 85, randomFn: () => Math.round(25 + Math.random() * 50), redraw: onlyTheseRedraws(["background", "backgroundCover"]), },
"grassDensity": { defaultValue: 120, randomFn: () => Math.round(25 + Math.random() * 50), redraw: onlyTheseRedraws(["background", "backgroundCover"]), },
"grassSpread": { defaultValue: 45, randomFn: () => Math.round(5 + Math.random() * 25), redraw: onlyTheseRedraws(["background", "backgroundCover"]), },
"autoredraw": { defaultValue: true, randomFn: null, redraw: noneRedraws(), },
};
return result;
}
There's a lot of value in having all of this in one place. Ordering isn't a problem here, just no need to refactor.
- throwaway2037 5y agoI have seen so much GUI code like this in my career! Real world sophisticated GUIs can have tens or hundreds of attributes to setup. Especially ancient Xlib stuff, this was the norm. You have a few functions with maybe hundreds of lines doing pure GUI setup. No problem -- easy to mentally compartmentalise. Your deeper point (if I may theorise): Stop following hard-and-fast rules. Instead, do what makes sense and is easy to read and maintain.
- joshka 5y ago> I know how to do it, I just don't always think it's worth it. Agreed:) Generally no problem with this method other than it's difficult to at a glance see what each item will get set to. Something like the following might be an easy first step: function getFullParameters() { function param(defaultValue, redraws) { return { defaultValue: defaultValue, randomFn: null, redraws }; } function param(defaultValue, randomFn, redraws) { return { defaultValue: defaultValue, randomFn: randomFn, redraws }; } const result = { "gridType": param(1, onlyOneRedraw("grid")), "gridSize": param(32, onlyOneRedraw("grid")), "gridOpacity": param(40, onlyOneRedraw("grid")), "width": param(1024, allRedraws()), "height": param(1024, allRedraws()), "seed": param(1, () => Math.round(Math.random() * 65536), allRedraws()), "treeDensity": param(40, () => Math.round(Math.random() * 100), onlyOneRedraw("trees")), "stoneDensity": param(40, () => Math.round(Math.random() * 20 * Math.random() * 5), onlyOneRedraw("stones")), "twigsDensity": param(40, () => Math.round(Math.random() * 20 * Math.random() * 5), onlyOneRedraw("twigs")), "riverSize": param(3, () => Math.random() > 0.5 ? Math.round(Math.random() * 10) : 0, onlyRedrawsAfter("river")), "roadSize": param(0, () => Math.random() > 0.5 ? Math.round(Math.random() * 10) : 0, onlyRedrawsAfter("river")), "centerRandomness": param(20, () => Math.round(30), onlyOneRedraw("trees")), "leavedTreeProportion": param(95, () => Math.round(Math.random() * 100), onlyOneRedraw("trees")), "treeSize": param(50, () => Math.round(30) + Math.round(Math.random() * 40), onlyOneRedraw("trees")), "treeColor": param(120, () => Math.round(Math.random() * 65536), onlyOneRedraw("trees")), "treeSeparation": param(40, () => Math.round(80 + Math.random() * 20), onlyOneRedraw("trees")), "serrationAmplitude": param(130, () => Math.round(80 + Math.random() * 40), onlyOneRedraw("trees")), "serrationFrequency": param(30, () => Math.round(80 + Math.random() * 40), onlyOneRedraw("trees")), "serrationRandomness": param(250, () => Math.round(100), onlyOneRedraw("trees")), "colorRandomness": param(30, () => Math.round(20), onlyOneRedraw("trees")), "clearings": param(9, () => Math.round(3 + Math.random() * 10), onlyRedrawsAfter("clearings")), "clearingSize": param(30, () => Math.round(30 + Math.random() * 20), onlyRedrawsAfter("clearings")), "treeSteps": param(2, () => Math.round(3 + Math.random() * 2), onlyOneRedraw("trees")), "backgroundNo": param(1, onlyTheseRedraws(["background", "backgroundCover"])), "showColliders": param(0, onlyOneRedraw("colliders")), "grassLength": param(85, () => Math.round(25 + Math.random() * 50), onlyTheseRedraws(["background", "backgroundCover"])), "grassDensity": param(120, () => Math.round(25 + Math.random() * 50), onlyTheseRedraws(["background", "backgroundCover"])), "grassSpread": param(45, () => Math.round(5 + Math.random() * 25), onlyTheseRedraws(["background", "backgroundCover"])), "autoredraw": { defaultValue: true, randomFn: null, redraw: noneRedraws(), }, }; return result; } For someone looking at this for the first time, the rationale for each random function choice is obtuse so you might consider pulling out each type of random function into something descriptive like randomIntUpto(65536), randomDensity(20, 5), randomIntRange(30, 70). Does it add value? Maybe - ask a junior to review the two and see which they prefer maintaining. Regardless, this code mostly exists at a single level of abstraction, which tends to imply simple refactorings rather than complex. My guess is if this extended to multiple (levels / maps / ?) you'd probably split the settings into multiple functions, one per map right...?