How to write readable code

A year after graduating from college, a classmate who is not a programmer asked: why do programmers always look at other people's code and not look good? At the beginning, I felt that this was a naked challenge to programmers. I denied it directly: isn't it like this? I feel like some people's code is good. But the problem has always been in my mind.

After working for so many years, I put forward a Bug list for one test and asked me to modify it. I'm not familiar with this module, but it's not difficult based on experience. When I see the code, my heart is: "oh my God, you silly X, what is this you wrote?" , but I think for a second, the other side's writing represents his understanding of this function, but I didn't understand it. Suddenly I wanted to find the answer to the question.

 

There is a big difference between the programmer position and other positions: deep ideological cooperation. From business understanding to scheme design to class naming, variable name and parameter order during implementation. Every decision you make will be contacted by the partners and the maintenance personnel in the future. Every decision you make will collide with the ideas of others. So how important it is to have a comfortable understanding of intention. It's no wonder that the first chapter of the book "Clean Code" devoted a whole chapter to the importance of code readability.

This is the answer to my classmates' questions: I think the main reason why programmers don't look at each other well is that they don't use programming language to express their intentions well.

I reread the code to find the cause of the problem

  • Isn't the method named well? It all looks right.

  • Is variable naming bad? Not bad either.

  • Is the function too long? It's basically in 20 lines, it's horribly short.

  • Is the cyclomatic complexity high? Functions are not long, where can they be complex?

It seems that the code conforms to various development specifications and the process quality is very good, but the result is still not easy to read.

After a period of thinking, I got a few points

Focus on business

Function requirements in Clean Code: short; only one thing to do. In fact, there is another requirement: each function has an abstract level. I understand it as: focus on the business.

 

In our general projects, there are three levels of structure: Controller, Service and Repository. Each layer is responsible for different responsibilities, the Controller is responsible for handling the key transport content of Http request items, the Service is responsible for handling business logic, and the Repository is responsible for handling persistence. When the Service layer receives the HttpRequest object, we can obviously know that the level is exceeded. But in fact, our business is not only three levels. Each function should be treated as one level, and each level has its own business.

You might say: Show me your code as a programmer. OK, code up. The following code is responsible for calculating a task, checking whether the task is valid, loading task information, calculating Kpi data, and saving the result

public void calculate(CalculateTask task) {
    if (!task.isValidate()) {
        return;
    }
    List<TaskData> taskDatas = loadTaskData(task);
    List<TaskKpiData> kpiData = calculateAllKpi(taskDatas);

    // Save Result
    String version = getVersion(task);
    List<String> ids = getKpiIds(kpiData);
    esObjectClient.deleteByVersion(version, ids);
    esObjectClient.saveAll(kpiData);
    updateLatest(kpiData, task);
}

The logic here is relatively simple, and the code is written in front of it quite simply. It can be seen from the comments that the code block behind is responsible for saving the calculation results, but the business of calculation method should be the scheduling process of the whole Task. This process only cares about when the action is saved, not how to save the details of the data, so it is more reasonable

public void calculate(CalculateTask task) {
    if (!task.isValidate()) {
        return;
    }
    List<TaskData> taskDatas = loadTaskData(task);
    List<TaskKpiData> kpiData = calculateAllKpi(taskDatas);

    saveResult(kpiData, task);
}

Here, the code block is referred to as a method, not because there are too many lines of code, but because the function of this method is to describe the calculation process, not the details. So it's also unreasonable to write the code as follows

public void calculate(CalculateTask task) {
    if (!task.isValidate()) {
        return;
    }
    loadAndCalculateKpiAndSaveResult(task);
}

The logic of calculation should include four processes: verification, loading, calculation and saving. These four processes need to be directly reflected in the method, not much or less.

Let's take another example 🌰 the function here is to load invoice data from a file, group and save it to the database according to the invoice number, and save the corresponding range of the invoice into the warehouse. Clarify the demand point

  • Load invoice data from file

  • Group by invoice number

  • After grouping, invoice information is saved to the database

  • Save receipt of invoice corresponding range

    Let's see how it works

