Coding Styles and Code Formatting

[lirantal]

Topic

Coding Styles and Code Formatting across the MEAN.JS project code base.

Conventions

• Named functions (no anonymous functions allowed)

Tool

We chose ESLint as the upcoming tool to perform the job, if you would like to propose anything else please comment with the pros/cons (see reference for PR request: #763)

[lirantal]

@ilanbiala @mleanos @codydaig @trainerbill @rhutchison we've got this place to discuss items now related to coding conventions. You're welcome to raise all the formatting and code style that were raised in other PRs here and we'll add them to the main issue description as this grows.

[codydaig]

No nested functions! It should be a closure instead: ie.

Incorrect:

function foo() {
  var user = 'cody';

  someFunctionReturningAPromise()
    .then(bar);

  function bar() {
    // uses the user variable
  }
}

Correct:

function foo() {
  var user = 'cody';

  someFunctionReturningAPromise()
    .then(bar(user));
}

function bar(user) {
  return function () {
    // Yay Closures!!
  }
}

[ilanbiala]

It should actually just be .then(bar) to keep it simplest.

[codydaig]

@ilanbiala so you prefer nested functions?

[trainerbill]

This is bound to ruffle some feathers, but for the client side / angular, can we just put conform to john papa style guide? Or are we creating a whole new style guide for this project? I personally like the john papa style guide, and they already have it documented to the T. No need to reinvent the wheel IMO.

[ilanbiala]

@codydaig not nested functions. I'm not sure how you get that. Look at this example with bluebird (the concept holds for promise as well):

// orders.between
exports.between = function findOrdersBetween(d1, d2) {
  var p = Promise.defer();

  Order
        .find({})
        .where('created').gte(d1).lte(d2)
        .sort('created')
        .populate('items')
        .exec(function(err, orders) {
            if (err) {
                p.reject(err);
            } else {
                p.resolve(orders);
            }
        });

    return p.promise;
};

function createDay(orders) {
    var p = Promise.defer();

    var today;

    today = new Day() // options omitted

    today.save(function(err, today) {
        if (err) {
            p.reject(err);
            console.error(err);
        } else {
            p.resolve(today);
        }
    });

    return p.promise;
}

function emailCSV(csvContents) {
    var today = new Date();
    var options = {
        attachments: [{
            filename: 'HTHS-bcc-' + today.toDateString().replace(/\s/g, '-') + '.csv',
            content: csvContents.oldOrderCSV
        }, {
            filename: 'HTHS-orders-' + today.toDateString().replace(/\s/g, '-') + '.csv',
            content: csvContents.customerCSV
        }, {
            filename: 'new-' + today.toDateString().replace(/\s/g, '-') + '.csv',
            content: csvContents.orderCSV
        }]
    };

    /* sending email code here */
}

orders
  .between(yesterday, today)
  .then(createDay)
  .then(emailCSV)
  .catch(function(err) {
    console.error('Error: ', err);
  });

I've removed some of the code for brevity, but look at the final promise chain. All I'm doing is returning the promise from each function and resolving/rejecting with something, and the following then/catch has access to it.

[ilanbiala]

@trainerbill I think we've already justified why certain things don't apply regarding John Papa's style guide.

[trainerbill]

@ilanbiala Then can we please link to the pieces we are using? A couple that I would put in there to start:

https://github.com/johnpapa/angular-styleguide#iife
https://github.com/johnpapa/angular-styleguide#modules
https://github.com/johnpapa/angular-styleguide#controllers
https://github.com/johnpapa/angular-styleguide#factories
https://github.com/johnpapa/angular-styleguide#route-resolve-promises
https://github.com/johnpapa/angular-styleguide#routing

[mleanos]

@ilanbiala In your example the emailCSV function is expecting the parameter csvContents to be the resolved data from the previous promise in the chain (createDay which resolves the today data).

Closures can be useful for when you need to pass additional parameters to your functions that are being called in a chain of promises.

To keep with your example, you may actually need to pass csvContents into the emailCSV function, and you will also need access to the today data that's being resolved. Using a closure, your function declaration would now look like this..

function emailCSV(csvContents) { // passed in from the chain
  return function (today) { // passed in from the previous resolved promise
    // do all your work here
  }
}

And then your chain would be this..

orders
  .between(yesterday, today)
  .then(createDay)
  .then(emailCSV(csvContents))
  .catch(function(err) {
    console.error('Error: ', err);
  });

[ilanbiala]

@mleanos while slightly less efficient, it's better in my opinion to pass around more data in case you ever need it and end up with clearer code. Closures in my opinion are nowhere near as clear as having an object result = {csvContents: "", today: ""} with a little extra info and ending up with a very simple promise chain that is clearer because it is consistent in its execution setup.

[mleanos]

@ilanbiala The problem with that strategy is that you end up having to pass around this extra info into each of your functions that are returning promises. They become too dependent upon data that it shouldn't even be concerned with. The only data that the promisified functions should be concerned with, is the data that it is calculating & resolving. This way they can act autonomously. Any extra data that needs to be passed around is almost always coming from the scope that the promise chain exists in, and should remain there.

[mleanos]

@trainerbill I'm fine with your proposed list of styles from John Papa. However, I'm still not convinced of the IIFE's; but I won't fight it. It would be nice to know the exact benefits of using them; with examples.

I'm not sure if this was implied by your list but I would add this as well...
https://github.com/johnpapa/angular-styleguide#bindable-members-up-top

[codydaig]

@ilanbiala That object idea only works if your making each of the functions that return a promise, but if you throw in a function from a node module that returns a promise, you can't pass it or expect it to return the object that contains everything.

[trainerbill]

@mleanos IIFE's are pretty much the most important part of the style guide and without them I am fairly certain we could not implement the rest of the style guide. For example if you are doing controller definitions without them, then if anyone made a function called ArticleController, which is probably rare but could happen, then the entire module would blow up as ArticleController would be global and overwritten. There is a reason it is #2 on the list as the rest of the style guide depends on it. And we are talking about doing non-nested functions which would introduce all the functions into the global scope and we would no doubt have conflicts. function save,update, etc.

[ilanbiala]

@mleanos the functions aren't dependent on that data existing, they unaware of it even being there, just that their necessary data is 1 level deeper in the argument. I've had no issues with it yet and my code has been much cleaner due to no closures and a simple promise chain.

[mleanos]

@trainerbill Yes. I understand that. You don't have to convince me. I'm not resisting IIFE's anymore.

[mleanos]

@ilanbiala I just don't see how that follows any sort of pattern of separation of concerns. I can see problems arising when the requirements of the functions, or the implementations around them, start to change. You would end up having to make modifications to the promisified functions that merely pass this data around. That doesn't seem very DRY to me.

For me, the closures are easy to read & make it very clear as to what's going on (unambiguous). I guess I don't see them as unreadable like you. However, I think their functional benefits would outweigh any concerns of their readability.

To further elaborate on @codydaig example..

function foo() {
  var user = 'cody';

  someFunctionReturningAPromise() // let's say this promise resolves with a list of products
    .then(productMetric(user))
    .then(processMetric)
    .then(reportSuccess)
    .catch(handleError);
}

function productMetric(user) {
  return function (products) {
    return someExternalLibraryReturningAPromise(user, products); // resolves with a metric of the likelihood that this user will order any of these products
  }
}

function processMetric(metric) { // no closure needed
  return someOtherFunctionReturningAPromise(metric); // resolves with a success response
}

function reportSuccess (response) {
  // do some extra work and/or log info to the console
}

function handleError (error) {
  // do some error handling here
}

All of this seems very clear, and easy to follow for me. I don't see any ambiguity with exactly what this promise chain is doing. All the while, each promisified function merely takes in the data that it is concerned with.

[ilanbiala]

@mleanos it also depends on global functions, and ideally we don't do have any except for modules. I'm not a fan of the approach, and you would still have to refactor your closures and the corresponding code for your approach.

[codydaig]

@ilanbiala How would you have handeled these situations then?
https://github.com/lirantal/meanjs/blob/6f8b5bfd58bf1e378a5245d5114c23e44317b728/config/lib/seed.js#L51
https://github.com/meanjs/mean/pull/937/files#diff-7dfe67f86c4c0773b98c0422a0cb606aR19

[ilanbiala]

@codydaig like I said, pass around a little bit more data and just keep passing it down the chain. Slightly less clean for the argument, but it removes closures and simplifies chains.

[codydaig]

@ilanbiala but then your forcing the database functions to handle data from multiple area of the site.
https://github.com/lirantal/meanjs/blob/6f8b5bfd58bf1e378a5245d5114c23e44317b728/config/lib/seed.js#L139
Which introduces an issue of name conflict across different portions of the site. Also, if the generate function wasn't our own promise function, but instead it was a built in mongoose function, then your idea of passing objects around would not work.

[ilanbiala]

@codydaig you can always create your own promise and return it. I'm not sure what you mean by name conflict.

[mleanos]

@ilanbiala If you create your own promise to accommodate this, then you can't just simply return the other promise (the one you're really interested in). You end up having to handle the resolve, and catch, inside this new promise to add this extra info to your data object. This will end up making things more complicated and less DRY. Think of how complicated things will get if you implement this type of strategy at every level of abstraction; it becomes unmanageable.

