makingsense/code-style-guides

Code style guides

code-style-guides's Issues

Add style for returning empty collections instead of null

https://blogs.msdn.microsoft.com/ericlippert/2009/05/14/null-is-not-empty/

Create code style guide for AngularJS

Inside the JavaScript section, create folders for frameworks / libraries, include one for AngularJS.

This seems like a good start:

• https://github.com/johnpapa/angularjs-styleguide

Discussion: not using setters and getters

In the JS ES6 code style guidelines, we have the following recommendation:

---

• 23.2 Do not use JavaScript getters/setters as they cause unexpected side effects and are harder to test, maintain, and reason about. Instead, if you do make accessor functions, use getVal() and setVal('hello').

  // bad
  class Dragon {
    get age() {
      // ...
    }
  
    set age(value) {
      // ...
    }
  }
  
  // good
  class Dragon {
    getAge() {
      // ...
    }
  
    setAge(value) {
      // ...
    }
  }

---

However, I don't really see the problem. The hiccups I see with using ES6 getters and setters are:

• They may not directly get or set a value (but may have side effects). This is actually good: it's the OOP design that you get in almost all typed languages for exposed properties. This allows for the internal consistency of a class where the only input and output to it are the public getters, setters and methods.
• There is a need for an internal backing variable This is because this.age will actually invoke the getter function and this.age = 15 will actually invoke the setter function. So in order to preserve a value there's a need for an internal (private! 😀) variable. This is also similar in typed languages.
• They are not harder to test They are just the external API to your class.
• They are not harder to think about A good name works miracles.

Outside of that, there are benefits to this approach (aside from the OOP class design I mentioned earlier): (taken from this SO question)

1. When you realize you need to do more than just set and get the value, you don't have to change every file in the codebase.
2. You can perform validation here.
3. You can change the value being set.
4. You can hide the internal representation. getAddress() could actually be getting several fields for you.
5. You've insulated your public interface from changes under the sheets.
6. Some libraries expect this. Reflection, serialization, mock objects.
7. Inheriting this class, you can override default functionality.
8. You can have different access levels for getter and setter.
9. Lazy loading.
10. People can easily tell you didn't use Python.

---

Proposal: remove that guideline. Allow developers to use getters and setters as much as they want.

Discussion: Not grouping shorthand properties at the top

Hi!

We currently have this guideline in the JS ES6 docs:

---

• 3.5 Group your shorthand properties at the beginning of your object declaration.

  Why? It's easier to tell which properties are using the shorthand.

  const anakinSkywalker = 'Anakin Skywalker';
  const lukeSkywalker = 'Luke Skywalker';
  
  // bad
  const obj = {
    episodeOne: 1,
    twoJediWalkIntoACantina: 2,
    lukeSkywalker,
    episodeThree: 3,
    mayTheFourth: 4,
    anakinSkywalker,
  };
  
  // good
  const obj = {
    lukeSkywalker,
    anakinSkywalker,
    episodeOne: 1,
    twoJediWalkIntoACantina: 2,
    episodeThree: 3,
    mayTheFourth: 4,
  };

---

I think we should not make such recommendation, here's why:

All things being equal, any order will be the same. I don't think having shorthand properties at the top brings any particular value: you still need to read that particular line to understand that it is a shorthand property, regardless of it being in the beginning or the end of the object.

However, in the real world usage, there are certain affinities between methods / properties that have more cohesion than others, and we should allow developers to add them in an order that makes more sense for a mental model of the object.

For example:

const invoice = {
  user: getUser(userId),
  billingAddress,
  shippingAddress,
  shippingMethod: getUserChoice(Constants.SHIPPING_METHOD)

  items: itemsForInvoice,

  subtotal: subtotalForInvoice,
  taxer: taxCalculatorFunction,
  total: currentTotal
};

Note how certain properties belong together for the metal model that we have of an invoice. The blank spaces help construct that mental model. Properties being put before before shorthand / alphabetical order / being a function or object is actually a criteria that does not help code readability, but gives an arbitrary decision to follow.

So, my proposal is: let's just remove this guideline.

What do you think?

Add Microsoft reference

It seems there is a new official style guide from Microsoft: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/inside-a-program/coding-conventions

It would be nice to add a reference at least.

Improve StyleCop/.editorconfig references

We are currently adding the reference to the styles even when we prefer the opposite style. We should make it clear that are only a reference and the value we choose for that style is on the attached tooling files.

As is right now, it looks like an error.

Create code style guide for markdown documentation

Good suggestions to start with:

• https://github.com/carwin/markdown-styleguide
• http://tirania.org/blog/archive/2014/Sep-30.html

SA1309 states that you shouldn't use underscores when naming fields

I know this could turn into a tabs vs. spaces debate but here goes:

The C# style guide links to SA1309 as a means to argue that you should name your private fields starting with underscore instead of using this. prefix, however; SA1309 (which name is FieldNamesMustNotBeginWithUnderscore) states that you should never use underscores for field names and you should in fact use the this. prefix instead.

Also; the General Naming Convention for the .Net Platform Guide states: "X DO NOT use underscores, hyphens, or any other nonalphanumeric characters."