void saveFileToDb() {
    Stream<InvoiceInterfaceEntity> invoiceEntities = loadInvoiceFromFile(); 
    saveDataToDb(getSortedData(invoiceEntities));
}

public void saveDataToInterface(List<InvoiceInterfaceEntity> entityList) {
    entityList.forEach(entity -> {
        entityManager.persist(entity);
    });

    saveInvoiceRecorder(entityList);
}

private void saveInvoiceRecorder(List<InvoiceInterfaceEntity> interfaceEntities) {
    Map<String, List<InvoiceInterfaceEntity>> invoiceToEntityMap = interfaceEntities.stream()
            .collect(Collectors.groupingBy(InvoiceInterfaceEntity::getInvoiceId));

    getInvoiceRecorderEntityStream(invoiceToEntityMap).forEach(entity -> {
        entityManager.persist(entity);
    });
}

private Stream<InvoiceRecorderEntity> getInvoiceRecorderEntityStream(Map<String, List<InvoiceInterfaceEntity>> invoiceToEntityMap) {
  // Convert each group of invoice information in the invoiceToEntityMap to InvoiceRecorderEntity, which is the object used to record the invoice start Id and end Id
  return ...
}

private List<InvoiceInterfaceEntity> getSortedData(Stream<InvoiceInterfaceEntity> invoiceEntities) {
  // Sort invoice data by invoice number
}

It can be seen that the name here is readable, meaningful and short, but do you understand what it is doing? What's the problem?

Four requirements are scattered in different places, which leads to poor readability, which is a nightmare for maintenance personnel. Here are the results after improvement

void saveFileToDb() {
    loadInvoiceFromFile()
            .collect(Collectors.groupingBy(InvoiceInterfaceEntity::getInvoiceId))
            .values().stream()
            .forEach(invoiceGroup -> {
                InvoiceRecorderEntity record = saveToDb(invoiceGroup);
                entityManager.persist(record);
            });
}

private InvoiceRecorderEntity saveToDb(List<InvoiceInterfaceEntity> invoiceGroup) {
    String invoiceId = invoiceGroup.get(0).getInvoiceId();
    long minId = Long.MAX_VALUE;
    long maxId = Long.MIN_VALUE;

    for (InvoiceInterfaceEntity entity : invoiceGroup) {
        entityManager.persist(entity);
        minId = Math.min(minId, entity.getId());
        maxId = Math.max(maxId, entity.getId());
    }

    return new InvoiceRecorderEntity(invoiceId, minId, maxId);
}

Here, almost one line in the method saveFileToDb corresponds to one requirement, and the core logic of the whole function is clearly stated in one method.

Strong correlation parameter of transfer method

"Clean Code" said: the most ideal number of parameters is zero. Maybe someone read this rule and wrote down the following code.

The following code is used to read the price information from the smpfp page of the Excel file, and then convert it to the ServicePackage information

public Stream<XXXDto> getServicePackages() {
    Map<String, Optional<XXXDto>> prices = parsePrices();
    // ...
    return null;
}

private Map<String, Optional<XXXDto>> parsePrices() {
    return getSheet(() -> "smpfp")
            .getObjects(XXXDto.class)
            .stream()
            .collect(groupingBy(this::generatePackageIdentity,
                            reducing((l, r) -> l))
            );
}

private SheetAgency getSheet(Supplier<String> sheetNameSpplier) {
    return this.excelReader.getSheet(sheetNameSpplier.get());
}

Look at this code. Are you curious

  • Where does parsePrices analyze the price?

  • Why does getSheet need such a strange parameter?

I think the "ideal number of parameters is zero" in "Clean Code" means that the number of parameters should be as small as possible, but it's not that there's no choice to reduce the number of parameters. The method name is parsePrices, which means to parse Price information from something. Without the semantics of "something", it is not perfect. Therefore, the "something" must be embodied, and the Price here must also be returned. What's more, you can pass whatever parameters you need, and don't pass anything you don't need. The method getSheet here needs to specify the name of the Sheet page, so as long as the name is enough, how to generate the name is not important, which is equivalent to passing too much information to getSheet.

