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 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.

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

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

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:

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:

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

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)

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.

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.

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).

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:

–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.

Find a home for your Upsource dist

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:

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.

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.

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:

Catching the Exception and wrapping it in a more appropriate one, possibly a RuntimeException, that can be handled at the right level.

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.

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.

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).

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.

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:

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.

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

It’s an existing class/method/field/variable that has been unused for some time.

It’s an existing class/method/field/variable that is now unused due to the changes introduced in the code review.

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.

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 redundancy, Java > 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.

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.

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:

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:

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.

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:

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.

Trying not to use 5 different versions of 3 different logging frameworks (for example)

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.

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.

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 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.

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.

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.

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.

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.

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!

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.

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 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.

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

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.

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.

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.

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:

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:

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:

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.

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:

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:

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:

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:

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:

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.

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 SearchingIterating 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.

Anti-Pattern: Frequent Reordering

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:

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.

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.

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.

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.

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.

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.

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

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.

Invite the whole team to review your code. Everybody has to know how great your features are.

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.

Introduce a tricky bug on purpose. See if they’re smart enough to find it.

If you are reviewing someone else’s code

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.

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.

See a bug? Perfect! First, question the author’s intelligence, then, when your superiority is well established, demand a fix.

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.

Embrace your inner grammar nazi: Focus on the spelling, it’s the most important thing! How can we misspell things and call ourselves professionals?

Count all the spaces. Point out every bad indentation.

Never compromise. Even if another solution offered by your teammate is better than yours. Stand your ground.

One of my colleagues struck me by saying “you are tool dependent”. From the very moment he said that to me, I agree deep within – although I never answer or loudly answer YES. I know what he means by that and now I’m asking myself that very question. In order to answer that question, lets define first what being tool dependent is?

Explores a lot of tools, recursively

Is lazy

Yet is ideal

The goal is to produce or optimize things with very little effort

Does not focus on the subject matter but instead focuses on the tools to accomplish the problem or the subject matter

Kills time for tool exploration

Plays alot with the tools

Spends time doing boilerplate codes

A tool slave – relies on the tools very much

Always buying time or always lacks time

Never get things done

Couldn’t get things done without the tools

Having a hard time controlling oneself

Does not have self-discipline

Have an attention deficit disorder

Easily persuaded by tools

Jack of all trades, master of none

Critical thinker

Creative thinker to the point of once a problem occurs, the solution instantaeneously pops out of the brain and then performs research about what tool accomplishes the problem instead of analyzing the problem first and then acting after.

An action man without thinking first

You overly submit oneself to that way

Has a lot of options because of the result of finding a lot of possible tools and solutions to solving a particular problem.

Always planning to escape being tool dependent but cannot

Tends to over-analyze things

From this definition alone, I would say I am. Well, I just made up this definition from what I understand to be a tool dependent.

Upon reflecting, here I think are ways to escape from being a tool dependent person.

A determination to change

An everyday re-assessment of goals if it gets done

Focus and focus on solving the subject matter by not relying from the tools

Organize

Follow up

Focus on personal development – by not using those tools

Have a general sense of whats happening – have a birds eye view

Think first before acting

Always rely to oneself. Don’t be dependent to things – and don’t escape the situation

Develop a sense of independence

Believe in your own competency

Gain personal mastery and self-control

Systematic way of tackling tool dependence.

Being dependent on things means holding on desperately to things to give life a meaning or direction. In this case, to give you a desired solution to the problem you are tackling.

Be willing to let go of the tools. Begin by resenting those tools that can keep you back from all that you are capable of.

Have the ability to self-initiate and lead oneself.

Limit the tool selection to just 3, so that at least you have the options.

Don’t over-analyze things for your pleasure.

Practice, practice, practice. From what I’ve heard, all it takes to acquire a habit is to perform it for two weeks consistently. For example, if you are lazy to brush your teeth everyday – and you want to do something about it; train your mind and *do* it for two weeks and it becomes a habit.