2018/04/26 Corrected the heading and text of "Trying to make anything constant" with reference to the comments. Thank you, @kagilinn. 2018/04/30 There was a judgment bug in the sample code, so I fixed it. Thank you for pointing out, @y_miz. Corrected typos in comments and terminology. Thank you @scivola for your editing request.

Create unnecessary instance variables

Since instance variables retain their state, it is easy to create bugs. It was often pointed out that "Is this a local variable?"

** Before creating an instance variable, consider whether it can be realized with a local variable. ** **

Attempts to declare all variables at the beginning of the method

It seems to be a common habit for people who entered from C language. Tracing is very cumbersome because the scope of variables expands unnecessarily.

bad example

public String function() {
    int count;
    int max;
    int min;
    List fooList;
    String hogeCode;
    String fugaCode;
    String ret;
    
    //Various processing

    return ret;
}

** Be aware of the scope when declaring variables, and make them when needed. ** **

Give an insufficient variable name

There is a beginner. Even veterans have surprisingly suitable people. Beginners tend to do things like omitting the model name or serial numbers that only the person who wrote them knows.

bad example

String str;  //I only know that it is a string

String code; //I don't know what the code is

int a;       //Terrible but rarely seen

File file1;  //Mysterious serial number
File file2;

static final String MSGID_E0001 = "E0001"; //There is a value in the constant name

Good example

String userName;

String messageCode;

int age;

File userListFile;
File companyListFile;

static final String MSGID_FILE_NOT_FOUND = "E0001";

However, it does not mean that you should give a variable name with plenty of qualifications for everything. Sometimes a short name is fine, such as a for statement counter or a caught exception.

** How detailed the variable name should be depends on the length of the scope. ** ** It's a good idea to give detailed names to instance variables and local variables used in long methods. Conversely, if the scope of the variable is limited to a few lines of blocks, a short variable name is fine.

The important thing is to be able to understand "what is stored in that variable" from the standpoint of the code reader (including myself a few months later).

Variable naming seems easy and in a very deep world, The famous book "Readable Code" also uses a considerable number of pages for variable names.

Use xxxflg for boolean variable name

The guy who didn't feel any discomfort until he was pointed out. With xxxFlg, it is difficult to understand what happens when it is true.

bad example

private boolean writeFlg = false; //True in what case/It is unclear whether it will be false

Good example

private boolean writable = false;

** Make boolean variable names propositional. ** ** When you write variable name == true, it is desirable that it can be understood as a sentence.

Method name example

public boolean isWritable() {
    return writable;
}

By using the method name as described above, the instance becomes the subject when calling, and the meaning is easy to understand as an English sentence.

if (note.isWritable()) { /* ... */ }

Some of the most commonly used method names are:

--is + adjective --can + verb prototype --has + past participle --Three simple verbs (+ nouns)

[Reference] An article summarizing the naming of booleans from the perspective of English grammar http://kusamakura.hatenablog.com/entry/2016/03/03/boolean_値を返却するメソッド名、変数名の付け方

Constantize without giving meaning

I was pointed out that "literal solid writing is useless", so this is an example of making various constants. The constants have no meaning, they just replace the letters.

private static final String HANKAKU_SPACE = " ";
private static final String BLANK = "";
private static final String COMMA = ",";
private static final String SLASH = "/";
private static final int ZERO = 0;

Always try to return at the end of the method

When I was first pointed out, I didn't understand what was wrong, I understood well when I was in a position to read.

bad example

boolean isPrimeNumber(int num) {
    boolean ret;
    if (num < 2) {
        ret = false; //Less than 2 is not a prime number
    } else if (num == 2) {
        ret = true;  //2 is a prime number
    } else if (num % 2 == 0) {
        ret = false; //Even numbers other than 2 are not prime numbers
    } else {
        ret = true; //Prime number if not divisible
        double sqrtNum = Math.sqrt(num);
        for (int i = 3; i <= sqrtNum; i+=2) {
            if (num % i == 0) {
                ret = false;   //If it is divisible, it is not a prime number
                break;
            }
        }
    }
    return ret; 
}

