standards-and-practices icon indicating copy to clipboard operation
standards-and-practices copied to clipboard

Proposed new process standard for workarounds in code

Open coreyshuman opened this issue 6 years ago • 9 comments

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.

coreyshuman avatar Jan 31 '19 23:01 coreyshuman

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];
}

carlosvargas avatar Feb 01 '19 20:02 carlosvargas

Oh yeah I like extending this rule to cover magic functions we find online.

coreyshuman avatar Feb 01 '19 23:02 coreyshuman

I'd like to add that you should always cite when you use someone's workaround that you found online.

kmausolf avatar Feb 01 '19 23:02 kmausolf

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 avatar Feb 01 '19 23:02 mwallert

@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.

ryekerjh avatar Feb 01 '19 23:02 ryekerjh

@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 avatar Feb 06 '19 01:02 coreyshuman

@coreyshuman I added issue #95 for this

ryekerjh avatar Feb 06 '19 01:02 ryekerjh

@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?

coreyshuman avatar Feb 29 '20 01:02 coreyshuman

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.

kmausolf avatar Feb 29 '20 01:02 kmausolf