[ilanbiala]

@mleanos I've had no issues with this system and I've used it quite a bit and I've had no problems.

[mleanos]

@ilanbiala I would expect it works fine for your use case. You probably just haven't needed to extend it much outside of the scope for how you need it to behave. Even though it may be working out for you in your specific use case, doesn't mean that it is DRY and follows proper SOC.

I would be worried that you may paint yourself into a corner, using that strategy. It's fine, until you need to get out of that corner; so to speak.

It may just boil down to differences of opinion and/or preference. In the end, I just don't see the readability issues that you see with the strategy I'm advocating for.

[lirantal]

Guys, @trainerbill has a point. We've not going to re-invent the wheel with regards to coding styles. Let's pick an existing coding style that best matches our desires and if we can't find a 100% perfect match for such code style then we can reference the 90% of it, and for the 10% that we don't want to follow we can state our own.

What do you say?

[ilanbiala]

@lirantal yep we can just state what conventions we will ignore.

[ilanbiala]

I think the JP has most of what we want.

[codydaig]

I'm cool with just sticking with that style guide.

[mleanos]

John Papa's guide seems sufficient to me as well.

[lirantal]

Ok, but John Papa's style is just for angular, right?
Can we re-use the same style guide for the server-side? For example regardless if we want to adopt IIFE or not, it doesn't make much sense for nodejs.

