• Skip to main content
  • Skip to footer

InRhythm

Your partners in accelerated digital transformation

  • Who We Are
  • Our Work
  • Practices & Products
  • Learning & Growth
  • Culture & Careers
  • Blog
  • Contact Us

code

Jan 15 2019

How to Write a Good Pull Request

Pull Requests are the backbone of open source software development. They allow contributions by anyone, from anywhere. PRs are also a vital form of communication, even within a localized development team working on proprietary software. What makes a good Pull Request? Let’s break it down into 4 rules of thumb:

1. Provide context

2. Make it as small as possible, but not any smaller

3. Take screenshots

4. Ask for assistance

Provide context

Providing context is an important first step in guiding the reviewer. Use this as an opportunity to explain why you are making this change. This can be as simple as referring to the bug/defect/issue number, and as detailed as necessary to describe your change.

In this example, I provided a link to the original issue on the npm.community site and linked directly to the source of pnpm that was referenced in the original issue.

https://github.com/npm/cli/pull/32#issue-204418739

In this more complicated example, I wanted to make sure I was consistent with how other PRs were made to this repository. One of my suggestions is the old when in Rome rule. Some repositories even provide a template, which can be helpful, but this one didn’t. So, I made each item discussed in the issue into a bullet item, checked off the ones I completed and noted anything I wasn’t sure about.

https://github.com/npm/cli/pull/61#issue-211591081

Make it as small as possible, but not any smaller

Everyone agrees that smaller PRs are easier to review. Sometimes it’s just not possible to make a very small change, so here’s some practical advice: don’t do more than necessary. If you need to stray from the core of the change you are making, separate it. Here’s an example – imagine you are adding a path through the code to handle a new requirement. Along the way you realize that some of the variables and functions could use better names, but changing those means that you now need to update a bunch of files in another area of the code. STOP! Don’t make that change! Or at least, don’t bunch it together with your other changes. Instead, make the rename change in its own commit, and probably in its own Pull Request too. In the PR you can explain why you are making the change and how it simplifies the real change you want to make easier.

This same strategy should be applied to most whitespace and refactorings you want to do during the course of implementing a new feature or resolving a defect. Be considerate of the reviewer’s time. There is nothing more frustrating than hunting through all the changes looking for the actual change and bumping into whitespace and renamings spread across many files.

Take screenshots

So you’re working on a story that affects the UI? Maybe you are fixing alignment in IE11, or adding a new interstitial modal when the user clicks a button. The code will get reviewed as it always does, but many people (like me) struggle with visualizing layout changes or CSS tweaks. Include a screen capture of the before & after. It’s usually pretty easy to get a before shot – just use the QA or production environment. Then for the after shot, use your local server. Both GitHub and Atlassian BitBucket allow you to paste images, so you can literally SHIFT-CTRL-CMD-4 (OSX) to copy a section of the screen to your clipboard, then CMD-V to paste it into the input box of your PR description.

Another incredibly helpful option is to use an application like GIPHY Capture to record an animated GIF that can be added to your PR. These are great for when you want to show an animation or a sequence of steps. Let’s face it, it’s a pain for the reviewers to fire up their Windows vm to try out your change that resolves an Edge problem. Make their life easier by including an animated GIF that shows exactly what changed right in the Pull Request!

Ask for assistance

I am a big fan of the quote “it is better to beg for forgiveness rather than ask for permission”. In so many instances, not just in software, this rule helps save time. But making too big a change in your PR may be received poorly, especially when you are not a regular contributing member. I’m not advocating “playing dumb,” but being cautious and curious can help lead to better engagement and ultimately a better solution.

Here is a comment I made on a PR to the tslint project I made a few years ago. You can see how I acknowledge the person’s comment/feedback and ask a clarifying question because of the impact it would have on so many files. This lets the reviewers know that I have respect and consideration for the size changes coming into their code base, and also that I want to be collaborative in finding the best solution.

https://github.com/palantir/tslint/pull/1738#issuecomment-261527450

Conclusion

I am hopeful that these points help you make Pull Requests that are more quickly reviewed and accepted. What other things do you like to do in your PRs? What kinds of things would you like to see more of in PRs that you are reviewing? What would you like to see less of? Leave a comment below or hit us up @GetInRhythm on Twitter.

Written by InRhythm · Categorized: InRhythm News, InRhythmU, Learning and Development · Tagged: best practices, code, github, Programming, pull request

Jul 11 2016

Code Challenge Accepted (And Answered)

One of engineers challenged our entire team with the following task:

 

Code challenge (in js, obvi).
Write a one-liner that outputs (int) 10 using only the following symbols: “+”, “[” and “]”
Oh, and you have to explain your answer.
Extra credit: Same restrictions, output (string) 20.

Now if you want to play around a little bit and try and figure out the answer – feel free! Make sure to drop your answer or logic in the comments below. Let us know if you want to see more challenges on the blog as well.

 

