Saturday, October 3, 2015

code reviews - yAy! or nAy!

"Regardless of what we discover, we understand and truly believe that everyone did the best job they could, given what they knew at the time, their skills and abilities, 
the resources available, and the situation at hand."
--Norm Kerth, Project Retrospectives: A Handbook for Team Reviews

Disclaimer: I will try to document my observations. Now, be warned, I may be entirely wrong and there is a fair possibility that it might not make ANY SENSE AT ALL.
But I am open to a discussion.

Having worked in a few Agile/Kanban/Hybrid/<insert_any_fancy_software_development_methodology_jargon> projects over the past few years, I have always had mixed feelings towards ‘Code Review’.

- As much as I have found code reviews a useful exercise, I have felt annoyed when a completed feature sat in code review status just because the reviewer didn’t have the time.

- Of all the encounters, this one grabs the cake. In one of my projects, the usual ritual was to include a distribution list with every pull request.
Now, usually, nobody would bother except the team which had a direct impact. But once in a while people would pop in (not knowing the context) and raise mindnumbingly insane questions often leading others to digress resulting in a ever growing mail chain heading nowhere.
That was a royal pain in the you-know-where. I can’t stress this enough.

- There were instances, where I was offended reading the comments. This one is an example.

 Man! I felt so ashamed that I wanted to wipe out the trail. I wanted this piece of check-in to be removed from the code (I swear to God. I did think about rewriting Git history.
But better sense prevailed. ) Slowly, it dawned to me that, the reviewer was critiquing the code and not the person who wrote it.
The reviewer would have said the same thing, had someone from his team had done it.

Reviewers won’t say a word if the author and the reviewer worked in same teams - MYTH BUSTED. No! It isn’t the case.


@Path("/email")
@POST
public void sendMessage(@QueryParam ("validate") @DefaultValue("true") String validationRequired, String jsonInput) {
// TODO: Why are we parsing the input argument as a json string?  This should be accepting the parsed object directly as a parameter.
// There are plenty of examples of this elsewhere in the in the code!
}


Code review is not:
- A bug hunt - No matter how hard we try and how many reviewers we had -- it doesn’t guarantee a defect free code. And how do we fix the gap? Add enough unit/integration/system tests to verify if things are working as expected.
- To drag the developer through the mud - we (the team is) are not here to spew hatred/frame opinions. The thing that gets reviewed is the code, and not the developer.
It’s solely up to the developer to learn from his/her mistakes.
- Is not a safety net - It doesn’t necessarily guarantee that you always have a few people to cover up for your mistakes. The liberty of one last check before it reaches production.
At the end of the day, you are responsible for the feature.

Code review is (the way I see it):
- A forum to discuss design decisions, raise questions and get them answered.
- A forum where you make yourself publicly accountable. You write a word the whole world notices. Ok! I am exaggerating. But you get the point right. Ensure that you have enough info
before you offer your opinion. When in doubt DONT. It’s ok to not know things once.  :)
- Compliment, reinforce, share good practices.
- Detect any slightest deviation which when unnoticed might change the way things are developed.

What do I look for when I review the code. (Note: Truth be told, I haven’t checked for these things meticulously on all occasions.)

1. Does the code meet the functional requirement?
2. Are there tests for the written code?
3. Is there an impact to the existing system?
4. Is the code consistent? Naming packages, methods, variable names etc
5. Does the code handle exceptions properly? Return codes, error messages
6. Can this be reused?
7. Is the logic straightforward?
8. Readability and maintenance. Logical flow, comments
9. Is the code optimized?
10. Is the code thread safe? (wherever applicable)

How developers can help
1. Ensure that the feature meets the functional requirement. If it’s a bug, ensure that it’s fixed.
2. Ensure that you add a brief description of the change. It needn’t be full blown. An abridged version of release notes would be perfect.
3. The testing strategy you’d followed. If it’s an endpoint, the URLs with the request/response would help.
4. Raise pull requests frequently so that we don’t have too much to review.
5. Annotate the code properly. Leave enough comments so that it helps others who read the code.

Thoughts?

PS: I wrote this to educate the the junior developers in my team about code reviews. You might have heard the same thing in many different ways.  :) 

Tuesday, July 1, 2014

stand up, sit down - who cares

"We are AGILE. We aren't aversive to change. Instead, we welcome change with both hands."

"Look at this magic triangle. Pay attention to its edges and the ball within it. The moment you increase the size of the ball the triangle goes out of shape. Value, Cost, Quality are the edges and the scope is the ball."

"You don't do planning poker for estimation and you call yourself AGILE? What the BLEEPITY-BLEEP!"