Good example

boolean isPrimeNumber(int num) {
    if (num < 2) {
        return false; //Less than 2 is not a prime number
    }
    if (num == 2) {
        return true;  //2 is a prime number
    }
    if (num % 2 == 0) {
        return false; //Even numbers other than 2 are not prime numbers
    }
    double sqrtNum = Math.sqrt(num);
    for (int i = 3; i <= sqrtNum; i+=2) {
        if(num % i == 0) {
            return false;   //If it is divisible, it is not a prime number
        }
    }
    return true; //Prime number if not divisible
}

Consider the case where a third party traces the case where 1 is entered in num.

In the "bad example", the reader has to remember the state of ret and continue reading until the final return because he does not know where the ret is rewritten.

On the other hand, in the "good example", each judgment returns on the spot. When tracing the case of num = 1, read up to return on the 3rd line.

This example is a method with more than 10 lines, but if you implement a method with 50 lines and 100 lines like a "bad example", the burden on the reader is immeasurable.

** The method should return when the return value is fixed **

In addition, returning quickly has the advantage that it is difficult for the nest to deepen.

Try to add public unnecessarily

As a beginner, I tend to make private variables and methods public. First, make it private, and if necessary, expand it to protected and public.

Easily create god classes such as xxxUtil, xxxHelper, xxxManager

Classes with unclear names are dangerous. It's good at the time of making it, but as it is maintained for 5 years and 10 years, the functions will be pushed ~ ~ and added ~ ~ and it will become untouchable.

The more abstract the xxx part, the worse. The worst is CommonUtil. I've seen CommonUtil made over 10 years ago, and it's over 4000 lines.

First, consider whether you can change the name to something that limits your role, such as Factory, Builder, Writer, Reader, etc.

Do not write any comments

I was desperate to implement it, so I was angry if I took it to a code review without writing any comments.

Let's write comments properly.

In particular, the following should be supplemented with comments.

--Esoteric processing and complicated conditional branching --Special circumstances and intentions that cannot be read from the code

Write trivial information as a comment

After being pointed out, "Write a comment," I wrote a line-by-line comment, which was also pointed out.

If you write a comment line by line that looks like a Japanese translation of the code, it will be noisy. Since the number of effective lines that fit in the screen is reduced, the visibility is also poor.

bad example

//Get user ID
String userId = user.getId();

//Add user ID to list
userIdList.add(userId);

Not considering 0 loops

When implementing loop statements such as for statements and while statements, we did not assume a case where the loop did not rotate even once, and there were times when bugs were discovered in the 0-case pattern of unit tests.

Foo foo = null;
for (int i=0; i < hogeList.size(); i++) {
    if (i == 0) {
        foo = new Foo();
    }
    //processing
}
foo.function(); //NullPointerException occurs when loop is 0 times

** When implementing a loop, assume 0, 1 or multiple patterns **

Sloppy method return / exception design

Despite the public common methods The specifications of arguments and return values were ambiguous.

For example, the following should be designed without omission and described in Javadoc.

--What to return if null or negative number is given as an argument ――When and what exception to throw ――When to return null as a return value

If you want to know a model example of Javadoc, you should take a look at Javadoc of Oracle. The familiar String class method will be easy to understand.

at the end

In order for new programmers to be able to write good code, not to mention their own efforts, I think it's important to get code reviews from good programmers. After receiving the advice from the reviewer, "I would write this for myself," I got the awareness that "Oh, is there such a thing?" This is because the know-how will be absorbed through the accumulation and it will be possible to write better programs.

There are pros and cons to using code reviews as a place of education, In my personal opinion, code review can be a study session as well as a quality improvement effort. I get suggestions and advice from other programmers for the code I wrote, and sometimes discuss it. Such an opportunity is unlikely except for code reviews. It's too wasteful to just point out a bug.