Java Code Review Guidelines | Java code geeks


Having another pair of eyes scanned your code is always helpful. It helped me improve my writing cleaner code and catch errors faster. You don’t have to be an expert to review someone’s code. Some programming language experience and a review checklist should get you started.

The following is an organized list of tips to keep handy when reviewing Java code.

To note: This list is not exhaustive but should get you started.

1. Follow the Java code conventions

By following linguistic conventions, allows you to quickly browse the code and make sense of it, thus improving readability.

All package names in Java are written in lowercase, constants in uppercase, variable names in CamelCase, etc. Find the complete list of conventions here.

Some teams develop their own conventions, so be flexible in such cases!

2. Replace the imperative code with lambdas and flows

If you are using Java 8+, replacing loops and extremely verbose methods with streams and lambdas makes the code cleaner. Lambdas and streams allow you to write functional code in Java.

The following code snippet filters odd numbers in the traditional imperative way:

List<Integer> oddNumbers = new ArrayList<>();
for (Integer number : Arrays.asList(1, 2, 3, 4, 5, 6)) {
	if (number % 2 != 0) {
	  oddNumbers.add(number);
  }
}

Here is the functional way to filter odd numbers:

List<Integer> oddNumbers = Stream.of(1, 2, 3, 4, 5, 6)
                .filter(number -> number % 2 != 0)
                .collect(Collectors.toList());
class Items {
	private final List<Integer> items;
	public Items(List<Integer> items) {
	        this.items = items;
	}
	public Integer highest() {
	  if (items.isEmpty()) return null;
	  Integer highest = null;
	  for (Integer item : items) {
	      if (items.indexOf(item) == 0) highest = item;
	      else highest = highest > item ? highest : item;
	  }
	  return highest;
	}
}

Before directly calling a method on an object, I recommend checking for null values ​​as shown below.

Items items = new Items(Collections.emptyList());
Integer item = items.highest();
boolean isEven = item % 2 == 0; // throws Null Pointer Exception ❌
boolean isEven = item != null && item % 2 == 0  // ✅

However, it can be quite tedious to have null checks all over your code. If you are using Java 8+, consider using the Optional class to represent values ​​that may not have valid states. It allows you to easily define alternative behavior and is useful for chaining methods.

In the snippet below, we use the Java Stream API to find the highest number with a method that returns a Optional. Note that we are using Stream.reduce, which returns a Optional value.

public Optional<Integer> highest() {
    return items
            .stream()
            .reduce((integer, integer2) -> 
							integer > integer2 ? integer : integer2);
}
Items items = new Items(Collections.emptyList());
items.highest().ifPresent(integer -> {             // ? ?
    boolean isEven = integer % 2 == 0;
});

You can also use annotations such as @Nullable or @NonNull which will result in warnings if there is a null conflict while building the code, ie. spend a @Nullable argument to a method that accepts @NonNull settings.

4. Direct attribution of customer code references to a field

The references exposed to the client code can be manipulated even if the field is definitive. Let’s better understand this with an example.

private final List<Integer> items;
public Items(List<Integer> items) {
        this.items = items;
}

In the code snippet above, we directly assign a customer code reference to a field. The client can easily mutate the contents of the list and manipulate our code as shown below.

List<Integer> numbers = new ArrayList<>();
Items items = new Items(numbers);
numbers.add(1); // This will change how items behaves as well!

In the code snippet above, we directly assign a customer code reference to a field. The client can easily mutate the contents of the list and manipulate our code as shown below.

List<Integer> numbers = new ArrayList<>();
Items items = new Items(numbers);
numbers.add(1); // This will change how items behaves as well!

Instead, consider cloning the ref or creating a new ref and then assigning it to the field as shown below:

private final List<Integer> items;
public Items(List<Integer> items) {
        this.items = new ArrayList<>(items);
}

5. Handle exceptions with care

  • When catching exceptions, if you have multiple catch blocks, make sure that the sequence of catch blocks is at least the most specific. In the snippet below, the exception will never be caught in the second block because the Exception the class is the mother of all exceptions.
try {
	stack.pop();
} catch (Exception exception) {
	//handle exception
} catch (StackEmptyException exception) {
	//handle exception
}
  • If the situation is recoverable and can be handled by the client (the consumer of your library or code), it is a good idea to use checked exceptions. for example. IOException is a checked exception that forces the client to handle the scenario and in case the client chooses to throw the exception again, it should be a conscious call to ignore the exception.

6. Think about the choice of data structures

Java collections provide ArrayList, LinkedList, Vector, Stack, HashSet, HashMap, Hashtable. Understanding the pros and cons of each is important to use them in the right context.

Some tips to help you make the right choice:

Map – Useful if you have unordered items in the form of key and value pairs and need efficient retrieve, insert, and delete operations. HashMap, Hashtable, LinkedHashMap are all implementations of Map interface.