Man! If I had a $ for every time I heard people say this, I would still be broke. :(

Stand-up meeting for starters is the one in which the team members gather around in circles and start telling each other about the past day's activities and what's in store for the day. Some lean back and start fiddling with their phones. That's a different story. ;-)

I've always looked at stand-up meetings as forums in which you(I) make yourself(myself) publicly accountable. At least to your team members, if not to a larger audience. It makes you push your limits to achieve something useful. Or, should I say tangible? Come on! Nobody likes to admit that they've been sitting on a issue for days together right? ;-)

If you look at the brighter side of things - You talk about issues, common problems, roadblocks and more often than not, a not-so-short explanation about how you had managed to solved a show stopper (KUDOS!) Eventually, you lose track of time. What was supposed to be a 15 minute meeting goes on and on for about 30-45 mins depending on the team size. It gets even worse with teams which are geographically separated.

"Mike, can you hear us? YES. NO. Now, it's better. We lost you again. I guess we are facing issues with the connection."
"We hear a lot of static. Can you please move away to a quieter place?"

Sky is the limit for the number of issues that we face during conference calls.

It is unfortunate and at times a little ironic - in a way that these theories carefully crafted to reduce the no. of meetings and mind numbing documentation ends up doing the same thing.

It makes me wonder. Are there any better alternatives?

What if we find a better way to collaborate? What if we don't do stand-ups and still call ourselves AGILE? Will that be a breach of any contract? As long as we manage to get the work done, who gives two hoots about how we call ourselves?

What's your take?

Disclaimer: I'm no saint. I do the same thing in stand-up meetings. At times, I tend to go overboard in an attempt to make everybody understand the concept so that we all are ON THE SAME PAGE.
I religiously follow this. I can't help it when I'm one of the participants. If I get a chance to host a meeting I make it a point to follow these rules.

Saturday, December 28, 2013

how to unit test javascript without setting your hair on fire?

This code looks familiar? Most of us, at some point in time would have written javascript this way to do a simple login validation. Right?


 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
function formSubmit(){
     var userName = $("#userName").val();
    var password = $("#password").val();

    if(!isValid(userName,password)){
    return false;
    }

    $.ajax({
        type: 'POST',
        url: "login",
        dataType: 'text',
        data : "username="+userName+"&password="+password,
        success: function (data) { 
            alert('Login Success');
        },
        error: function( xhr, err) {
            alert('Login Failed.'+ xhr.status);
        }
    });
}

The real problem comes when we try to write unit tests for this kind of javascript. The issue is the code which is completely mixed with HTML and inline event handlers.
So what? Big Deal. Why not write test cases to test HTML dependencies and DOM manipulations?
Yeah! You could. But you would end up writing ONLY test cases rather than writing actual functions.

How about we refactor the code so that it's testable?

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
function formSubmit()
{
    var cred = readValuesFromUI(); //separate out the function which reads values from ui.
    var val = validate(cred); // separate out validations.
    if(val.isValid)
    {
        doLogin(cred); // ajax call to server
    }
    else
    {
        alert('Invalid Credentials.');
    }
}

Jasmine Specs to the rescue
Jasmine - An automated test framework for Javascript.
Say you have a function which returns the sum of two numbers

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
function sum(num1, num2){
    return num1 + num2;
}

describe("this is a suite which tests the function sum with various inputs", function() {
    it("this is a spec which tests +ve scenario", function() {
    var s = sum(1,2);       
    expect(s).toEqual(3);
    });
});


'expect' is equivalent to 'assert' and 'toEqual' is the matcher.
Let's spice this up a little bit. Say, we validate the input params (num1 & num2) before returning the sum

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
function sum(num1, num2){
    var isValid = validateParams(num1,num2);
    if(isValid)
        return num1+ num2;
    else
        return -1;
}
function validateParams(num1,num2)
{
 // some logic
}

Now, what if the validateParams function fails when testing the sum function? We are not bothered about the functionality of the validateParams function when testing the sum function right? All we need is the function to return true/false based on then input so that the sum function can go ahead and do its job. So why not mock the function validateParams?

Jasmine provides a way to spy on functions. No matter how the validateParams function works, we would still be able to test the sum function.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
describe("this is a suite", function() {
    it("this is a spec where validateParams returns true", function() {
    var fakeValidateParams = jasmine.createSpy();
spyOn(window,'validateParams').andReturn(true);
    var s = sum(1,2);       
    expect(s).toEqual(3);
    });
    it("this is a spec where validateParams returns false", function() {
    var fakeValidateParams = jasmine.createSpy();
spyOn(window,'validateParams').andReturn(false);   
    var s = sum(1,2);       
    expect(s).toEqual(-1);
    });
});

This way, its easier to fake ajax calls, spy on library functions. I have put together a simple login app with bare minimum javascript. It's integrated with maven and the specs can be run as a part of your build without having to make use of the browser.