[mleanos]

@lirantal I think we can take some of John Papa's styles to the server-side. Along with our strategy of handling promises.

@codydaig Has a great example of how this would look. Care to share @codydaig ?

I like the modular structure of the code using John Papa's guide. The server-side components would end up looking, and acting, more like services; which I think we should strive for.

[codydaig]

https://github.com/nobsjs/nobsjs/blob/master/modules/core/server/controllers/core.controller.server.js

That is a general sense of what I would like to strive for. My only objection to that example is closures. We didn't use closures in that project, we used nested functions. We're in the process of refactoring, but the changes have not been pushed yet.

[ilanbiala]

I'm not a fan of functions in functions, and I also think closures in Node don't really offer much benefit.

[mleanos]

I think this discussion got a little derailed. I think we should stick to the client-side implementation of John Papa's style guide. We can have a separate discussion if we want to take some of this to the server-side.

With that said, where do we stand with @trainerbill suggested styles?

@codydaig @ilanbiala @jloveland @lirantal @rhutchison @trainerbill

[codydaig]

I agree.

[gustavodemari]

For server-side, I like @felixge Node.js style guide

[codydaig]

@gustavodemari I like that style guide as well. But for sanity right now, let's keep this discussion to client side. We've got outstanding PRs waiting on this decision.

[gustavodemari]

@codydaig No problem, it is just a suggestion for, maybe, use this guide as a starting point for server-side discussion.

[ilanbiala]

@gustavodemari @codydaig @lirantal I think we should just have 2 sections at the top for client and server side. Also, Felix's is pretty good, except for these in my opinion:
https://github.com/felixge/node-style-guide#use-multi-line-ternary-operator
https://github.com/felixge/node-style-guide#no-nested-closures

In my opinion those should be handled case by case, because that ternary operator is quite short and I think it is just unnecessary to break it up.

[codydaig]

I don't like the multi line ternary operator either, but I do like the no nested closures.

[ilanbiala]

@codydaig nested closures in general are probably bad, but his example seems to simple to not do that in my opinion. I think if it's simple enough, then it's fine.

[mleanos]

By writing your code without nested closures, you'll end up with cleaner & more reusable code; making things much more modular. For instance, two separate resolved promises could use the same closure function to satisfy a need. Even in simple examples, it makes sense for the purpose of extending the use case in the future.

I don't like the multi-line ternary either; although I'm tempted if one becomes complex :)

This is really the only only other thing I don't agree with in Felix's style guide.
https://github.com/felixge/node-style-guide#method-chaining

I really prefer this. It looks much better & is very clear to me. With the "right" way, it's hard to see where the chain ends when scanning the code; due to the extra spacing/whitespace

User
.findOne({ name: 'foo' })
.populate('bar')
.exec(function(err, user) {
  return true;
});

[ilanbiala]

@mleanos the indentation makes sense just like it's a function.

[codydaig]

@mleanos I have to agree with @ilanbiala on that one. Indentation makes more sense too me as well.

[mleanos]

@codydaig @ilanbiala I know. I think it's just a weird preference of mine. The rest of the world is wrong, I'm right :) j/k

[lirantal]

We should indeed create 2 different style guides:

1. AngularJS frontend
2. NodeJS backend

Seems like airbnb's style guide is also quite a popular option on Github: https://github.com/airbnb/javascript though it's quite long and comprehensive compared to Felix's.

For NodeJS I think the choice is easier to make.
I'm not expert at Angular so you guys need to workout the best option for a style guide there.

I'll add that I think one requirement is that we use a style guide that maintains an eslint file so we don't need to invest time writing it from scratch and always updating it with changes. There is enough maintenance already on the project.

[ilanbiala]

For everyone on this thread who is interested, #989 is all about style formatting. Feel free to suggest any changes, but the initial PR is just to get it added in.

[jloveland]

XO seems to simplify code style checking...https://github.com/sindresorhus/xo
WDYT?

[lirantal]

@jloveland how is it different?

[jloveland]

I haven't used XO yet, but it sounds like something of interest for us to consider...

From the XO readme:

No decision-making. No .eslintrc, .jshintrc, .jscsrc to manage. It just works!

Sounds like it can reduce some configs.

[ilanbiala]

@jloveland I looked at xo and it prefers a less strict configuration, which we kind of need when having so many contributors.

[lirantal]

Closing this one.