List – Very commonly used to create an ordered list of items. This list may contain duplicates. ArrayList is an implementation of List interface. A list can be secure with threads using Collections.synchronizedList thus removing the need to use Vector. Hereis a little more information on why Vector is essentially obsolete.

Set – Similar to the list but does not allow duplicates. HashSet implements the Set interface.

7. Think twice before exhibiting

There are a number of access modifiers to choose from in Java – public, protected, private. Unless you want to expose a method to the client code, you might want to keep everything private by default. Once you expose an API, there is no going back.

For example, you have a class Library which has the following method to order a book by name:

public checkout(String bookName) {
	Book book = searchByTitle(availableBooks, bookName);
  availableBooks.remove(book);
  checkedOutBooks.add(book);
}

private searchByTitle(List<Book> availableBooks, String bookName) {
...
}

If you do not keep the searchByTitle private method by default and it ends up being exposed, other classes might start to use it and build logic that you might have wanted to be part of the Library classify. This could break the encapsulation of the Library class or it may be impossible to revert / modify later without breaking someone else’s code. Consciously exhibit!

8. Code at interfaces

If you have concrete implementations of certain interfaces (for example ArrayList or LinkedList) and if you use them directly in your code, it can lead to high coupling. Stick to the List The interface allows you to switch the implementation at any time in the future without breaking any code.

public Bill(Printer printer) {
	this.printer = printer;
}

new Bill(new ConsolePrinter());
new Bill(new HTMLPrinter());

In the code snippet above, using the Printer the interface allows the developer to switch to another concrete class HTMLPrinter.

9. Do not force adjustment interfaces

Take a look at the following interface:

interface BookService {
		List<Book> fetchBooks();
    void saveBooks(List<Book> books);
    void order(OrderDetails orderDetails) throws BookNotFoundException, BookUnavailableException;	
}

class BookServiceImpl implements BookService {
...

Is there an advantage to creating such an interface? Is there a possibility for this interface to be implemented by another class? Is this interface generic enough to be implemented by another class? If the answer to all of these questions is no, I highly recommend that you avoid this unnecessary interface that you will have to maintain in the future. Martin Fowler explains it very well in his Blog.

So what’s the right use case for an interface? Let’s say we have a class Rectangle and one class Circle which has a behavior to calculate the perimeter. If there is a requirement, to sum up, the perimeter of all shapes – a use case for polymorphism, then having the interface would make more sense, as shown below.

interface Shape {
		Double perimeter();
}

class Rectangle implements Shape {
//data members and constructors
    @Override
    public Double perimeter() {
        return 2 * (this.length + this.breadth);
    }
}

class Circle implements Shape {
//data members and constructors
    @Override
    public Double perimeter() {
        return 2 * Math.PI * (this.radius);
    }
}

public double totalPerimeter(List<Shape> shapes) {
	return shapes.stream()
               .map(Shape::perimeter)
               .reduce((a, b) -> Double.sum(a, b))
               .orElseGet(() -> (double) 0);
}

10. Replace hashCode when you replace equal to

Objects that are equal because of their values ​​are called Valuable objects. for example, money, time. These classes should replace the equals to return true if the values ​​are the same. the equals The method is typically used by other libraries for comparisons and equality checks; so overwhelming equals is necessary. Each Java object also has a hash code value that differentiates it from another object.

class Coin {
    private final int value;

    Coin(int value) {
        this.value = value;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        Coin coin = (Coin) o;
        return value == coin.value;
    }
}

In the example above, we replaced only the equals method of Object.

HashMap<Coin, Integer> coinCount = new HashMap<Coin, Integer>() {{
            put(new Coin(1), 5);
            put(new Coin(5), 2);
        }};
        //update count for 1 rupee coin
        coinCount.put(new Coin(1), 7);

				coinCount.size(); // 3 🤯 why? 

We would wait coinCount to update the number of coins from 1 rupee to 7 since we are replacing equals. But HashMap internally checks if the hash code for 2 objects is equal and only then performs the equality test via the equals method. Two different objects may or may not have the same hash code, but two equal objects must always have the same hash code, as defined by the contract of the hashCode method. So checking the hash code first is an early exit condition. This implies that both equals and hashCode methods must be replaced to express equality.

Enter DeepSource

I have described 10 issues you might encounter when examining Java code. There is, however, an endless list of issues that could be overlooked by one or more individuals. While code review is a good opportunity to learn, it could be a repetitive and tedious task; this is where DeepSource comes in.

Posted on Java Code Geeks with the permission of deepsource, partner of our JCG Program. See the original article here: Java Code Review Guidelines

The opinions expressed by contributors to Java Code Geeks are their own.



Source link

Deixe uma resposta

O seu endereço de e-mail não será publicado. Campos obrigatórios são marcados com *