But for those of you who want to dive right into it – here is our answer explained!

 

What happens when we perform a math operation on an empty array?

 

+[] // 0

Int 0. Int-eresting, it looks like javascript automatically type casts our array into an int.

 

Now, using some basic cs principles we can have some fun. Let’s try outputting int 1. We know that we can get 0 from +[] so let’s try to increment it

 

++[] // Invalid left-hand side expression

Well… that sucks. However, we’ve got another trick up our sleeve. We can shove our outputted 0 into an array, access index[0] and then increment that.

 

Huh?

Just watch, you’ll get it.

 

[+[]]
That’s our array[0], now let’s get to that precious 0 it’s keeping safe for us.

 

[+[]][0]
Bingo, but we can do better. We know +[] is zero, so we can access the 0 index using +[] instead

 

[+[]][+[]]
Bang, we’ve got our 0 again. But we’ve already had a 0, what’s the difference? I’ll tell you. Nothing… except that this 0 won’t throw an invalid left handed expression error when we try to increment it.

 

++[+[]][+[]]
And there’s our int 1.

 

Let’s try and get a string 20. Seems pretty easy. We can take our newly created int 1 and increment that to a 2 then just add a string 0. But how do we get a string 0. Simple. We just add two arrays together. That makes sense, right?

 

[]+[] // empty string

Now, what happens when you add a string and an int? It gets cast as a string.

 

[++[[++[+[]][+[]]+[]]][+[]]]+[+[]] === "20" // true
Have fun wrapping your head around that one.

 

barney-more-challenges

Written by inrhythmAdmin · Categorized: Software Engineering · Tagged: challenge, code, development, engineering, growth, JavaScript, puzzle, software

Aug 10 2015

Nothing is Impossible – Making the Switch from ES5 to ES6

I have been writing javascript for about a decade now. When I first saw ES6, it felt like JavaScript had been empowered with so many new functionalities, I just had to rewrite everything to ES6. Once I actually begun that process it was tricky in the existing code base and not possible to rewrite everything using pure ES6 functionalities. I took a step back and decided to start with a smaller subset of ES6 features – class, let/const, lexical scoping, and object. Here’s what I learned along the way:

I have been writing javascript for about a decade now. When I first saw ES6, it felt like JavaScript had been empowered with so many new functionalities, I just had to rewrite everything to ES6. Once I actually begun that process it was tricky in the existing code base and not possible to rewrite everything using pure ES6 functionalities. I took a step back and decided to start with a smaller subset of ES6 features – class, let/const, lexical scoping, and object. Here’s what I learned along the way:

Before getting started you need to prepare the development environment with tools to help write code is ES6, I used gulp (Grunt is also a good choice) to build files. Here’s some Gulp plugins you can install to get started with compiling ES6 to ES5 compatibility versions.

  • ESLint (gulp-eslint) – An open source project which helps validate your ES6 javascript files and also lets you configure rules in .eslintrc in order to target ES6 features as per your needs.
    • File: https://github.com/jskungfu/linters/blob/master/.eslintrc
    • Plugin: https://www.npmjs.com/package/gulp-eslint/
  • Babel transpiler (gulp-babel) – There many browsers which do not support ES6 so we need to transforms ES6 code back down to ES5.
    • Plugin: https://www.npmjs.com/package/gulp-babel
  • Gulp-concat – To concatenate javascript files into one file.

Here is a simple gulp file to help get started:

    
    var gulp = require('gulp');
    var babel = require('gulp-babel');
    var concat = require('gulp-concat');
    var eslint = require('gulp-eslint');
    //Register compilation task
    gulp.task('compile', function() {
        // place code for your default task here
        return gulp.src([
            'src/libs/file1.js',
            'src/libs/file2.js',
            'src/xxx.js',
            'src/yyyy.js'
        ])
        // eslint() attaches the lint output to the eslint property
        // of the file object so it can be used by other modules.
        .pipe(eslint({
            "useEslintrc": true
        }))
        //eslint.format() outputs the lint results to the console.
        // Alternatively use eslint.formatEach() (seeDocs).
        .pipe(eslint.format())
        .pipe(babel({
            "sourceMaps": false
        }))
        .pipe(concat('yourfilename-concat.js'))
        .pipe(gulp.dest('dist'));
    });
    //Register default task
    gulp.task('default', ['compile']);
    

Here are some of the things I learned during my rewriting process:

1. Class

Class was really easy to get started, constructor methods really stands out and it offers a much nicer, clearer syntax for creating objects and dealing with inheritance.

Here is a simple class example:

    
    class Meter {
        constructor(config) {
            this.count = config.count;
        }
        getCount() {
            return this.count;
        }
        static reload() {
            window.location.reload();
        }
    }
    

