[Java] Really scary switch statement story

Introduction

A long time ago, I summarized the story when I messed up with a switch statement. It's a very embarrassing story, but I've put together an article to prevent a similar accident from happening again.

1. Initial state

• Like the following TestServlet.java, the switch statement was used for the purpose of distributing the processing based on the value of the request parameter.
• Actually, the number of conditional branches was close to 10, so I adopted the switch statement instead of the if statement.

TestServlet.java

@WebServlet("/TestServlet")
public class TestServlet extends HttpServlet {

	private static final long serialVersionUID = 1L;

	protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

		//Request parameter"id"Get the value of.
		int id = Integer.parseInt(request.getParameter("id"));

		// "id"Processes are distributed based on the value of.
		switch(id) {
		case 1:
			//Process 1-A
			break;
		case 2:
			//Process 2-A
			break;
		case 3:
			//Process 3-A
			break;
		default:
			//Default processing
			break;
		}
		...
	}
}

2. Renovation due to additional functions

• As the processing in the switch statement increased with the addition of functions, the switch statement became bloated as a whole as shown below.
• At this time, while writing and erasing the code repeatedly, ** it seems that I accidentally erased one break **.
• I didn't notice that I deleted the break because the switch statement was bloated ...
• ** By the way, deleting break does not cause a compile error! ** **

TestServlet.java

@WebServlet("/TestServlet")
public class TestServlet extends HttpServlet {

	private static final long serialVersionUID = 1L;

	protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

		//Request parameter"id"Get the value of.
		int id = Integer.parseInt(request.getParameter("id"));

		// "id"Processes are distributed based on the value of.
		switch(id) {
		case 1:
			//Process 1-A
			//Process 1-B
			//Process 1-C
			break;
		case 2:
			//Process 2-A
			//Process 2-B
			//Process 3-C (★ I erased the break immediately after this...）
		case 3:
			//Process 3-A
			//Process 3-B
			//Process 3-C
			break;
		default:
			//Default processing
			break;
		}
		...
	}
}

3. Accident occurred

• Some time after releasing the code with added functions, the user inquired that "the system behaves strangely".
• I should have passed all the tests from the unit test to the system test, but when I looked into it in detail, I found that the processing result was strange.

4. Reason for the accident

• At the point where break disappeared from the switch statement, a totally unintended ** "fallthrough" ** occurred.
• As mentioned above, the disappearance of break does not cause a compile error, so if you do not read the code carefully, you will miss it ...
• The reason why it passed the test is that "the test did not cover the conditional branching".
• However, creating a test that covers all conditional branches can be a daunting task ...

5. What to do after the accident

• I don't think it's the best response, but I said "don't use the switch statement unnecessarily".
• Since the cause of this accident is "a switch statement that has become bloated and has poor visibility", I made my rule that "when using a switch statement, it is assumed that the entire switch statement is slim and has good visibility".
• In the first place, "code with poor visibility" itself is a problem, but in order to prevent "unintentional fallthrough", I thought it was safer not to use the switch statement carelessly. (Maybe "switch statement allergy" ...)
• I tried to think more than ever before making unit tests.
• Even with minor modifications, we have come to devise ways to eliminate missing test patterns, such as using the decision table to identify test patterns. (It seems that you are too low in consciousness ...)
• At that time, objective indicators such as checking the coverage rate at the branch level with Cobertura etc. came to be used.

Reference URL

• Dangerous use of switch statement
• I met a great switch statement