Here is the result of the modification

public Stream<XXXDto> getServicePackages() {
    Map<String, Optional<XXXDto>> prices = parsePrices(excelReader);
    // ...
    return null;
}

private Map<String, Optional<XXXDto>> parsePrices(ExcelReader excelReader) {
    return getSheet("smpfp", excelReader)
            .getObjects(XXXDto.class)
            .stream()
            .collect(groupingBy(this::generatePackageIdentity,
                            reducing((l, r) -> l))
            );
}

private SheetAgency getSheet(String sheetName, ExcelReader excelReader) {
    return this.excelReader.getSheet(sheetName);
}

Functions need to pass important parameters related to logic, not many, not many. Function names and parameters can express complete meanings.

Parameters: high and low

The world is not equal, it is a parameter, but the parameters are different.

The following method is a method to check whether the value is within the specified range. If it is not within the range, an error message will be recorded

private void checkValue(MessageRecorder messageRecorder,
                        Set<String> range,
                        String value,
                        ExcelRowWrapper<XXXData> row,
                        String title)

It will be more friendly after adjusting the order

private void checkValue(
        String value,       // The content to be detected is the core object
        String title,       // Title is not the core information, but it is the key subsidiary information of the core information
        Set<String> range,  // Data scope is secondary important information
        MessageRecorder messageRecorder,  // If there is an error, take part in the core operation
        ExcelRowWrapper<XXXData> row) {   // Perfect information at the end

There's nothing to say about this. Feel for yourself.

Forward naming

In refactoring, the first one about the bad taste of code is the mysterious name, which emphasizes that the name must be accurate. But is it easy to name exactly? Please see the following example

Case 1:

public boolean notValidated() {
    return date == null || !date.isValidated();
}

This is a method to check whether a Task is invalid. The naming is very accurate. When it can be seen that it is invalid, it returns true. The code used is as follows:

public void someFun() {
    ToDoItem item = new ToDoItem();
    if (item.notValidated()) {
        showRedColor();
    }
    //...
    
    if (!item.notValidated()) {	// What kind of logic is that?
        queryMoreDetail();
    }
}

Case 2: I want to make a tool to query database, and I want to provide a flexible structure to express conditions

public class StringEqualCriteria {
    private String field;   // Database fields to query
    private String value;   // Field needs matching value
}

// Query all completed tasks
queryTask(Arrays.asList(new StringEqualCriteria("status", "complete")));

With this StringEqualCriteria, it is much more convenient to query any combination of conditions. However, the requirements are always changing. One day, you need to add conditions that are not equal, so StringEqualCriteria becomes like this

public static class StringEqualCriteria {
    private String field;   // Database fields to query
    private String value;   // Field needs matching value
    private boolean isNot;  // Indicates whether the condition is reversed
}

To query all the unfinished tasks, there is such code

queryTask(Arrays.asList(new StringEqualCriteria("status", "complete", true)));

Does it feel strange? Do you feel like this?

new StringEqualCriteria("status", "complete", true);   // Feeling indicates that the state is complete
new StringEqualCriteria("status", "complete", false);  // Feeling indicates that the status is not completed

This code has appeared in my project, because it is a public tool, which is used in many places, and the meaning of parameters cannot be modified at will. Although it has led to several bugs, we still can't do anything about it.

 

One might ask: does StringUtils seem to provide the isNotEmpty method?

I want to say: whether the tool provides isNotEmpty or not, it must provide isEmpty.

Painful experience tells us that reverse naming can occur when naming method attributes, but methods or attributes with forward naming cannot be discarded. The simple rule is positive naming.

Reasonable annotation

Many books have bad comments on the notes

  • "Reconstruction" wrote

      If you need comments to explain what a piece of code does, try refining functions 
      If the function has been refined, but you still need comments to explain its behavior, try changing the function declaration 

     

  • "Clean Code"

     The proper use of annotations is to make up for our failure in expressing our intent in code. Note that I used the word "failure". I mean it. Annotation is always a failure.

     

So we now abhor annotations and ban all annotations.

But it's not noticed that "Reconstruction" has also been reversed for the annotation, which is said in the book

Don't worry, we're not saying you shouldn't write notes. In terms of smell, annotation is not only a bad taste, but also a fragrance.

"Clean Code" is more generous to give a whole chapter of notes, explaining how to use notes in detail. The first sentence of Chapter Four:

Nothing is more useful than putting good comments.

In particular, it was emphasized that:

Notes are not like Schindler's list. 

Some recommended annotation scenarios in 4.3 good annotation of Clean Code

  • Legal information

    In the company specification requirements are added

  • Interpretation of intention
private static synchronized String getUniqueTail() {
    // Reset the counter if it is greater than 99999
    // And ECC agree that the number changes from 0-99999. This is the explanation of intention
    if (counter > 99999) {
        counter = 0;
    }
    counter = counter + 1;
    return Long.toString(System.currentTimeMillis()) + "_" + counter;
}
  • warning
// Simple DateFormat is not thread safe
// So we need to use this cache in single thread
// We must not add 'static' qualifler
private Map<String, DateFormat> formatMap = new HashMap<>();

public static <T> Long newVersion(ESDocumentWrapper<T> wrapper) {
    // When we don't find doc in es, It does not mean the doc never appear,
    //  when the doc appear in es sometime, the doc version will can't start from 1
    //  So we return null when not find doc.It may be overwrite doc sometime.
    return (wrapper == null) ? null : wrapper.getVersion() + 1;
}
  • TODO notes

It's used by everyone

SearchRequestBuilder requestBuilder = client.prepareSearch(indexName)
        .setSize(10000) // TODO: it should be read in Scroll mode. The current data volume is less than 10000. You can use this temporary scheme
        .setQuery(queryBuilder);
  • Java Doc of public API

If the code is a public library, remember to add comments

/**
 * Group by Key
 * The previous Iterator must be sorted by key
 * @param keyGenerator key How to get
 * @param <K> key Types
 * @return Entity iterators grouped by key
 */
public <K> EasyIterator<Map.Entry<K, List<T>>> groupBy(Function<T, K> keyGenerator) {
    return new GroupSortedListIterator<>(this, keyGenerator, v -> v);
}

/**
 * Implement the logic of conversion between a data type and a string
 * @param <T> converter Data type responsible for processing
 * @param <S> Subclass type, used to return itself without cast
 */
public interface IPrimitiveConverter<T, S extends IPrimitiveConverter<T, S>> {
    /**
     * Returns all supported data types. This type will be used for registration. If the type is repeated, the Converter corresponding to the type will be registered
     * @return All supported data types
     */
    Class[] dataTypes();

Annotation is not the source of all evils. When necessary, you should do it.

Whatever the notes, make sense. Meaningless comments must be deleted.

Comments don't beautify bad code. If there is a way to express information through code, try to use code.

When the code is not clearly expressed, make sure to add comments. Code with comments is better than code with unclear expression.

Public code should have good comments.

summary

  • A function should have a good description of its own functions, not many, not many.

  • Functions need to pass important parameters related to logic, not many, not many. Function names and parameters can express complete meanings.

  • The parameters are classified into high and low. Arranged according to semantics and relevance.

  • Function or variable needs positive naming.

  • The tool class needs to be annotated, preferably with a demo.

Reference resources

Alibaba specifications: https://alibaba.github.io/Alibaba-Java-Coding-Guidelines/

Google specification: https://google.github.io/styleguide/javaguide.html

Published 5 original articles, won praise 4, visited 97
Private letter follow

Tags: Database less Java github

Posted on Sun, 12 Jan 2020 10:19:58 -0500 by eyekkon