Review points for 2019 new employee training

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.


Prerequisite information

Below, we will describe the information that is a prerequisite for the circumstances under which the review was conducted.

About the sample application

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

Configuration management tool

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

Static analysis tool

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.


Program review issues

I pointed out the following.


** Replace with logger instead of using System.out. ** **

   @Override
	public void getDelItem(TestDTO dto) {
		System.out.println("###Flow dto.getItemNumber() = " + dto.getItemNumber());
		getItemPrimitive().delItem(dto);
	}
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.


Wicket initialization process is implemented in ʻonInitialize ()` instead of the constructor of Page class and Panel class.

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 + "');");
    }
 /**
     *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 Java 7, you can use the diamond operator.

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<>();

Define only the necessary variables as member variables.

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

}
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);

      ...
   }

The switch statement defines the default clause.

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;
	}

	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

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.


Use camel case for variable names unless there are restrictions

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 = "";

Delete the commented out line of the program

Delete the commented out line of the program. The reasons why you should delete it are as follows.


Delete unused variables

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


Consider whether label tag can be added to input tag

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>

What to think after conducting a program review

Describe your thoughts through the program review. It's also called a mere worry.


Should I teach best practices or anti-patterns?

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.


There is a level where you do not understand the warning content of static analysis tools.

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.


Make a bad program by diverting a bad program.

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.


Finally

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.

Recommended Posts

Review points for 2019 new employee training
Code review points
Points for refactoring (if statement)