At our company, we will create a sample application for about January as part of the training for new engineers. The scale of the sample application is about 6-7 screens, and during the creation period, a senior engineer who has been committed to configuration management on a daily basis and has become an educator will conduct a code review.
In this article, I will pick up and introduce some of the contents pointed out during the code review, and describe what I thought after conducting the program review as an improvement point from next year onwards.
Below, we will describe the information that is a prerequisite for the circumstances under which the review was conducted.
The components of the programming language, server, etc. used in the sample application are as follows. Version information is omitted.
-** Programming language ** Java
-** J2EE server ** Glassfish
-** Web framework ** Apache Wicket
I'm using Subversion. Vesion is as follows.
svnadmin --version
svnadmin,Version 1.7.14 (r1542130)
Compile date and time: Nov 20 2015, 19:25:09
Get source code from Subversion on SonarQube Check daily. When conducting the review, the static analysis results will also be referred to as reference information.
I pointed out the following.
logger instead of using System.out. ** **System.out to output the log. @Override
public void getDelItem(TestDTO dto) {
System.out.println("###Flow dto.getItemNumber() = " + dto.getItemNumber());
getItemPrimitive().delItem(dto);
}
** Reasons not good **
I am using System.out.println for debugging, but since System.out.println is output to the Server.log of the AP server (Glassfish), starting and stopping the AP server itself, It is mixed with the error information.
Depending on the character string to be output, the log monitoring system may malfunction.
** Good example ** Use logger to specify the log level and output the log.
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
private Logger logger = LoggerFactory.getLogger(this.getClass());
...
@Override
public void getDelItem(TestDTO dto) {
logger.info("###Flow dto.getItemNumber() = " + dto.getItemNumber());
getItemPrimitive().delItem(dto);
}
The Logger library manages the log output destination in the configuration file. Depending on the settings, you can output only to the log file, output only to the console, output to both the log file and the console, etc. If you are using Logger, even if you keep the log output for debugging, you can change the configuration file so that the production environment does not output the log.
Starting with Wicket 1.5x, it is recommended to implement the initialization process in onInitialize () instead of the constructor.
/**
*Constructor
*
* @param params input parameters
*/
public PracticeMyPage(PageParameters params) {
super(params);
super.renderJavaScriptFile("PracticeMyPage.js");
add(new AbstractMyPage("MyPagePanel", params) {
@Override
protected AbstractMyPageMediator getMyPageMediator() {
return kikakuMyPageMediator;
}
});
String serverDate = DateUtil.dateTimeToStr(DateUtil.getSystemDate());
this.setBodyOnLoad("setServerDate('" + serverDate + "');");
}
** Reasons not good **
This is because there are many restrictions on the implementation order, such as the constructor must call super () first.
** Good example **
/**
*Constructor
*
* @param params input parameters
*/
public PracticeMyPage(PageParameters params) {
super(params);
}
/**
*Initialization process
**/
@Override
public void onInitialize() {
super.renderJavaScriptFile("PracticeMyPage.js");
PageParameters params = super.getPageParameters();
add(new AbstractMyPage("MyPagePanel", params) {
@Override
protected AbstractMyPageMediator getMyPageMediator() {
return kikakuMyPageMediator;
}
});
String serverDate = DateUtil.dateTimeToStr(DateUtil.getSystemDate());
this.setBodyOnLoad("setServerDate('" + serverDate + "');");
//Parent on Initialize()Call may be at the end of the process
super.onInitialize();
}
Starting with Java7, you can use the diamond operator. Some code is written in Java 6 or earlier, and some code does not use the diamond operator, but if you create a new one, let's actively use it.
List<String> strings = new ArrayList<String>();
Map<String,List<Integer>> map = new HashMap<String,List<Integer>>();
List<String> strings = new ArrayList<>();
Map<String,List<Integer>> map = new HashMap<>();
** Supplement ** Features related to the diamond operator include: When Java is upgraded, it can be implemented with a new syntax, so collect information regularly.
[New releases of Java 8 (14) Other new features (1) --Useful utilities, improved generics | Mynavi News](https://news.mynavi.jp/article/20140311-s_java8/ 14) In Java8, method type parameters can also be omitted.
Java var memo (Hishidama's Java var Memo) Java 10 introduced the var variable.
I think that there are many patterns in which the description of the diversion source source remains as it is, but variables that do not need to be member variables are defined as member variables.
public class AbstractRegistItem extends BaseNewUIPanel {
/** Form */
private HImForm<Void> form = null;
/**Product code*/
private HImTextField<String> code = null;
/**Product name*/
private HImTextField<String> syouhin = null;
/**Quantity/unit*/
private HImTextField<String> num = null;
......
}
** Reasons not good **
If the variable definition and usage are separated, the readability of the program will decrease.
** Good example **
public class AbstractRegistItem extends BaseNewUIPanel {
/**
*Page initialization process
*/
private void initPage(PageParameters params) {
/** Form */
HImForm<Void> form = new HImForm<>("form", this);
//1st text box
HImTextField txtItemName = new HImTextField<>("txtItemName");
form.add(txtItemName);
this.add(form);
...
}
Be sure to include the default clause in the switch statement.
switch(dto.getSelectedValue()){
case "1":
sbSql.append("UPD_DATE DESC ");
break;
case "2":
sbSql.append("UPD_DATE ASC ");
break;
case "3":
sbSql.append("ITEM_NUMBER DESC ");
break;
case "4":
sbSql.append("ITEM_NUMBER ASC ");
break;
}
** Reasons not good **
In the maintenance phase after implementation, it is not possible to determine whether the absence of the default clause is an omission of implementation or whether it is intentionally done.
In my experience, bugs that missed the implementation of the default clause sometimes appear, which can be a sad memory.
** Good example **
switch(dto.getSelectedValue()){
case "1":
sbSql.append("UPD_DATE DESC ");
break;
case "2":
sbSql.append("UPD_DATE ASC ");
break;
case "3":
sbSql.append("ITEM_NUMBER DESC ");
break;
case "4":
sbSql.append("ITEM_NUMBER ASC ");
break;
default:
//It's close to the case, but throws an exception if the value can never be set.
throw new llegalArgumentException("dto.getSelectedValue() is illegal...");
}
Delete unused import statements as they will be noisy during the maintenance phase after implementation.
Eclipse on Windows allows you to organize shortcuts ctrl + shift + o imports.
For variable names, use the camel case specified in the coding standard unless there are restrictions on the framework.
private String SelectedValue = "";
private String selectedValue = "";
Select target variable> Right-click> Convert to display a list of standard conversion processes that can be performed in Eclipse.Delete the commented out line of the program. The reasons why you should delete it are as follows.
Delete unused variables. This is also because it causes noise when investigating the program. SonarQube outputs the following warning.
Remove this unused "selLineCnt" private field.
If for some reason you need to keep the variable, you can suppress the warning by giving @SuppressWarnings ("unused") .
This is an HTML point. A pair of label attributes can be added to the input tag. If there is information that can be labeled, add a label so that the input attribute can be focused by clicking the label.
<th colspan="2">
<p class="disp-ic"><img height="13" width="23" class="ic-required" alt="Mandatory" src="/web2/jp/images/ic-must.gif"></p>
Product code
<div class="letter-control">(Within 20 single-byte characters)</div>
</th>
<td>
<span class="cstm-tx">
<span class="cstm-tx-bg">
<input wicket:id="txtCode" type="text" maxlength="20" rel="Within 20 single-byte characters" class=" w100 noticeTip ime-actv" id="txtCode" name="c1-29" tabindex="40" />
</span>
</span>
</td>
<th colspan="2">
<p class="disp-ic">
<img height="13" width="23" class="ic-required" alt="Mandatory" src="/web2/jp/images/ic-must.gif">
</p>
<!--Label the product code-->
<label for="c1-29">Product code</label>
<div class="letter-control">(Within 20 single-byte characters)</div>
</th>
<td>
<span class="cstm-tx">
<span class="cstm-tx-bg">
<input wicket:id="txtCode" type="text" maxlength="20" rel="Within 20 single-byte characters" class=" w100 noticeTip ime-actv" id="txtCode" name="c1-29" tabindex="40" />
</span>
</span>
</td>
Describe your thoughts through the program review. It's also called a mere worry.
Program reviews should do this for anti-patterns. I felt that there were few opportunities to teach that it was better to write like this. I think that if you proceed with implementation with pair pro and mob pro and explain the part that you do not understand the meaning of implementation, you will have an opportunity to teach best practice system. Also, I wonder, "Which of the best practices or anti-patterns is faster to grow?", But no answer has been given at present.
This was a big realization for me personally. I think static analysis tools are the best tools for self-learning anti-patterns. In the training for newcomers, the warning content is translated into Japanese for some people How to use the SpotBugs Eclipse plugin — spotbugs 4.0.0-beta4 document I had .html) installed, but I don't know what to do by looking at the contents of the warning. So I decided not to see it. I was in a situation of `. In this case, it may be necessary to teach how to read the content of the warning, have them learn information processing-related vocabulary, and teach how to look up if they do not understand.
The training application is created by diverting the code of the service that is actually in operation and converting it into a skeleton. Due to circumstances during development, there were cases where the quality was stable and some were not stable, and the review pointed out that the diversion source was not good. In terms of teaching the ideal, I thought it would be better to improve the quality of the training application.
In next year's training, I would like to take advantage of this year's failure points and carry out a better review. I have done pair pros and mob pros several times, but I would like to increase the number a little more.
that's all.