Interesting things for developers from ES5 world to note:

  1. We don’t have to write the function key word when starting new function
  2. No comma needed for adding a new function
  3. We can’t stick default properties to a class like we do with prototypical inheritance, you have to use this.xyz = ‘abc’ from the function to add a property to the instance.
  4. Static functions are not tied to an instance and you can not call it calling this.reload() instead you have to use a constructor to use this method Meter.reload().
  5. Class declarations are not hoisted

It takes some time to forget the practices that we have been following for years, but once you get hold of it, you will never want to go back again.

2. Variable declaration

Let and const are two new ways to declare variables and both of them are block scope.

2.1 Let

Let is similar to var but it is available to the current code block.

    
    function get() {
        if (condition) {
            let value = "xyz";
            return value;
        } else {
            //value does not exist here
            return null;
        }
    }
    

2.2 Const

    
    if (condition) {
        const MAX_COUNT = 10;
        //more code which uses variable
    }
    //MAX_COUNT is not accessible here
    

In the code above MAX_COUNT is declared within the if statement and MAX_COUNT is only available within the block. As soon as the block statement is finished executing MAX_COUNT is destroyed.

Some important facts to notice here:

  1. We must provide a value when we declare constant.
  2. If we try to assign a value to MAX_COUNT again, the line will throw error.

Interesting things for developers from ES5 world to note:

  1. Let and const are not hoisted to functions or block scope – they are not even hoisted within their own block scope.
  2. We must provide a value to the const variable and if you try to reassign a value to a const variable then it will throw an error.

3. Arrow function

This is one of my favorite feature of ES6, functions defined with a new syntax that uses an arrow(=>) and it works as below:

  • Lexical this binding – The value of this inside of the function is determined by where the arrow function is defined not where it is going to be used.
  • Can not be used with new keyword – it does not provide a new keyword
  • Scope remains constant – this value can not be changed inside the function, it maintains the same value throughout the life cycle of the function.
  • An arguments object is not available – you must pass named arguments.

Examples:

    
    var getName = () => "Hetal";
    // will be transpiled to
    var getName = function () {
        return "Hetal";
    };
    

Lexical this binding

This is the most common use case where we had to come up with storing scope in ES5 world and it has been made easy in ES6 with arrow function.

ES5 you would probably write some function like as below:

    
    var Badge = {
        userName: "Hetal",
        getBadge: function () {
            return "<div class='badge'>" + this. userName + "</div>";
        },
        init: function (type) {
            var self = this;
            //notice how difficult it is maintain scope here
            $(document).on("click", function () {
                self.getBadge.call(self, e);
            });
        }
    };
    

ES6 will make same code easier to write

    
    var Badge = {
        userName: "Hetal",
        getBadge: function () {
            return "<div class=‘badge’>" + this. userName + "</div>";
        },
        init: function (type) {
            //using lexical scope,
            //we can write same code in one line as below
            $(document).on(“click”, (e) => { this.getBadge(e)});
        }
    };
    

This is great way to reduce code lines and write clean code.

 

File Size comparison:

The transpiling process adds some extra code to make it compatible with all browsers which increases the size of the final output file. For instance the file (minified and concatenated versions) that I rewrote is 25KB in ES5 javascript but the ES6 version comes out to be 32KB.

To conclude, Initially I thought that it would be really difficult to convert my project into ES6 but once I started taking baby steps by picking some ES6 features, configuring workflow process using gulp, it all eventually fell in place. I really enjoyed rewriting my ES5 javascript project files into ES6 – which also helped me better organize and structure my codebase and opened my mind to a whole new world. I hope what I learned will help someone who has just started converting projects into ES6. Happy coding!

References:
https://leanpub.com/understandinges6/read

Written by Hetal Thakkar

Written by inrhythmAdmin · Categorized: Software Engineering · Tagged: code, ES6, Node.js, tutorial

Footer

Interested in learning more?
Connect with Us
InRhythm

140 Broadway
Suite 2270
New York, NY 10005

1 800 683 7813
get@inrhythm.com

Copyright © 2022 · InRhythm on Genesis Framework · WordPress · Log in

This website uses cookies to improve your experience. We'll assume you're ok with this, but you can opt-out if you wish. Cookie settingsACCEPT
Privacy & Cookies Policy

Privacy Overview

This website uses cookies to improve your experience while you navigate through the website. Out of these cookies, the cookies that are categorized as necessary are stored on your browser as they are essential for the working of basic functionalities of the website. We also use third-party cookies that help us analyze and understand how you use this website. These cookies will be stored in your browser only with your consent. You also have the option to opt-out of these cookies. But opting out of some of these cookies may have an effect on your browsing experience.
Necessary
Always Enabled
Necessary cookies are absolutely essential for the website to function properly. This category only includes cookies that ensures basic functionalities and security features of the website. These cookies do not store any personal information.
Non-necessary
Any cookies that may not be particularly necessary for the website to function and is used specifically to collect user personal data via analytics, ads, other embedded contents are termed as non-necessary cookies. It is mandatory to procure user consent prior to running these cookies on your website.
SAVE & ACCEPT