Code Review tips

v\:* {behavior:url(#default#VML);}
o\:* {behavior:url(#default#VML);}
w\:* {behavior:url(#default#VML);}
.shape {behavior:url(#default#VML);}

Normal
0
false

false
false
false

EN-US
X-NONE
X-NONE

/* Style Definitions */
table.MsoNormalTable
{mso-style-name:”Table Normal”;
mso-tstyle-rowband-size:0;
mso-tstyle-colband-size:0;
mso-style-noshow:yes;
mso-style-priority:99;
mso-style-parent:””;
mso-padding-alt:0cm 5.4pt 0cm 5.4pt;
mso-para-margin-top:0cm;
mso-para-margin-right:0cm;
mso-para-margin-bottom:8.0pt;
mso-para-margin-left:0cm;
line-height:107%;
mso-pagination:widow-orphan;
font-size:11.0pt;
font-family:”Calibri”,”sans-serif”;
mso-ascii-font-family:Calibri;
mso-ascii-theme-font:minor-latin;
mso-hansi-font-family:Calibri;
mso-hansi-theme-font:minor-latin;
mso-ansi-language:EN-US;
mso-fareast-language:EN-US;}

Code Review, Discussions and Sharing

Top of Form

Bottom of Form

Upsource is great place for your team to communicate with the intent of improving your code base:

Discussions

You can start discussions in code or diff views, with or without creating a formal code review.

When viewing any revision of a file, you have access to all relevant discussions from prior revisions of this file, be it within code reviews or in standalone discussions that weren’t parts of formal code reviews.

Essentially, knowledge about specific files in your code base is accumulated and made available for future reference.

Code reviews

Anyone can request or start post-commit code review on any revision or branch.

If you prefer to review individual revisions, feel free to create code reviews from the revision list.

If you use the increasingly popular workflow of reviewing entire branches, then as soon as you click Create branch review on a branch, Upsource puts all revisions in the branch under a single code review, and makes sure to automatically add any new revisions as they appear. Branch tracking stops when a code review is closed and resumes if it’s reopened later.

The code review process in Upsource is relaxed, and it doesn’t impose any strict workflow:

  • If a developer who made a change wants their change to be reviewed, that’s fine.
  • If someone else on the team wants to raise a concern over a teammate’s change, they can perfectly do that by starting a code review, too.

When starting a code review, you can add one or multiple reviewers, picking the teammate(s) most proficient in the part of code that you’re modifying. Upsource can suggest reviewers to you based on history of files that are being changed, as well code review history.

Reviewers are expected to examine your changes, discuss them if they aren’t clear, and finally accept the changes.

You can also add teammates as watchers in code reviews so that they become aware of important changes in your code base. Watchers aren’t expected to take part in the code review process, but they are kept updated of your project’s status whenever an important change is made.

Developers taking part in a code review can discuss specific lines in the diff view, or add general comments to the review as a whole.

You can add new revisions to a code review if a revision originally submitted for review required further changes stemming from the discussion. Upsource will add new revisions automatically if the subject of your review is the entire branch.

Upsource tracks which revisions were already seen and approved. When new revisions are added to the code review, a reviewer only sees the diff of those revisions, and they don’t have to inspect previous changes once again. To show or hide any revisions within a code review, there’s a revision selector that quickly toggles revisions on and off:

You can access code reviews that you are involved in as an author, reviewer or watcher from your project’s home view. Alternatively, browse all code reviews in the project, or search code reviews by author, reviewer, state, or commit ID:

You can monitor status updates in a project via personalized e-mail notifications. Alternatively, you can track the News feed in the project’s home view, which displays updates such as new comments addressing you, and recently opened and closed code reviews.

If you want to get a high-level overview of code review activities in your project, there’s a set of code review reports that show the share of revisions that are covered with code reviews, how many open and closed code reviews you have in your project, and which project developers are most involved in the code review process.

Code review statistics in Upsource project

Code review in the IDE

We know that most developers are best seated in their IDE and prefer not to switch between tools unless absolutely necessary. Knowing this, we offer a code review plug-in for IntelliJ IDEA and other JetBrains IDEs which allows you to participate in code discussions and manage code reviews from the comfort of your IDE.

The plug-in allows viewing and creating code review comments right from the text editor, and provides a Review tool window that lists code reviews in the current Upsource project and lets you manage them.

Code review plug-in for IntelliJ IDEA: Reviews tool window

In addition, the plug-in integrates into the IDE’s own controls such as Version Control tool window and Commit Changes dialog box.

Upsource integration in IntelliJ IDEA's Git log view

The plug-in works across all IDEs based on the IntelliJ platform, namely:

Code sharing

In Upsource, everything has a URL and can be shared with teammates. This includes code reviews, revision diffs, discussions on code and on revisions, reports or filters applied to commit graphs (for example, all commits by developer X in time span Y).

You can even share custom selections of code in any specific revision:

Share a link to a random selection of code

When you share a URL that Upsource generates for a selection, anyone you share it with can open it and have the selection highlighted like this:

Link to selection as the receiving person sees it

 


 

In addition to being a code review tool, Upsource is also a repository browser. Learn how you can use Upsource to browse and monitor changes in code repositories based on Git, Mercurial, Subversion and/or Perforce.

If you’re a Java developer, you might also be interested to know about additional code insight features in Java projects including code inspections and advanced navigation.


 

Upsource Docs & Demos

Check out Upsource online help to:

If you have a question on using the product that you can’t find an answer to, feel free to contact the Upsource team, and we’ll try our best to help you out.

 

Videos

Watch videos demonstrating how Upsource can be used as a repository browser and code review tool.

Hub, YouTrack and Upsource Integration

Hub, YouTrack and Upsource Integration

12 August 2015

Code review workflow in Upsource

Code review workflow in Upsource

9 June 2015

Upsource plugin for IntelliJ IDEA

Upsource plugin for IntelliJ IDEA

1 June 2015

Upsource plugin for JetBrains IDEs

Upsource plugin for JetBrains IDEs

1 June 2015

JetBrains Upsource Overview

JetBrains Upsource Overview

20 May 2015

What's New in Upsource 2.0

What’s New in Upsource 2.0

20 May 2015

 

View all videos about Upsource and other JetBrains team collaboration tools.


 

Things to check before starting up

Before you proceed to install Upsource, please make sure to…

Check your team’s demands and expectations

  • Upsource is the right tool for you if your development team is looking for ways to browse different revisions of the code base without checking them out to local machines, and to discuss and review changes made in the code base.
  • Upsource only recognises changes that are committed to your repository.
  • Upsource doesn’t support Android Studio projects

Check your hardware and software

  1. Since Upsource is an on-premises application, you should have a server to deploy Upsource to, and the server should:
    • Have 8 GB of RAM or more
    • Run one of the following 64-bit operating systems:
      • Windows Vista or later
      • Mac OS X 10.7 or later
      • Linux (based on our knowledge, any 64-bit distribution should do)
  2. As Upsource is a set of Java applications, it requires Java runtime to be installed. Upsource bundles JDK 1.8 for Windows and Mac OS X and you don’t have to explicitly install JDK if you’re going to run Upsource on one of these operating systems. However if you’re going to install Upsource on Linux, you should first install JRE 1.8 or JDK 1.8 for Linux.
  3. Your fully qualified hostname should be resolvable to your IP address. To check it:
    • On Linux or Mac OS, run the following command:

ping $(hostname -f)

    • On Windows, to get fully qualified hostname, run:

ipconfig /all

then try to ping that name (it should look like mymachine.mydomain.com)

  1. Your development team should use Git, Mercurial, Subversion, or Perforce for version control.
  2. Upsource users should use a modern web browser. Upsource supports Internet Explorer 10+ and recent versions of Firefox, Chrome and Opera.

Things to configure before starting Upsource

When it comes to mainstream installation cases Upsource requires minimal configuring. All you have to do is:

Disable conflicting software (Windows)

If you run Upsource on Windows, please disable all anti-viruses, Windows Defender, and Windows Search service for Upsource root directory, as they may conflict with Upsource processes.

Adjust resource limits (Linux)

If you run Upsource on Linux, insufficient resource limits may result in a number of errors. To prevent that, we recommend to set:

  • maximum open files to 100000
  • memory locking and address space limit to unlimited
  • number of processes to 32768

You can do it by adding the following lines to the /etc/security/limits.conf file:

* – memlock unlimited

* – nofile 100000

* – nproc 32768

* – as unlimited

Other installation options

You can also:

You are now ready to launch Upsource

As soon as you have made all configuration changes that are relevant in your environment, you can proceed to start Upsource.

Proxy configuration

Note: <upsource_home>\directory_name should be read as “open the console and change directory to directory_name under Upsource home directory.”

All commands listed below are Windows commands. If you’re working on a Linux or Mac OS X server, simply replace .bat with .sh.

You can set up Upsource to work behind a reverse proxy server. There are two requirements that your environment should meet to make this possible:

  • Your proxy server must support WebSockets. For example, Nginx supports WebSockets since version 1.3.13.
  • Upsource should be hosted under root URL (/) on your virtual host.

If these requirements are met, start with configuring Upsource to use a base URL (the URL that end users will request for to access your Upsource installation):

<upsource_home>\bin\upsource.bat configure –listen-port 1111 –base-url http://upsource.mydomain.com:2222

where:

  • 1111 is the port number Upsource will listen to
  • http://upsource.mydomain.com is the address of your proxy server
  • and 2222 is the port number your proxy will listen to

Now configure headers in your proxy server, and you’re done. Configuration guidelines for Nginx and Apache HTTP Server are provided below.

Nginx configuration

To ensure support for WebSockets, please use Nginx 1.3.13 or later.

Here’s a sample Nginx header configuration (non SSL):

server {

      listen       2222;

      server_name  localhost;

location  / {

      proxy_set_header X-Forwarded-Host $http_host;

      proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;

      proxy_set_header X-Forwarded-Proto $scheme;

      proxy_http_version 1.1;

       

      # to proxy WebSockets in nginx

      proxy_set_header Upgrade $http_upgrade;

      proxy_set_header Connection “upgrade”;

      proxy_pass http://upsourcemachine.domain.local:1111/;

      }

}

where:

  • listen 2222 is the port that you have previously specified as a –base-url parameter
  • proxy_pass http://upsourcemachine.domain.local:1111/ is the path to your Upsource machine with the port that you have previously specified using the -–listen-port command

Nginx SSL configuration goes as follows:

  • Configure base url:

<upsource_home>\bin\upsource.bat configure –listen-port 1111 –base-url https://upsource.mydomain.com/

  • Nginx configuration file:

        server {

            listen 443 ssl;

   

             ssl_certificate <path_to_certificate>

             ssl_certificate_key <path_to_key>

   

            server_name  localhost;

   

        location  / {

                proxy_set_header X-Forwarded-Host $http_host;

                proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;

                proxy_set_header X-Forwarded-Proto $scheme;

                proxy_http_version 1.1;

   

                # to proxy WebSockets in nginx

                proxy_set_header Upgrade $http_upgrade;

                proxy_set_header Connection “upgrade”;

                proxy_pass http://upsourcemachine.domain.local:1111/;

                        }

               }

Note: Please refer to the corresponding Nginx documentation pages for a description of server_name, proxy_set_header, proxy_pass.

Apache HTTP server configuration

To ensure support for WebSockets, please use Apache HTTP Server 2.4.10 or later.

Make sure to enable proxy_wstunnel, proxy_http, rewrite modules (and optionally headers if you want to use SSL) using the a2enmod script:

    $ a2enmod headers

    $ a2enmod rewrite

    $ a2enmod proxy_wstunnel

    $ a2enmod proxy_http

Add the following directives to the VirtualHost section of a relevant .conf file:

    RewriteEngine on

    AllowEncodedSlashes on

      

    RewriteCond %{QUERY_STRING} transport=polling

    RewriteRule /(.*)$ http://127.0.0.1:1111/$1 [P]

   

    ProxyRequests off

    ProxyPass /~socket.io/ ws://127.0.0.1:1111/~socket.io/

    ProxyPassReverse /~socket.io/ ws://127.0.0.1:1111/~socket.io/

       

    ProxyPass / http://127.0.0.1:1111/

    ProxyPassReverse / http://127.0.0.1:1111/

where 1111 is the port number you configured Upsource to listen to.

If you want to use SSL, additionally add the following directives to the VirtualHost section:

  RequestHeader set X-Forwarded-Proto “https”

IIS reverse proxy

To use IIS and ARR as a reverse proxy:

  1. Install ARR from here
  2. In IIS Manager, connect to the IIS server – in this case, localhost
  3. Highlight the server in the Connections pane
  4. Double-click URL Rewrite
  5. Click View server variables on the right pane
  6. Add HTTP_X_FORWARDED_HOST, HTTP_X_FORWARDED_SCHEME and HTTP_X_FORWARDED_PROTO to the list
  7. Highlight the server in the Connections pane
  8. Double-click Application Request Routing Cache
  9. Click Server Proxy Settings under the Proxy heading in the Actions pane.
  10. Tick the Enable proxy checkbox and then click Apply. Leave the default values in place.
  11. In the Connections pane, under Sites, highlight Default Web Site
  12. Double-click the URL Rewrite feature, and click Add Rule(s)… in the Actions pane
  13. Add a reverse proxy rule, with server name: localhost:1111 (replace with real location and port of your Upsource service)
  14. Open created rule, check rewrite url, add server variables:
    • set HTTP_X_FORWARDED_HOST to {HTTP_HOST}
    • set HTTP_X_FORWARDED_SCHEME to https (if the IIS site is configured to https, else set to http)
    • set HTTP_X_FORWARDED_PROTO to https (if the IIS site is configured to https, else set to http)
  15. Make sure that Anonymous Authentication is enabled:

a) In the Connections pane, under Sites, highlight Default Web Site

b) Double click Authentication → Select Anonymous → Click Enable in the right pane.

In-cloud installation

Upsource can be easily installed and run in a cloud, using one of the cloud computing services available on the market.

As an example we’ll outline the Upsource installation requirements for Amazon EC2, since it’s one of the most popular cloud solutions at the moment. For easy service setup and trouble-free Upsource installation we encourage you to read EC2 documentation as well.

  1. You can choose any platform: Linux, Mac, or Windows.
  2. The EC2 instance type you select should be M3.large or higher.
  3. When configuring EC2 security group, add a custom rule with the following parameters:
    • Protocol to allow: TCP
    • Port to allow: XXXX (Specify the default Upsource port (80 for Windows, 8080 for Linux and Mac), unless it’s taken or you want to use a different port for other reasons. In this case you’ll have to change it for Upsource as well (Step 4).
  4. Install Upsource, then prior to starting it, configure the base URL (the URL for end users to access your Upsource installation) and port by running the following command:

<upsource_home>\bin\upsource.bat configure –listen-port XXXX –base-url http://upsource.mydomain.com/

where:

    • –listen-port should match the port that you’ve specified for EC2 rule in Step 3. Skip, if you’ve specified the default Upsource port in Step 3.
    • –base-url should match the public DNS name of your EC2 instance.

Note: <upsource_home>\directory_name should be read as “open the console and change directory to directory_name under Upsource home directory.” Above is a Windows command. For Linux or Mac OS X, simply replace .bat with .sh.

  1. You are ready to run Upsource.

Installing Upsource on Linux

To help you install Upsource quickly and easily, let’s take a common Ubuntu installation as an example and go through the entire process.

First of all we need to install Java

1.     Unless you already have one, download JRE 1.8 installation package from the Oracle web-site.

2.     Launch the terminal and create a folder where java binaries will be stored:

sudo mkdir –p /usr/local/java

3.     Go to the folder with the downloaded jre archive (the default location is shown here):

cd /home/user_name/downloads

4.     Copy the archive to your java folder:

sudo cp –r jre-7u25-linux-x64.tar.gz /usr/local/java

5.     Go to the java folder:

cd /usr/local/java

To execute our archive, we need to set permissions.

1.     Change the permissions:

sudo chmod a+x jre.tar.gz

2.     Unpack the archive:

sudo tar xvzf jre.tar.gz

Now we are going to set system variables.

1.     Edit /etc/profile:

sudo gedit /etc/profile

and add the following lines to the bottom of the file:

JRE_HOME=/usr/local/java/jre1.8.0_25 
PATH=$PATH:$JRE_HOME/bin 
export JRE_HOME 
export PATH

Save the file and close it.

2.     Tell your system where JRE is located:

sudo update-alternatives –install “/usr/bin/java” “java” “/usr/local/java/jre1.8.0_25” 1

3.     Make the JRE the default one:

sudo update-alternatives –set java /usr/local/java/jre1.8.0_25/bin/java

4.     Check if java was installed correctly:

sh java -version

Next step is to adjust your system resource limits

Add the following lines to the /etc/security/limits.conf file:

* - memlock unlimited
* - nofile 100000
* - nproc 32768
* - as unlimited

Find a home for your Upsource dist

1.     Download a new Upsource build from our website.

2.     Create a folder for it. We’ll refer to this folder as Upsource home directory:

Sudo mkdir –p /opt/Upsource

3.     Go to the folder with the downloaded Upsource archive (the default location is shown here):

cd /home/user_name/downloads

4.     Copy the archive to your Upsource folder:

cp Upsource.zip /opt

5.     Set permissions:

sudo chmod a+x Upsource.zip

6.     Unpack the archive:

sudo unzip Upsource.zip

7.     Make the Upsource folder writable:

sudo chmod -R a+rwX /opt/Upsource

Now we can start and configure Upsource.

1.     Launch the terminal and go to the bin folder in the Upsource home directory:

cd /opt/Upsource/bin

2.     Run Upsource:

upsource.sh start

When you run Upsource for the first time, it will open Configuration Wizard in your default browser, where you can specify initial settings.

That’s it. As soon as you’re finished, you’ll be taken to Upsource welcome page from where you can proceed to creating your first project.

Starting and stopping Upsource

Note: All commands listed below are Windows commands. If you’re working on a Linux or Mac OS X server, simply replace .bat with .sh. For Mac OS X or Linux, please make sure to start Upsource as a non-root user.

Running Upsource as a background process

To start Upsource, run the following command:

<upsource_home>\bin\upsource.bat start

Running Upsource as a Windows Service

To start Upsource, run the following command:

<upsource_home>\bin\upsource.bat service install /runAsSystem

To start Upsource with another user account, run the following command:

<upsource_home>\bin\upsource.bat service install /user=<user> [/domain=<domain>] /password=<password>

Stopping

To stop Upsource, run the following command:

<upsource_home>\bin\upsource.bat stop

Restarting

To restart Upsource, run the following command:

<upsource_home>\bin\upsource.bat restart

Displaying other commands

To explore other commands that Upsource provides, such as those for running in the current console, configuring, and displaying status, run

<upsource_home>\bin\upsource.bat ?

Things to configure after the first start

When you run Upsource for the first time, it will open Configuration Wizard in your default browser. First you can specify System settings

https://www.jetbrains.com/upsource/help/2.0/assets/setup/upsource_setup1.png

  • Base URL – you might want to change the default value if:
    • your server has several DNS interfaces and you wish to make Upsource accessible by a particular DNS interface;
    • you have set up Upsource to work behind reverse or terminating HTTP proxy (see Proxy configuration) – in this case the new value should match the base URL you’ve set before.
  • Application Listen Port – you can change the default port that Upsource will use for HTTP communications if the suggested port is or expected to be taken.
  • Location of system directories (Under Advanced Settings)

Click Next to proceed to User management settings

https://www.jetbrains.com/upsource/help/2.0/assets/setup/upsource_setup2.png

Here you have to choose to set up Upsource with either build-in Hub or external (standalone) Hub. If you are going to integrate with the YouTrack issue tracker, select External Hub. Otherwise you may choose Build-in Hub.

Please note: Prior to selecting External Hub, you should first install and start your external Hub service.

If Build-in Hub selected:

  • Create admin login and password – specify login name and password for your administrator account. (You’ll be able to reset your password if you ever forget it.)
  • Send usage statistics anonymously – keep this option checked to help us make Upsource better. We never share collected data with any third party.

If External Hub selected:

  • Enter your Hub URL and verify the connection.

Click Next to proceed to Licenses where you can keep the default free license or change it to a different one:

https://www.jetbrains.com/upsource/help/2.0/assets/setup/upsource_setup3.png

Click Finish when you’re done.

Upsource will open its welcome page (http://your-host-name:port_for_upsource/bundle/starting/) which will also be opened upon any consequent startup.


 

What to look for in a Code Review: Upsource Quick Wins

Posted on by Trisha Gee

We’ve had two themes running through the articles on what to look for in a code review:

  1. Lots of what we’re tempted to look for can (and should) be automated
  2. Humans are good at checking things computers can not.

In this final post in the series, we’re going to look at the grey area in between – we’re going to look at how the features in Upsource can make your job as a human code reviewer easier.

As a developer, code reviews can sometimes be a frustrating process – you’re used to all the power of your IDE, yet when you open up a set of changes in your code review tool you can’t leverage any of that. The cognitive load of having to navigate through the code in an unfamiliar way and losing all the context your IDE provides is one of the things that makes developers less keen to perform code reviews.

Let’s look at some of the features that Upsource provides that overcome these issues.

Navigation

It might seem like a trivial thing, but the ability to navigate through the code via the code is something we simply take for granted when we use an IDE like IntelliJ IDEA. Yet the simple Cmd + click feature to navigate to a given class, or to find the declaration of a field, is often missing when we open code in a tool that is not our IDE. Upsource lets you click on a class name, method, field or variable to navigate to the declaration.

Symbol actions

While this is very useful, something I find myself doing a even more in my IDE is pressing Alt + F7 to find usages of a class, method or field. This is especially useful during code review, because if a method or class has been changed in some way, you as the reviewer want to see what the impact of this change is – which means locating all the places it’s used. You can see from the screenshot above that this is easily done in Upsource – clicking on a symbol gives you the option to highlight the usages in the file, or to find usages in the project.

Inspections

Intuitive navigation is great for a reviewer as it lets you browse through the code in a way that’s natural for you, rather than having some arbitrary order imposed on you – it makes it easier to see the context of the changes under review.

But there’s another IDE feature that would be extremely useful during code review – inspections. If you’re already using IntelliJ IDEA, for example, you’re probably used to the IDE giving you pointers on where the code could be simpler, clearer, and generally a bit better. If your code review tool offered the same kind of advice, you could easily check that all new/updated code doesn’t introduce new obvious issues, and possibly even cleans up long-standing problems.

Upsource uses the IntelliJ IDEA inspections – we actually covered how to enable them for Upsource in the last post. There are rather a lot of inspections available in IntelliJ IDEA, so we’re just going to give a taste of what’s possible – we’re going to cover some of the default ones that you may find useful during code review.

Exception Handling Issues

Inspections can catch potential problems around how error conditions are handled. For example, empty catch blocks.

Empty catch block

Solutions
It’s difficult to think of a time when catching and ignoring an Exception is the right thing to do. A code reviewer should be suggesting:

  1. Catching the Exception and wrapping it in a more appropriate one, possibly a RuntimeException, that can be handled at the right level.
  2. Logging the Exception (we also touched on appropriate logging in the last post).
  3. At the very least, documenting why this is OK. If there’s a comment in the catch block, it’s no longer flagged by the inspection.

ExpectedException

Configuring
“Empty ‘catch’ block” is enabled in the default set of inspections.  This and other related inspections can be found in IntelliJ IDEA’s inspections settings under Java > Error Handling.

Probable Bugs

There are a number of inspections available for “probable bugs”. These inspections highlight things that the compiler allows, as they’re valid syntax, but are probably not what the author intended.

String format potential bug

Examples

  • String.format() issues like the one above.
  • Comparing Strings using == not .equals().
  • Querying Collections before putting anything in them (or vice versa).
  • Accessing Collections as if they have elements of a different type (sadly possible due to the way Java implemented generics on collections).

Collections probable bugs

Solution
Not all of these problems are automatically bugs, but they do look suspicious. They’ll usually require you, the code reviewer, to point them out to the author and have a discussion about whether this code is intentional.

Configuring
Inspections to highlight all the potential problems listed are already selected by default. To find more inspections in this category, look under Java > Probable Bugs in the inspections settings.

Code can be simplified

It’s easy as you evolve code to end up with statements and methods that are more complicated than they need to be – it just takes one more bit of boolean logic or an additional if statement. As code reviewers, we’re in a fortunate position of being one step back from the coal-face of the code, so we can call out areas ripe for simplification. Fortunately, we don’t have to do this alone – Upsource shows us some of these things automatically.

Boolean expression can be simplified

Examples

  • Using explicit true and false in a boolean expression (in the example above this is unnecessarily verbose).
  • Boolean expressions that can be simplified, or re-phrased to be simpler to understand.
  • if or while expressions that always evaluate to the same value:

Condition can be simplified

Solutions

  • As with the other examples above, you may simply want to flag them in the code review so the author can use IntelliJ IDEA’s inspections to apply the recommended fix.
  • In some cases, like if statements that can be simplified in equals() methods, the simplified code is not always easier to read. If this is the case, you may want to suggest the code author suppresses the inspection for this code so it is no longer flagged.
  • In other cases, the inspection might be pointing to a different smell.  In the if statement above, the inspection shows this code (which is in a private class) is always called with a particular set of values so this if statement is redundant. It may be viable to remove the statement, but as this specific example is only used in test code it implies there’s a missing test to show what happens when the two objects are equal. The code reviewer should suggest the additional test, or at least have the author document why it wasn’t needed.

Configuring
These types of inspections can be found in Java > Control flow issues and Java > Data flow issues.

Unused Code

Upsource highlights all unused code (classes, methods, fields, parameters, variables) in a grey colour, so you don’t even need to click or hover over the areas to figure out what’s wrong with it – grey should automatically be a smell to a code reviewer.

Unused code

Examples
There are a number of reasons a code review might contain unused code:

  1. It’s an existing class/method/field/variable that has been unused for some time.
  2. It’s an existing class/method/field/variable that is now unused due to the changes introduced in the code review.
  3. It’s new / changed code that is not currently being called from anywhere.

Solutions
As a reviewer, you can check which category the code falls into and suggest steps to take:

  • Delete the unused code. In the case of 1) or 2) above, this should usually be safe at the field/variable level, or private classes and methods. At the class and method level, if these are public they might be used by code outside your project. If you have control over all the code that would call these and you know the code is genuinely unused, you can safely remove them. In case 3) above, it’s possible that some code is work-in-progress, or that the author changed direction during development and needs to clean up left over code – either way, flag the code and check if it can be deleted.
  • Unused code could be a sign the author forgot to wire up some dependencies or call the new features from the appropriate place. If this is the case, the code author will need to fix the unused code by, well, using it.
  • If the code is not safe to delete and is not ready to be used, then “unused code” is at the very least telling you that your test coverage is not sufficient. Methods and classes that are used by other systems, or will be used in the very near future, should have tests that show their expected behaviour. Granted, test coverage can hide genuinely unused code, but it’s better to have code that looks used because it’s tested than have code that is used that is not tested. As the reviewer, you need to flag the lack of tests. For code that existed before this code review, you might want to raise a task/story to create tests for the code rather than to slow down the current feature/bug being worked on with unrelated work. If the unused code is new code, then you can suggest suitable tests. New code that’s untested should not be let off lightly.
  • If you and the code author decide not to address the unused code immediately by deleting it, using it or writing tests for it, then at least document somehow why this code is unused. If there’s a ticket/issue somewhere to address it later, refer to that.

Suppress unused warnings

Gotchas
Inspections are not infallible, hence why they’re useful pointers for reviewers but not a fully automated check with a yes/no answer. Code might be incorrectly flagged as unused if:

  • It’s used via reflection
  • It’s used magically by a framework or code generation
  • You’re writing library code or APIs that are used by other systems

Configuring
These types of inspections can be found in Java > Declaration redundancyJava > Imports and Java > Probable bugs. Or you can search for the string “unused” in the IntelliJ IDEA inspection settings.

And to make it even easier…

The navigation and inspection features are all available in the Upsource application. While it would be great if the app could provide everything we as developers want, sometimes we just feel more comfortable in the IDE.  So that’s why there’s also an Upsource plugin for IntelliJ IDEA and other JetBrains IDEs, so we can do the whole code review from within our IDE.  There’s also a new Open in IDE feature in Upsource 2.5 which, well, lets you open a code review in your IDE.

Conclusion

While many checks can and should be automated, and while humans are required to think about bigger-picture issues like design and “but did it actually fix the problem?”, there’s also a grey area between the two. In this grey area, what we as code reviewers could benefit from is some guidance about code that looks dodgy but might be OK. It seems logical that a code review tool should provide this guidance. Not only this, but we should also expect our code review tool to allow us to navigate through the code as naturally as we would in our IDE.

Upsource aims to make code review not only as painless as possible, but also provide as much help as a tool can, freeing you up to worry about the things that humans are really good at.

Posted in Code review practices | Leave a comment

What to look for in a Code Review: Security

Posted on by Trisha Gee

This is part 6 of 6 posts on what to look for in a code review. See previous posts from the series.

How much work you do building a secure, robust system is like anything else on your project – it depends upon the project itself, where it’s running, who’s using it, what data it has access to, etc. Often, if our team doesn’t have access to security experts, we go too far in one direction or the other: either we don’t pay enough attention to security issues; or we go through some compliance checklist and try to address everything in some 20 page document filled with potential issues.

As usual, this blog post aims to highlight some areas you might like to look at when reviewing code, but mostly it aims to get you having discussions within your team or organisation to figure out what it is you do need to care about in a code review.

Automation is your friend
A surprising number of security checks can be automated, and therefore should not need a human. Security tests don’t necessarily have to be full-blown penetration testing with the whole system up and running, some problems can be found at code-level.

Common problems like SQL Injection or Cross-site Scripting can be found via tools running in your Continuous Integration environment. You can also automate checking for known vulnerabilities in your dependencies via the OWASP Dependency Check tool.

Of course, Upsource also provides numerous security inspections. These can inform a reviewer of potential security problems in the code. For example, this code executes a dynamically generated SQL string, which might be susceptible to SQL Injection:

SQL Warning

Sometimes “It Depends”
While there are checks that you can feel comfortable with a “yes” or “no” answer, sometimes you want a tool to point out potential problems and then have a human make the decision as to whether this needs to be addressed or not. This is an area where Upsource can really shine. Upsource displays code inspections that a reviewer can use to decide if the code needs to be changed or is acceptable under the current situation.

For example, suppose you’re generating a random number. If all your security checks are enabled, you’ll see the following warning in Upsource:

Security Warning - Random Number

The JavaDoc for java.util.Random specifically states “Instances of java.util.Random are not cryptographically secure”. This may be fine for many of the occasions when you need an arbitrary random number.  But if you’re using it for something like session IDs, password reset links, nonces or salts, as a reviewer you might suggest replacing Random with java.util.SecureRandom.

If you and the code author decide that Random is appropriate for this situation, then it’s a good idea to suppress this inspection for this line of code, and document why it’s OK or point to any discussion on the subject – this way future developers looking at the code can understand this is a deliberate decision.

Suppress Warning

So while tools can definitely point you at potential issues, part of your job as a code reviewer is to investigate the results of any automated checks and decide which action to take.

If you are using Upsource to review your code, you can customise your inspection settings, including selecting security settings. Do this by opening your project in IntelliJ IDEA and navigating to the Inspections settings. Select the settings you want and save them to the Project Default profile. Make sure Project_Default.xml is checked in with your project code, and Upsource will use this to determine which inspections to run.

At the time of writing, these are the available security inspections:

Security Inspections

Understand your Dependencies
Let’s move on to other areas that need a human reviewer. One of the areas where security vulnerabilities can creep into your system or code base is via third party libraries. When reviewing code, at the very least you want to check if any new dependencies (e.g. third party libraries) have been introduced. If you aren’t already automating the check for vulnerabilities, you should check for known issues in newly-introduced libraries.

You should also try to minimise the number of versions of each library – not always possible if other dependencies are pulling in additional transitive dependencies. But one of the simplest way to minimise your exposure to security problems in other people’s code (via libraries or services) is to

  • Use a few as sources as possible and understand how trustworthy they are
  • Use the highest quality library you can
  • Track what you use and where, so if new vulnerabilities do become apparent, you can check your exposure.

This means:

  1. Understanding your sources (e.g. maven central or your own repo vs arbitrarily downloaded jar files)
  2. Trying not to use 5 different versions of 3 different logging frameworks (for example)
  3. Being able to view your dependency tree, even if it’s simply through Gradle/Maven

Check if new paths & services need to be authenticated
Whether you’re working on a web application, or providing web services or some other API which requires authentication, when you add a new URI or service, you should ensure that this cannot be accessed without authentication (assuming authentication is a requirement of your system). You may simply need to check that the developer of the code wrote an appropriate test to show that authentication has been applied.

You should also consider that authentication isn’t just for human users with a username and password. Identity might need to be defined for other systems or automated processes accessing your application or services. This may impact your concept of “user” in your system.

Does your data need to be encrypted?
When you’re storing something on disk or sending things over the wire, you need to know whether that data should be encrypted. Obviously passwords should never be in plain text, but there are plenty other times when data needs to be encrypted. If the code under review is sending data on the wire, saving it somewhere, or it is in some way leaving your system, if you don’t know whether it should be encrypted or not, try and locate someone in your organisation who can answer that question.

Are secrets being managed correctly?
Secrets are things like passwords (user passwords, or passwords to databases or other systems), encryption keys, tokens and so forth. These should never be stored in code, or in configuration files that get checked into the source control system. There are other ways of managing these secrets, for example via a secrets server. When reviewing code, make sure these things don’t accidentally sneak into your VCS.

Should the code be logging/auditing behaviour? Is it doing so correctly?
Logging and auditing requirements vary by project, with some systems requiring compliance with stricter rules for logging actions and events than others. If you do have guidelines on what needs logging, when and how, then as a code reviewer you should be checking the submitted code meets these requirements. If you do not have a firm set of rules, consider:

  • Is the code making any data changes (e.g. add/update/remove)? Should it make a note of the change that was made, by whom, and when?
  • Is this code on some performance-critical path? Should it be making a note of start-time and end-time in some sort of performance monitoring system?
  • Is the logging level of any logged messages appropriate? A good rule of thumb is that “ERROR” is likely to cause an alert to go off somewhere, possibly on some poor on-call person’s pager – if you do not need this message to wake someone up at 3am, consider downgrading to “INFO” or “DEBUG”. Messages inside loops, or other places that are likely to be output more than once in a row, probably don’t need to be spamming your production log files, therefore are likely to be “DEBUG” level.

Conclusion
This is just a tiny subset of the sorts of security issues you can be checking in a code review. Security is a very big topic, big enough that your company may hire technical security experts, or at least devote some time or resources to this area. However, like other non-coding activities such as getting to know the business and having a decent grasp of how to test the system, understanding the security requirements of our application, or at least of the feature or defect we’re working on right now, is another facet of our job as a developer.

We can enlist the help of security experts if we have them, for example inviting them to the code review, or inviting them to pair with us while we review. Or if this isn’t an option, we can learn enough about the environment of our system to understand what sort of security requirements we have (internal-facing enterprise apps will have a different profile to customer-facing web applications, for example), so we can get a better understanding of what we should be looking for in a code review.

And like many other things we’re tempted to look for in code reviews, many security checks can also be automated, and should be run in our continuous integration environment. As a team, you need to discuss which things are important to you, whether checking these can be automated, and which things you should be looking for in a code review.

This post has barely scratched the surface of potential issues. We’d love to hear from you in the comments – let us know of other security gotchas we should be looking for in our code.

Posted in Code review practices | Leave a comment

Upsource In Action, Webinar Recording

Posted on by Maria Khalusova

The recording of our September 29th webinar with Trisha Gee, Upsource In Action, is available on JetBrains YouTube channel.

In this webinar Trisha goes over the typical code review workflow in Upsource, then explores Java support features similar to what you have in IntelliJ IDEA, she also shows how you can use Upsource to explore your code bases, and find exactly what you need using powerful search engine. In the end, she demonstrates some of the features that we are working on for the upcoming Upsource 2.5 release.

Below is the timeline of the webinar and select Q&A.

0:22 – Typical code review workflow in Upsource
10:12 – Java support
15:40 – Exploring code bases with Upsource
19:28 – Powerful search
22:35 – New features in upcoming Upsource 2.5

Q: Are there plans to integrate with Atlassian JIRA?
A:Integration with JIRA is already available starting with Upsource 2.0.3.

Q: What is the effect of setting default branch in Upsource?
A: When the default branch is set, the UI becomes a bit smarter when displaying the commits graph, rendering README files, comparing branches, etc.

Q: When is git repository hosting coming?
A: It is planned for Q1 2016.

Q: Is it possible to create a review for a MergeRequest(gitlab)/PullRequest(gihub)?
A: Pull requests support is planned for Q1 2016.

Q: Can I, as a reviewer do the review in IntelliJ? See all the changed files and their diff? And add comments?
A: Yes, you can participate in code review either from Upsource UI, or from IntelliJ IDEA, if you have Upsource plugin installed, and do all the same as in Upsource UI: create a review, view diffs, participate in discussions, leave comments, resolve comments, receive notifications, approve/reject, etc.

Q: Are there any plans in roadmap to integrate with other IDEs beside IntelliJ IDEA?
A: All JetBrains IDEs are supported including PhpStorm, RubyMine, WebStorm, CLion, AppCode, PyCharm.

Trisha_Gee
Trisha has developed Java applications for a range of industries, including finance, manufacturing and non-profit, for companies of all sizes. She has expertise in Java high performance systems, is passionate about enabling developer productivity, and dabbles with Open Source development.

Posted in Webinars | Leave a comment

Upsource 2.5 Early Access is open!

Posted on by Maria Khalusova

It has been slightly more than a year since the first ever Early Access Upsource build has seen the light. A lot has changed since then, two major releases came out, many new features were introduced, and our up-and-coming code review tool has significantly matured. And we are devoted to keep making it better, faster, and, of course, smarter!

Today we are opening the Early Access Program for the next major version – Upsource 2.5, which is planned to be made available within this year. Here’s what we have for you so far.

Enhanced Email Notifications

We no longer send emails as soon as some noteworthy event occurs (comment was left on your review, changes accepted or rejected, etc.). Instead, we bundle them and send a single email so you have less in your inbox to go through. It is also possible to fine-tune the types of notifications you want to receive. Don’t want to get notified when a review is closed by its author? Disable this particular notification on the Settings page! In addition to that, notifications also feature better code highlighting and other design improvements.

email-notification

Reply by email

Another great feature of reworked email notifications you may notice on the screenshot above is the ability to reply to comments via email. Create a mailbox, configure it in Upsource and you’ll be able to participate in ongoing reviews without ever leaving your email client.

Smarter and faster indexer

Upsource 2.5 drastically reduces the time needed to index a newly added project by introducing bulk import of repositories and on-demand indexing of old revisions. While previously a repository with 10,000 commits took hours to index, it now takes only a few short minutes. Indexing has become 10x faster for Git projects.

Thanks to a smarter indexer, we are now able to reindex the project when settings such as project model or character encoding are changed.

Discussion Labels

Labels can now be applied to discussions to denote priority, category, or any other information that you find useful. Some predefined labels are provided (“bug”, “style”, “enhancement”, “help wanted”), others can be created on a per-project basis. As we strive to make our data eminently searchable, you’re able to find discussions marked with a certain label using label: query.

discussionLabels

Branches page

See the activity across branches on the brand-new Branches page. You can quickly search the branches and see which ones are active or stale.

branches_page

Default branch setting was added to the project configuration screen. When the default branch is set, the UI becomes a bit smarter when displaying the commits graph, rendering README files, comparing branches, etc.

Open in IDE

Version 2.0 introduced an IDE plugin that allowed interacting with Upsource from the IDE. In 2.5 we are providing integration in the opposite direction – it is now possible to open a review (or simply a piece of code) in the IDE by clicking a link in the browser so you can quickly resume work on the code under review.

OpenInIDE

Various improvements

  • Sometimes to understand a change in the inline diff we need to see more than the default context. You can now gradually reveal additional lines of context by clicking on cut lines.
  • Reviews can now be removed. Code comments, if any, are preserved as part of corresponding revisions.
  • We now show the number of discussions right in the revisions list as well as in the list of reviews for instant visibility of your colleagues’ activity.
  • Review timeline has a new option of showing only unresolved discussions.
  • When an issue ID or review ID is present in the commit message we automatically attach the revision to the matching review if it’s’ open – which means you no longer have to do it manually. This behavior is always enabled and not configurable, we plan to provide more customisable workflows later on.

This list of features is not final, as we have more great surprises planned for you.
If you want to play with the EAP build, you can download it here. Please remember, that Early Access builds represent work in progress, therefore we recommend installing them on a trial server. See the reasons why.
As usual, we will be happy to hear your feedback!

Posted in EAP | 3 Comments

Live Webinar: Upsource In Action

Posted on by Maria Khalusova

Join us Tuesday, September 29th, 09:00 AM-10:00 AM PDT (16:00 – 17:00 GMT) for our free live webinar with Trisha Gee featuring Upsource, our code review and repository browsing tool, in action.

In this webinar you’ll see how you can have lightweight transparent code reviews without the need to change your existing process. Next, we are going to show you the “superpowers” Upsource gives to Java teams.

In this presentation you will learn how Upsource helps teams with their daily routines, and for dessert we’ll give you a sneak peek of the features we are preparing for the upcoming release.

Space is limited, please register now.

This webinar is geared towards developers of different proficiency regardless of programming language of choice. During the webinar there will be an opportunity to ask questions. The recording will be available after the webinar.

Trisha_Gee
Trisha has developed Java applications for a range of industries, including finance, manufacturing and non-profit, for companies of all sizes. She has expertise in Java high performance systems, is passionate about enabling developer productivity, and dabbles with Open Source development.

Posted in Webinars | Leave a comment

Mysterious “Build System” Setting

Posted on by Maria Khalusova

From time to time we receive questions from our users about a setting called “Build System” under Upsource Properties on Project Setting page:

build-system-setting

The most common ones are: “Why do I even need to specify build system?” and “What does it do?”. Sometimes there’s also confusion about what exactly one needs to specify there. While we do plan to address this ambiguity through UI modifications, we feel it’s a good idea to explain why a code review tool even needs it in the first place and what to do with it.

In short, this setting unlocks all the code insight and intelligence Upsource offers for Java teams. So if you’re using Upsource to perform code reviews on a project written in another language, feel free to ignore it. But if your team uses Java, and you want to make use of IDE-level server side code inspections and navigation, then keep reading:)

Currently 3 types of build systems are supported: IDEA, Maven and Gradle. Let’s take a look at each of them.

IDEA

Upsource has IntelliJ IDEA engine built in to provide the same level of code intelligence as IntelliJ IDEA does. Needless to say, Upsource natively understands IDEA project model, so you only need to let it know where the .idea folder is located.

In some teams each developer configures their IDE to their liking and everybody has different .idea, which they .gitignore, so in the end there’s no .idea folder in the repository. However, the recommended way is to store most of the .idea folder contents in version control, only omitting the user-specific settings, such as .idea/workspace.xml and .idea/shelf – this way Upsource will be able to provide Java code insight for your project.

Maven/Gradle

Things are a bit different for Maven/Gradle project models, as IntelliJ IDEA engine does not understand them natively. To be able to offer code insight for Java projects with these models Upsource first converts specified project model to IDEA format and uses it later on. If your pom.xml doesn’t have unresolved dependencies and builds fine without Upsource, it should convert properly into IDEA format as well: you can always check if all went well in the maven.out/gradle.out file. To find it, open your Upsource project, click Browse code at the top of the revisions list and navigate to /.idea/maven/mvn.out or /.idea/gradle/gradle.out.

Not sure which pom.xml to specify in Upsource properties? You need the one that knows about all the modules you have and you want to have code insight for, i.e. it should be the parent pom. If your project modules are hosted in multiple repositories (e.g. each module in their own repository), you can configure them all in one Upsource project, and keep the parent pom.xml in the first repo.

When it comes to Gradle, the number one troublemakers are Android Studio projects. Unfortunately, Upsource does not support them at the moment. We are looking into it, and by all means feel free to follow/upvote the issue.

There also have been known cases when gradle plugin converted project model, and configured wrong SDK. It’s a known Gradle bug. For the moment, if this is happening, you can find the workaround here. If you plan to work on Gradle projects without a wrapper, please check Upsource documentation for the instructions.

Naturally, we are always happy to answer your questions if you have any.

Posted in Feature | Leave a comment

Upsource 2.0.4 bug-fix update

Posted on by Maria Khalusova

Hi everyone!

Previous Upsource update featured YouTrack integration via external Hub, and some of our users have experienced issues with the integration. We apologise for inconvenience this may have caused and we would like to thank you for your patience and for reporting to us the problems you have encountered.
We have addressed the issues and today we have made available a bug-fix update. Please, upgrade your Hub as well as soon as the new Hub version is available.

Take a look at the complete list of fixes here, and download the build.

We recommend upgrading your instance to the latest version: for the upgrade instructions, please refer to Upsource documentation. Don’t forget to backup your current instance before upgrading.

Posted in Update | Leave a comment

What to look for in a Code Review: SOLID Principles

Posted on by Trisha Gee

This is part 5 of 6 posts on what to look for in a code review. See other posts from the series.

In today’s post we’ll look more closely at the design of the code itself, specifically checking to see if it follows good practice Object Oriented Design.  As with all the other areas we’ve covered, not all teams will prioritise this as the highest value area to check, but if you are trying to follow SOLID Principles, or trying to move your code in that direction, here are some pointers that might help.

What is SOLID?

The SOLID Principles are five core principles of Object Oriented design and programming. The purpose of this post is not to educate you on what these principles are or go into depth about why you might follow them, but instead to point those performing code reviews to code smells that might be a result of not following these principles.

SOLID stands for:

Single Responsibility Principle (SRP)

There should never be more than one reason for a class to change.

This can sometimes be hard to spot from a single code review. By definition, the author is (or should be) applying a single reason to change the code base – a bug fix, a new feature, a focussed refactoring.

You want to look at which methods in a class are likely to change at the same time, and which clusters of methods are unlikely to ever be changed by a change to the other methods. For example:

Single Responsibility Principle

This side-by-side diff from Upsource shows that a new piece of functionality has been added to TweetMonitor, the ability to draw the top ten Tweeters in a leaderboard on some sort of user interface. While this seems reasonable because it uses the data being gathered by the onMessage method, there are indications that this violates SRP. The onMessage and getTweetMessageFromFullTweet methods are both about receiving and parsing a Twitter message, whereas draw is all about reorganising that data for displaying on a UI.

The reviewer should flag these two responsibilities, and then work out with the author a better way of separating these features: perhaps by moving the Twitter string parsing into a different class; or by creating a different class that’s responsible for rendering the leaderboard.

Open-Closed Principle (OCP)

Software entities should be open for extension, but closed for modification.

As a reviewer, you might see indications that this principle is being violated if you see a series of if statements checking for things of a particular type:

Open-Closed Principle

If you were reviewing the code above, it should be clear to you that when a new Event type is added into the system, the creator of the new event type is probably going to have to add another else to this method to deal with the new event type.

It would be better to use polymorphism to remove this if:

Open-Closed Principle

Open-Closed Principle

As always, there’s more than one solution to this problem, but the key will be removing the complex if/else and the instanceof checks.

Liskov Substitution Principle (LSP)

Functions that use references to base classes must be able to use objects of derived classes without knowing it.

One easy way to spot violations of this principle is to look for explicit casting. If you have to cast a object to some type, you are not using the base class without knowledge of the derived classes.

More subtle violations can be found when checking:

Imagine, for example, we have an abstract Order with a number of subclasses – BookOrder, ElectronicsOrder and so on. The placeOrder method could take a Warehouse, and could use this to change the inventory levels of the physical items in the warehouse:

Liskov Substitution Principle

Now imagine we introduce the idea of electronic gift cards, which simply add balance to a wallet but do not require physical inventory. If implemented as a GiftCardOrder, the placeOrder method would not have to use the warehouse parameter:

CR5-LSP1

This might seem like a logical use of inheritance, but in fact you could argue that code that uses GiftCardOrder could expect similar behaviour from this class as the other classes, i.e. you could expect this to pass for all subtypes:

CR5-LSP2

But this will not pass, as GiftCardOrders have a different type of order behaviour. If you’re reviewing this sort of code, question the use of inheritance here – maybe the order behaviour can be plugged in using composition instead of inheritance.

Interface Segregation Principle (ISP)

Many client specific interfaces are better than one general purpose interface.

Some code that violates this principle will be easy to identify due to having interfaces with a lot of methods on.  This principle compliments SRP, as you may see that an interface with many methods is actually responsible for more than one area of functionality.

But sometimes even an interface with just two methods could be split into two interfaces:

CR5-LSP3

In this example, given that there are times when the decode method might not be needed, and also that a codec can probably be treated as either an encoder or a decoder depending upon where it’s used, it may be better to split the SimpleCodec interface into an Encoder and a Decoder. Some classes may choose to implement both, but it will not be necessary for implementations to override methods they do not need, or for classes that only need an Encoder to be aware that their Encoder instance also implements decode.

Dependency Inversion Principle (DIP)

Depend upon Abstractions. Do not depend upon concretions.

While it may be tempting to look for simple cases that violate this, like liberal use of the new keyword (instead of using Dependency Injection or factories, for example) and overfamiliarity with your collection types (e.g. declaring ArrayList variables or parameters instead of List), as a reviewer you should be looking to make sure the code author has used or created the correct abstractions in the code under review.

For example, service-level code that uses a direct connection to a database to read and write data:

CR5-DIP1

This code is dependent on a lot of specific implementation details: JDBC as a connection to a (relational) database; database-specific SQL; knowledge of the database structure; and so on. This does belong somewhere in your system, but not here where there are other methods that don’t need to know about databases. Better to extract a DAO or use the Repository pattern, and inject the DAO or repository into this service.

Summary

Some code smells that might indicate one or more of the SOLID Principles have been violated:

  • Long if/else statements
  • Casting to a subtype
  • Many public methods
  • Implemented methods that throw UnsupportedOperationException

As with all design questions, finding a balance between following these principles and knowingly bending the rules is down to your team’s preferences. But if you see complex code in a code review, you might find that applying one of these principles will provide a simpler, more understandable, solution.

Posted in Code review practices | 4 Comments

What to look for in a Code Review: Data Structures

Posted on by Trisha Gee

This is part 4 of 6 posts on what to look for in a code review. See previous posts from the series.

Data structures are a fundamental part of programming – so much so it’s actually one of the areas that’s consistently taught in Computer Science courses. And yet it’s surprisingly easy to misuse them or select the wrong one. In this post, we’re going to guide you, the code reviewer, on what to look out for – we’re going to look at examples of code and talk about “smells” that might indicate the wrong data structure was chosen or that it’s being used in an incorrect fashion.

Lists

Probably the most common choice for a data structure. Because it is the most common choice, it’s sometimes used in situations it shouldn’t be.

Anti-Pattern: Too Much Searching Iterating over a listIterating over a list is not, in itself, a bad thing of course. But if iteration is required for a very common operation (like the example above of finding a customer by ID), there might be a better data structure to use. In our case, because we always want to find a particular item by ID, it might be better to create a map of ID to Customer.

Remember that in Java 8, and languages which support more expressive searches, this might not be as obvious as a for-loop, but the problem still remains.

Iterating over a list using Java 8

Anti-Pattern: Frequent Reordering

List Sorting

Lists are great if you want to stick to their default order, but if as a reviewer you see code that’s re-sorting the list, question whether a list is the correct choice. In the code above, on line 16 the twitterUsers list is always re-sorted before being returned. Once again, Java 8 makes this operation look so easy it might be tempting to ignore the signs:

List sorting in Java 8

Upsource shows the pre-Java-8 method of sorting (lines 16-20, in red) replaced with the Streams solution (line 16, in green)

In this case, given that a TwitterUser is unique and it looks like you want a collection that’s sorted by default, you probably want something like a TreeSet.

Use a sorted set instead of a list

Upsource’s side-by-side diff with the change

Maps

A versatile data structure that provide O(1) access to individual elements, if you’ve picked the right key.

Anti-Pattern: Map as global constant

The map is such a good general purpose data structure that it can be tempting to use globally accessible maps to let any class get to the data.

CR4MapGlobal

In the above code, the author has chosen to simply expose the CUSTOMERS map as a global constant. The CustomerUpdateService therefore uses this map directly when adding or updating customers. This might not seem too terrible, since the CustomerUpdateService is responsible for add and update operations, and these have to ultimately change the map. The issue comes when other classes, particularly those from other parts of the system, need access to the data.

CR4MapAbuse

Here, the order service is aware of the data structure used to store customers. In fact, in the code above, the author has made an error – they don’t check to see if the customer is null, so line 12 could cause a NullPointerException. As the reviewer of this code, you’ll want to suggest hiding this data structure away and providing suitable access methods. That will make these other classes easier to understand, and hide any complexity of managing the map in the CustomerRepository, where it belongs. In addition, if later you change the customers data structure, or you move to using a distributed cache or some other technology, the changes associated with that will be restricted to the CustomerRepository class and not ripple throughout the system. This is the principle of Information Hiding.

CR4MapHidden

Although the updated code isn’t much shorter, you have standardised and centralised core functions – for example, you know that getting a customer who doesn’t exist is always going to give you an Exception. Or you can choose to have this method return the new Optional type.

Note that this is exactly the sort of issue that should be found during a code review – hiding global constants is hard to do once their use has propagated throughout the system, but it’s easy to catch this when they’re first introduced.

Other Anti-Patterns: Iteration & Reordering

As with lists, if a code author has introduced a lot of sorting of, or iterating over, a map, you might want to suggest an alternative data structure.

Java-specific things to be aware of

In Java, map behaviour usually relies on your implementation of equals and hashCode for the key and the value. As a reviewer, you should check these methods on the key and value classes to ensure you’re getting the expected behaviour.

Java 8 has added a number of very useful methods to the Map interface. The getOrDefault method, for example, could simplify the CustomerRepository code at line 11 in the example above.

Sets

An often-underused data structure, its strength is that is does not contain duplicate elements.

Anti-pattern: Sometimes you really do want duplicates

Let’s assume you had a user class that used a set to track which website they had visited. Now, the new feature is to return the most recently visited of these websites.

CR4SetMisuse

The author of this code has changed the initial set that tracks the sites a user has visited from HashSet to LinkedHashSet – this latter implementation preserves insertion order, so now our set tracks every URI in the order in which they were visited.

There are a number of signs in this code that this is wrong though. Firstly, the author has had to do a costly full iteration of the whole set to reach the last element (lines 13-15) – sets are not designed for accessing elements by position, something that lists are perfect for. Secondly, because sets do not contain duplicate values, if the last page they visited had been visited previously, it will not be in the last position in the set. Instead, it will be where it was first added to the set.

In this case, a list, a stack (see below), or even just a single field, might give us better access to the last page visited.

Java-specific things to be aware of

Because one of the key operations of a set is contains, as a reviewer you should be checking the implementation of equals on the type contained in the set.

Stacks

Stacks are a favourite of Computer Science classes, and yet in the real world are often overlooked – in Java, maybe this is because Stack is an extension of Vector and therefore somewhat old-fashioned. Rather than going into a lot of detail here I’ll just cover key points:

  • Stacks support LIFO, and should ideally be used with push/pop operations, it’s not really for iterating over.
  • The class you want for a stack implementation in Java (since version 1.6) is Deque. This can act as both a queue and a stack, so reviewers should check that deques are used in a consistent fashion in the code.

Queues

Another CS favourite. Queues are often spoken about in relation to concurrency (indeed, most of the Java implementations live in java.util.concurrent), as it’s a common way to pass data between threads or modules.

  • Queues are FIFO data structures, generally working well when you want to add elements to the tail of the queue, or remove things from the front of the queue. If you’re reviewing code that shows iteration over a queue (in particularly accessing elements in the middle of the queue), question if this is the correct data type.
  • Queues can be bounded or unbounded. Unbounded queues could potentially grow forever, so if reviewing code with this type of data structure, check out the earlier post on performance. Bounded queues can come with their own problems too – when reviewing code, you should look for the conditions under which the queue might become full, and ask what happens to the system under these circumstances.

A general note for Java developers

As a reviewer, you should be aware not only of the characteristics of general data structures, but you should also be aware of the strengths and weaknesses of each of the implementations all of which are documented in the Javadoc:

If you’re using Java 8, remember that many of the collections classes have new methods. As a reviewer you should be aware of these – you should be able to suggest places where the new methods can be used in place of older, more complex code.

Why select the right data structure?

We’ve spent this blog post looking at data structures – how to tell if the code under review might be using the wrong data structures, and some pointers for the pros and cons of various data structure so not only can you, as the reviewer, identify when they might be being used incorrectly, but you can also suggest better alternatives. But let’s talk about why using the right data structure is important.

Performance

If you’ve studied data structures in computer science, you’ll often learn about the performance implications of picking one over another. Indeed, we even mentioned “Big O Notation” in this blog post to highlight some of the strengths of particular structures. Using the right data structure in your code can definitely help performance, but this is not the only reason to pick the right tool for the job.

Stating Expected Behaviour

Developers who come to the code later, or who use any API exposed by your system, will make certain assumptions based on data structures. If the data returned from a method call is in a list, a developer will assume it is ordered in some fashion. If data is stored in a map, a developer can assume that there is a frequent need to look up individual elements by the key. If data is in a set, then a developer can assume it’s intentional that this only stores an element once and not multiple times. It’s a good idea to work within these assumptions rather than break them.

Reducing Complexity

The overall goal of any developer, and especially of a reviewer, should be to ensure the code does what it’s supposed to do with the minimal amount of complexity – this makes the code easier to read, easier to reason about, and easier to change and maintain in the future. In some of the anti-patterns above, for example the misuse of Set, we can see that picking the wrong data structure forced the author to write a lot more code. Selecting the right data structure should, generally, simplify the code.

In Summary

Picking the right data structure is not simply about gaining performance or looking clever in front of your peers. It also leads to more understandable, maintainable code. Common signs that the code author has picked the wrong data structure:

  • Lots of iterating over the data structure to find some value or values
  • Frequent reordering of the data
  • Not using methods that provide key features – e.g. push or pop on a stack
  • Complex code either reading from or writing to the data structure

In addition, exposing the details of selected data structures, either by providing global access to the structure itself, or by tightly coupling your class’s interface to the operation of an underlying data structure, leads to a brittleness of design, and will be hard to undo later. It’s better to catch these problems early on, for example during a code review, than incur avoidable technical debt.

Posted in Code review practices | 5 Comments

Malicious code reviews: 11 tips to piss the whole team off

Posted on by Maria Khalusova

Disclaimer: Please do not take this post seriously, and happy Friday!

Your team has decided to adopt code review practice, and your perfect code now will be judged by someone else. Well, how dare they think they can find any issues with your brilliant code? On top of that, you need to spend your valuable time reviewing someone else’s code? That does it! Fight back the tyranny by following these simple rules:
If your code is about to be reviewed

  1. Commit all the features and bug-fixes you’ve worked on this week in one go, and ask for a review. Let them see how much work you’ve done.
  2. Invite the whole team to review your code. Everybody has to know how great your features are.
  3. Be original. Do not use spell checker, do not use static code analysis. Do, however, use ReSharper or IntelliJ IDEA to reformat code to your style, ignoring the company’s standards.
  4. Introduce a tricky bug on purpose. See if they’re smart enough to find it.

If you are reviewing someone else’s code

  1. Take your time. Got a code review assigned to you? Take your time, it’s not important. No, really, have a cup of tea, answer all unanswered emails, tweet something, see what you friends have been up to on facebook, go home. Why should reading someone’s code be more important than what you want to do? Feel free to ignore notifications for a week or two: you are busy.
  2. Take your time doing review. They wanted you to find issues? Don’t close the review until you find at least a dozen, even if it takes months.
  3. See a bug? Perfect! First, question the author’s intelligence, then, when your superiority is well established, demand a fix.
  4. Found an elegant solution to a problem? Don’t tell anyone, especially the code author, that you’re impressed. Keep it cool, act like you’ve always knew how to do things best.
  5. Embrace your inner grammar nazi: Focus on the spelling, it’s the most important thing! How can we misspell things and call ourselves professionals?
  6. Count all the spaces. Point out every bad indentation.
  7. Never compromise. Even if another solution offered by your teammate is better than yours. Stand your ground.

Good luck!

 

Advertisements

Desktop (client) specs computer hardware

Here’s my mini-server specs: https://gerardlee.wordpress.com/2011/01/22/server-specs-computer-hardware/

For desktop client use:

  • Ordinary PC capable of Java applications
  • Cash drawer
  • POS-like structure
  • Barcode Scanner
  • Barcode Printer
  • Dot-Matrix receipt printer
  • Laser jet or Photo printer with CISS capability.

Server specs computer hardware

Here’s the desktop specs: https://gerardlee.wordpress.com/2011/01/22/desktop-client-specs-computer-hardware/

For buying:
Casing:
Orient Transformer with 600W Yellow/Red/Gray        1,550.00

Processor:
AMD Athlon II X3 400 3.0Ghz                            3,250.00

Motherboard:
Emaxx AMD880HD3-Pro                                    2,700.00

Memory:
4GB DDR3 1333 Emaxx (2-yr wty)                        2,100.00        8 GB
4GB DDR3 1333 Geil Single (2-yr wty)                2,350.00

HDD: (for data storage)
500GB Seagate 7200RPM 16MB (2-yr wty)                1,950.00
500GB Transcend Storejet with Anti-shock            3,650.00

Graphics: (for monitor)
Palit GF 9800GT 512MB 256Bit DDR3 Green Edition        3,600.00

Mouse:
A4Tech G7-630-5 2.4G Wireless Optical                  690.00

WiFi: (for internet access)
TP-Link TL-WN821N USB WiFi Adapter                    1,150.00        2 pcs

UPS: (for power outages; no server should be down)
1200VA Powergarde UPS with AVR                        3,400.00        1

Optical Drive: (for data backup and server installation)
Liteon ETAU108-32 8X External DVD-RW                2,150.00        1

Total:                                                31,240.00

Network cabling:
UTP Cable (per box)                                    1,250.00        1 box
RJ 45 Connector                                            3.00        20 pcs

Expressing words in binary

Do you feel the need to shortcut everything?  Especially when you feel lazy typing everything on a messaging system like Yahoo or Skype… or even cellphone to some techy guys.  Here’s how:

Convert short answers to binary.

Okay or not = Yes or no = true or false = just type 1 or 0

Thanks = thank you = 10x (tenx) = 2x (in decimal form, but every techy guys understands this)

Finally, on some surveys that you dont wanna give your name exclusively, but you feel honest about it.  Just give your name in binary form. – That way its still valid.

my dev batch file

C:\DevTools\cmd.bat

@echo off
@set CATALINA_HOME=C:\servers\apache-tomcat-5.5.27
@set JAVA_HOME=C:\Program Files\Java\jdk1.6.0_16
@set M2_HOME=C:\DevTools\java\apache-maven-2.2.1
@set MAVEN_OPTS=-Xmx1024m -XX:MaxPermSize=512m
@set TOMCAT_HOME=%CATALINA_HOME%
@set VIM=C:\DevTools\vim
@set COMSPEC=%SystemRoot%\system32\cmd.exe
@set HOME=C:\Documents and Settings\SG0208743
@set RUBY_HOME=C:\DevTools\ruby-1.8.7

@rem =======================
@rem local path
@rem =======================
@set

PATH=C:\DevTools\gnuwin32\bin;C:\DevTools\MinGW\bin;%RUBY_HOME%\bin;%M2_HOME%\bin;%JAVA_HOME%\bin;C:\DevTools\Sysint

ernalsSuite;C:\DevTools\vim;C:\DevTools\eText\cmd;C:\DevTools\cygwin\usr\local\narwhal\bin;

@rem =======================
@rem git
@rem =======================
@set git_install_root=C:\DevTools\git-1.7.1
@set PLINK_PROTOCOL=ssh
@setlocal
@set path=%PATH%;%git_install_root%\bin;%git_install_root%\mingw\bin;%git_install_root%\cmd;
@if “%HOME%”==”” @set HOME=%USERPROFILE%
@cd %HOME%

@rem =======================
@rem global path
@rem =======================
@set PATH=%PATH%;C:\DevTools\oraclexe\app\oracle\product\10.2.0\server\bin;C:\Program Files\CollabNet\Subversion

Client;C:\DevTools\MySQL\MySQL Server 5.0\bin;%SystemRoot%\system32;%SystemRoot%;%SystemRoot%\System32\Wbem;

@rem =======================
@rem start command process
@rem =======================
@start C:\DevTools\cygwin\bin\mintty.exe %COMSPEC%

A hack for mintty to use as Windows command console cmd

If you’re like me, vain in changing the Windows command console (cmd.exe) and if you like the functionality and look and feel of mintty (cygwin), here’s how to change the look and feel of cmd.exe:

  1. Install Cygwin with Shells -> Mintty included.
  2. start <cygwin directory>/bin/mintty %COMSPEC%

Start is a windows command for spawning new applications.

<cygwin directory>/bin/mintty is the application you want to start.

%COMSPEC% is the parameter for mintty.  Windows will substitue this to cmd.exe.  Well you can call it directly using “start <cygwin directory>/bin/mintty cmd.exe”

Example screehshot:

Hope this helps