standards-and-practices
standards-and-practices copied to clipboard
Proposed new process standard for workarounds in code
I would like to propose a new Shift3 rule: If you add code that is a workaround, you should include a comment which explains why it's there, and links to an official Github issue.
We have an example in our Normalize app, where navigation doesn't work correctly when simultaneously closing a modal window. Under this rule, all instances of the workaround would look something like this:
// This is a workaround for a navigation issue when closing a modal:
// https://github.com/NativeScript/nativescript-angular/issues/1380
return setTimeout(() => Helpers.getRoute("badges", this.router), 50);
This will avoid confusion during code review and during future maintenance. This also documents the issue, so the workaround reasoning is not forgotten in the future.
I second this. While we should add an official Github issue if there is one, I would suggest that we should also add any Stack Overflow or blog post links that were used to come up with the workaround, since most of the time these links have more documentation about the actual issue than the Github issue itself.
I would also extend this to any overly complicated code that we didn't write. For example:
// Calculate the Luminance of a color so that we can
// figure out if we should use a light or dark background
// see: http://stackoverflow.com/a/3943023/112731
function getLuminance(c: RgbArray): number {
let i, x;
const a = []; // so we don't mutate
for (i = 0; i < c.length; i++) {
x = c[i] / 255;
a[i] = x <= 0.03928 ? x / 12.92 : Math.pow((x + 0.055) / 1.055, 2.4);
}
return 0.2126 * a[0] + 0.7152 * a[1] + 0.0722 * a[2];
}
Oh yeah I like extending this rule to cover magic functions we find online.
I'd like to add that you should always cite when you use someone's workaround that you found online.
Yeah these are all good, sometimes we have a lot of "magic" we get from elsewhere which can create confusion when it needs to be adjusted/used elsewhere. @coreyshuman I'm thinking now we create a Shift3 Rules to Code By folder in the root and link it in the base readme. Thoughts everyone?
@mwallert I agree, we should have a designated folder for this stuff with a basic README.md to explain. It will grow over time time sure.
@mwallert @ryekerjh I think that's a great idea. We should make sure the new employee on-boarding / getting started hits that Rules to Code By folder.
@coreyshuman I added issue #95 for this
@mwallert @ryekerjh I'm continuing to find things that don't quite need a full dedicated file, but would be considered standards. Instead of a Rules to Code By folder, I think this could be a single file with important small items such as:
- Use Flow in React
- Use Typescript in Node
- Put business login in the server
- Write unit tests for your business logic
- add a comment with link to resources when implementing a workaround or magic function
Anyone else have opinions?
I feel like this is the situation where we need a Wiki. Separating these out into folders is nice, because I can quickly find what I'm looking for, but having them all in one file prevents us from having to maintain a ton of different folders. A Wiki for this would benefit both ends of this.