Jasmine, through the jasmine-jquery plugin also lets you use json fixtures to test functions which take inputs.

getJSONFixture(*.json) - loads the json data and makes it available for your specs. By default, fixtures are loaded from this location -

spec/javascripts/fixtures/json.

Jasmine has a lot more to offer, with support to fake events, fake timers etc. Do read the documentation.

Steps:

1. Clone the project from github.
2. Navigate to the path where you find pom.xml (pom.xml is configured with jasmine-maven-plugin and the saga-maven-plugin to generate coverage report)
3. Run "mvn clean test" to run the specs and "mvn clean verify" to run the specs & generate the coverage report.
4. Open the total-report.html and see the magic unfold in front your eyes ;-)

Jasmine specs run from the terminal


Coverage Report



To conclude :

Although, I ended up writing unit test cases for two whole modules, there was nothing conclusive about these tests. Yeah! Agreed that these tests help you test the flow. Which block of code gets called for what kind of input sort of way but that is all about it.
I had to do quite a bit of refactoring to make the already existing javascript functions testable.
Similar opinions expressed on this stackoverflow link as well.

Do let me know what you think.

PS: Title courtesy - This Post

Sunday, August 4, 2013

My tyrst with Atmosphere Framework.


Recently, I was asked to implement a feature which involved sending notifications from the server to the client about the state of a server resource (in my case Database). Till now, we were doing a poll once in every four seconds to get the job done. Before we started development, we outlined the set of requirements so that we could work towards them.

- Update all connected clients about the state of the server via Broadcast.
- Pick a framework which doesn’t involve any external server.
- Integrate seamlessly with existing application.
- Be easy to implement and be scalable.

Once we had the set of requirements clear, we embarked on the process of identifying the framework that would fit the bill. We (each one of us tried a poc with different technologies) evaluated Node.js, Vert.x, ActiveMq and Atmosphere.

We chose Atmosphere since it satisfied most of our requirements.
- Doesn't require any external server.
- Has a jersey module which integrates seamlessly with any webapp.
- Switches back to the fallback transport if the browser/server doesn’t support the specified transport.

POC

At a high level, the POC has the following components:
- Atmosphere's server side component which takes care of suspending the incoming request and broadcasting the data back to the clients when invoked.
- Atmosphere's client side component which is responsible to initiate a request and handle the response it receives via the broadcaster.
- A rest resource and a client(for testing purpose) which takes care of generating the content that needs to be broadcasted.
- BroadcastFilters which intercept the broadcast before it is being sent to the clients.
- Interceptors which intercept the incoming requests.
- EventsLogger which logs the Framework related events like onSuspend, onPresuspend, onReconnect, etc.

The POC, per say, is pretty straightforward. I have borrowed the concepts from several sample programs that the author himself has shared. What's more interesting is the number of problems that ran into during the process of developing the POC and testing it.

Issues that I faced:

- In our project we have tomcat servers which host the application and there is Apache Httpd 2.4 to take care of Authenication via ldap. Apache 2.4 doesn't support websockets by default so the request did not get through. I had to upgrade the server from 2.4 to 2.4.6 which has the module mod_proxy_wstunnel which supports websockets.

- After upgrading Apache, I had issues with IE8. IE8 doesn't support websockets and the framework downgrades the transport to long-polling (IE8 supports long-polling). In spite of this the request didn't get through. The client tries to establish a connection until it reaches the max no. of retries and eventually errors out.

- Assume that a client tries to subscribe for a topic to which he doesn't have access. So, I tried to return the response back to the client without suspending the request with this message – "Access restricted. You don’t have sufficient privileges to receive updates for this topic."

- I couldn't get the cache working. When the client loses connection to a server, the broadcasted messages go into the cache and when the client resumes connection, the messages get delivered to the client from the cache.

- Open 3 browsers Safari, Firefox and Chrome. Each has three tabs opened. Open the application and hit subscribe on the three tabs one after the other. You could see that the connection gets shared. Meaning, there is only one websocket connection that gets opened and it gets shared between tabs in each browsers. This is due to the shared attribute in the request which we set to true. Now, I run the Rest client which pushes the data to the application which in turn broadcasts it to the clients. Only the active tab receives the broadcasts. Not sure if it's the intended behaviour but it does happen. 

Please try this out and let me know if it works. :)

My Dev environment:
Ubuntu 12.04 and Windows 7
Browsers:
Firefox 22.0
Chrome Version 28.0.1500.95 m
Safari : 5.1.7 for Windows

Step 1: Deploy the application and open it in any of the browser.
Step 2: Choose a topic from the drop down. As of now, the FeedRestResourceClient publishes only content for the topic 'Computers & Technology'.
Step 3: Run the FeedRestResourceClient
Step 4: You'll see that the client has received the broadcast.




Reference Links:


~ cheers.!

Monday, June 10, 2013

just about a month in the new company..........

On May 15th, I had completed one month with Lister. I was going to write a post at that time but I hadn't done anything substantial apart from attending day long inductions and spending time getting to know things. Now that I've started checking in code on a regular basis, I feel now's the best time to share my experiences and to pause and reflect on my days so far.

I am not doing anything different.

To begin with, I am not doing anything different from what I've been doing in my previous companies [1, 2]. In my previous roles, I was more of an individual contributor who takes up a requirement, codes and commits the changes against the said tasks. Even now, I am doing the same thing but within the confines of a team.

The team here uses a number of open source tools and frameworks [1, 2, 3] and I get to play around with them. Learning my way out. This part of my job is the most exciting to say the least.

Small Company

Like I had mentioned here, it's a small company. Everybody knows everybody kind of an environment. Its been just over a month and within this span of time, I'd come to know about many teams (at least in my bay) where they'd studied, where they'd worked previously without me having to put any extra effort. Feels good to get to know people.

Commute is grueling

I spend 4 hours on a daily basis commuting. And by the time I reach home, I think about nothing but hitting the sack so that I can get some sleep before starting it all over again. But the good part – I am able to spend time reading books. I have read 2 books [1, 2] this month and I am gonna finish another one. If you look at it, it might appear as if I've been given the gift of time. :) Time almost comes to a standstill when I'm traveling. ;-)

To sum up

Apart from the work, I have been keeping myself busy reading stuff and trying out things that are out of my comfort zone and documenting things here. Things are pretty nascent at this stage and once they take shape, I'll share the more with you guys.
Over all, I wouldn't say that I am disappointed but at the same time, I have started worrying about this - What if I get a comfortable hold of the open source frameworks and tools I use? What next? Will it become monotonous? How am I going to keep myself engaged and psyched?

But at the moment all I can do is to hope for the best.

See you soon. :)

~ cheers.!

Monday, June 3, 2013

Shutting down Live Score Card Android App

Last year, out of boredom I wrote a Live Wallpaper (Android) which would display the live scorecard. I scrapped data from a very popular site.

Wait, allow me to explain. I'd no intentions to steal data. Or, I didn't have any intentions to create something like this.

In my opinion, I followed a proper way

- I tried contacting several teams (sites) asking if they offer any APIs. Many didn't respond and few replied stating that they offer only paid solutions which start at a minimum of 5K per month (* Refer attached mail chain).

- The other URLs that I got from Stackoverflow spoke nothing about restricted access (even their robots.txt said nothing). At the same time, the data that they provided weren't enough to build a fully featured app.

At that point, I thought, "Why let trivial things such as access and data bother app development? Let me finish building it and then I'll decide whether to continue it or not. Anyhow, I'm not going to make money."

All things said, the scraper was pretty light weight and given the scale of the website, a cron accessing the site and scraping a single page is no big deal (Technically, of course. ;-) ). So, I went ahead and scraped their URL. Apart from a handful of my friends who helped me out with testing app in their devices, I extensively used it for 3-4 months, totally forgot about it until recently when I was asked about the app in one of interviews :D

Now, out of sheer guilt I have disabled the api which pulled data from their site. :'(

Sorry guys.
Nothing personal.

Happy coding :)
~ cheers.!

---
* mail chain

---------- Forwarded message ----------
From: Pankaj Chhaparwal 
Date: Mon, Sep 3, 2012 at 5:51 PM
Subject: RE: RSS Feeds to get live scores
To: karthick r 

The smallest pkg we have is for rs 5000 per month. 

Regards,
Pankaj
 

From: karthick r [xxxx] 
Sent: Monday, September 03, 2012 5:35 PM
To: Pankaj Chhaparwal
Subject: Re: RSS Feeds to get live scores
 
Hi, 
Thanks for the prompt reply.
Please let me know the applicable rates. It'll help me decide. 

Regards,
Karthick.R

On Mon, Sep 3, 2012 at 5:14 PM, Pankaj Chhaparwal  wrote:

Hi Karthick, 
We can provide you this content, but we only offer a paid solution. 
Let me know if you would be interested. 

Regards,
Pankaj Chhaparwal



From: xx@gmail.com
Sent: Monday, September 03, 2012 2:36 PM
To: xxxxxxxxxx
Subject: RSS Feeds to get live scores
 
Hi, I am working on an android app to display live scores. Just want to know if you are providing any API to pull the match/score data. Once developed, this app will be available for others to download from Google Play for Free. Please let me know if you provide any such APIs. Thanks, Karthick Website: http://about.me/r.karthick 
Company: Developer
-- 
regards,
r.karthick