The content is as the title says, but I have no intention of taking down anyone who wrote anti-pattern code. There is also the word that you hate the code and not the people. [^ code] [^ code]: I really found it when I googled it.
Instead of such a negative motive, I am writing with a positive motive to draw as much lessons as possible from anti-patterns and move forward.
There are a lot of old stories on the whole, so I'm hoping that there are fewer such codes and situations these days.
As many of you may know, Struts 1 was out of support many years ago, so at this point I have only a bad feeling.
It's still fine if you use Struts as it is, but as you can see from the heading, I used the oleore framework that extends Struts. In that framework, it is necessary to create a large number of classes to write the processing of one request, Struts Action and FormBean, BLogic that describes business logic, BLogicInputBean for input to BLogic, BLogicOutputBean for output , JSP, and in some cases DAO and DAOBean to pass to DAO were also needed. Besides, struts-config, Struts validation, Spring definition, iBATIS sqlMap and XML file modification are also included.
In particular, it was a mystery that I had to create many kinds of similar beans, but I had no choice but to create those beans as a framework convention, and the contents are almost the same, but they are different types, so It took me a while to write the refills manually. [^ bean] [^ bean]: Later, I found out that there is a library that copies the contents of beans that are similar but of different types, which made that a little easier. Moreover, since these are created "for each request", it was necessary to create the above for each of the display request, new registration / update request, request to update some items on the screen, etc. .. Anyway, the amount of correction was large, so the productivity was very low.
At that time, there weren't many powerful frameworks to replace Struts, so I think there was no choice but to adopt Struts to some extent. However, it is a mystery that I modified it to force the creation of a large number of unnecessary classes in plain Struts. I think I should have used the plain Struts as it is. A quick look reveals that such frameworks aren't about increasing productivity, they're about getting things done even with low developer skills. [^ invade] [^ invoke]: Reference: Invasive Framework
Perhaps they adopted such a framework and assigned newcomers with low unit prices to reduce costs. (Actually, there were many newcomers) If the choice of this framework is for that political reason, it can't be helped anymore. I can only think of a way to deal with it by running away. If you have high technical skills, you may be able to do your best technically like the reference link, but I couldn't think of it at that time. I may not be a decent programmer.
For the time being, there's no reason to choose Struts for new development. I wonder if Spring Boot is good after all.
Copy programming is what you end up with when you create a function that is similar to an existing function. It is that you copy and paste the existing code and correct only the difference. I can't say it's great because I sometimes do it, and I don't mean that copy programming is always bad, but I have to point out that copy programming still has its drawbacks.
Copy programming gives you some pleasure because you can quickly implement functionality even with code that you don't understand well, and at first glance it looks more productive, but during maintenance. See hell. First of all, if there is any modification, you have to make the same modification for all the copied parts. Besides, there is a risk of causing strange bugs due to omission of correction when copying. Code that is assembled at once even though you do not understand the contents well by copying is very difficult to read, so it tends to be sloppy to check, and correction omissions often occur. Missing code fixes is a problem, but missing comments is also a problem. In a sense, it may be worse than a code omission, as a comment omission does not affect behavior.
Copy programming is rampant in the projects I've seen, and I suspect that most of the coding was done by copy programming. There were so many omissions in the project that the comments weren't credible anymore. That is the same as no comment. You probably know the pain of maintaining code with no comments at all.
If it was the same process, it should have been extracted and shared in a method etc. instead of copying. In that case, even if a correction occurs, you only have to fix it in one place, and since it is not copied in the first place, there is no omission of correction. However, conversely, if you modify one part, it will affect multiple parts, so you should carefully consider "Is it really okay to share?" [^ share] [^ share]: Reference: [Share carefully](https://xn--97-273ae6a4irb6e2hsoiozc2g4b8082p.com/%E3%82%A8%E3%83%83%E3%82%BB%E3%82% A4 /% E5% 85% B1% E6% 9C% 89% E3% 81% AF% E6% 85% 8E% E9% 87% 8D% E3% 81% AB /)
It is the root of all evil. It's a notorious guy you've probably heard of. It's fine to comment out temporarily, but what I'm saying here is to leave all fixes permanently commented out. In addition, the project had to insert a comment to indicate the start of the modification and a comment to indicate the end of the modification as a rule for modification. It's very annoying because every time you make a correction, the amount of garbage increases. It's very hard to read and it can be a noise for your search. The code is only getting dirty as all the fixes remain. Where there are many corrections, it is too chaotic to read. Needless to say, reading code is a very frequent task, and this was also one of the causes of lost productivity. This is what it means to have a lot of harm and no profit.
It can be SVN or Git, but you should use some version control system. [^ version] [^ version]: Well, unless you have some special circumstances, you should choose Git. With a version control system, you don't have to do this at all. At this time, no project is being developed without a version control system. …But it is not………? However, in that project, even though version control was done with SVN, it was the above rule, so it was completely unclear ...
There were often situations where a large amount of business logic was written in JSP. It was very hard to read because it was a mixture of HTML and Java. Also, since the amount of code described in one JSP file was too large and an error occurred [^ jsp_error], the JSP was divided on one screen, which was also difficult to read. It's okay to divide it into appropriate units, but in this case it was a method of cutting a huge JSP into two or three and assigning serial numbers to the file names, so what is division in appropriate units? It was hard to say. [^ jsp_error]: Reference: I want you to solve the problem that cannot be executed immediately if the compilation result exceeds 65535 bytes in one method which is a limitation of java.
Also, in order to implement that the screen display changes depending on the condition, I honestly wrote everything using conditional branching. This also caused the amount of code to swell and become difficult to read.
Write your business logic in your model. The basics of MVC. Also, if the screen display changes depending on the conditions, you should take measures to prevent one file from becoming bloated, such as creating a sub JSP and loading it.
By the way, I don't think there are many new developments that use JSP now. After all, Thymeleaf is the mainstream right now.
Since there is no curse on the untouched code, even if there is a problem with the code, it should not be touched unless there is a problem with the operation. As a result, minor problems were not fixed, and the number increased steadily. Even if there was a warning, it could not be corrected, so the warning was accumulated in the pool. This is not very good, as a large number of warnings will bury the really problematic warnings.
Also, a large number of TODO comments were left unattended. If it is left as TODO for a long time, it will be difficult to know how to handle it. It is also an obstacle because the TODO that I added personally will be buried.
I should have taken some time to refactor. It sounds correct at first glance to not do it because you don't have time, but it's not. There's a lot to do, so it's unlikely that you'll have time naturally. You can't do time without thinking about adjusting other tasks to make time. What is not is motivation, not time. [^ time] [^ time]: It was true that I didn't have time because everyone was working until about 22:00 every day regardless of the time of year and sometimes I had a day off. However, it was an inefficient development method as a whole, some people took a rest frequently due to poor physical condition, some people did not come back for about 30 minutes after leaving their seats, etc. I think it was possible to find time if I wanted to.
There is also a story that I hate it because I have to test the code when I touch it. I know that, but there are times when you need to test and refactor. Also, when I fix a code by fixing a bug or adding a function, I have to test it anyway, so I think that I could and should have devised a refactoring etc. if I wanted to. It seems that a small modification can be secured by executing UT, but in this project I did not write UT at all in the first place. First of all ...
However, as long as there is a [Comment out the corrections and leave all](#Comment out the corrections and leave all) rule, there is no refactoring or shit. At a minimum, this rule must be destroyed at all costs.
In general, it is good to treat fixed values that appear many times as constants, but sometimes a constant that makes no sense is created without understanding its intention.
public static final int INT_0 = 0;
public static final int INT_1 = 1;
//The same thing continues below
This is no different from using literals. It's a mystery, but the person who wrote this may have been imprinted by the wrong education that using literals directly is evil. I intended to think of the term constant disease myself, but it seems that some people have already thought about it. It's exactly like this. [Source] "DIV disease" is synonymous with being dangerous if you overdo everything
I don't think it's necessary to explain much, but I think that constants are originally used for values that may change in the future. By defining a constant and referencing the constant where it is needed, even if something changes, you only have to modify the initialization part of the constant. Also, due to the nature that it may change in the future, the constant name should be named based on what kind of value it means, and based on what kind of value is specifically included. The name is nonsense. Here is a simple example.
//Bad example
public static final String LF = "\n";
//Good example
public static final String LINE_SEPARATOR = "\n";
If it's a bad example, you'll be in trouble if it changes in the future. As shown in the example below, use a name that does not make you feel uncomfortable even if the value changes.
Also, if the value is unlikely to change in the future, I think you can use it as it is without forcibly using a constant. Here is another example.
public static boolean isEven(int num) {
return num % 2 == 0;
}
As you can see, it is a code that determines whether the number is even. I'm using numeric literals like "2" and "0" as they are, but in this example it's unlikely that these numbers will change in the future. Therefore, I don't think there is any particular problem if you use literals as they are.
In places where extended for statements can be used, I often see code that works hard with ordinary for statements. If you don't need a loop counter, use an extended for statement. It's easier to write, has less room for bugs, and is easier to read. I can't think of a reason not to use it when it can be used, but you probably don't know its existence. There is such a person! ?? You might think, but I think it's normal. But as I wrote at the beginning, I have no intention of taking them down. If you don't know, you just need to know from now on. Also, if you are using Java 8 or later, consider using the Stream API.
It's common to give classes a name that identifies their responsibilities, but in one project all classes were named by feature ID. This is an example that I thought about properly, but for example, "AP060.java", "AP060BLogic.java", "AP060Bean.java", and so on. With this, I have no idea what each class is. Well, if you remember which function ID is what function, it's surprisingly okay, so I thought that human beings are amazing, but in general, recognize that it is an abnormal naming convention. is needed.
You don't need to write this either, but give it a name that tells you what the class is. Name important.
I've also often seen code where the number of lines in an if statement is too long to understand what is written where. Hundreds of lines and processing are written in the if statement, and when I think that it is over, it seems that similar code continues endlessly in else this time. Most of the branching conditions are the same, such as for new registration and update, but there are many cases where the values are different. Use if statements only for differences ...
I wrote a little, but if it is the same process, you should put it out of the if statement and conditional branch only the difference. Also, even if it is a meaningful conditional branch, it is still painful to be long, so in such a case, I would like to pursue readability with a little ingenuity, such as cutting out the processing in the if statement to a method and giving it an easy-to-understand name. .. In some cases, you may want to consider whether polymorphism can be used to eliminate the conditional branch itself.
I often see code like below that defines a local variable at the beginning of a method, but actually uses that variable. If you don't do that in C (an older version), you'll get a compile error, but Java doesn't have that limitation, so it doesn't make any sense. It just makes it difficult to understand the definition part of the variable and the part to be used. In addition, there were some variables that were purposely defined as fields even though local variables were sufficient. If it is a field, it may be read and written by other methods, so it will be difficult to follow the transition of the value.
Let's define local variables when we need them. Also, define variables that are used only within one method as local variables, not fields. Well, the IDE may warn you around here.
It's a pseudo code, but I often see code like this. Do you know what's wrong?
ResultSet rs = null;
try {
for(HogeBean bean : beanList) {
//Processing using beans...
rs = stmt.executeQuery();
//Processing using rs...
}
} finally {
if(rs != null) {
rs.close();
}
}
Well, as you might guess from the heading, the resource release isn't done properly. Since we are reusing the variable rs in the loop, close () is only called properly in the last one.
Make sure that close () is called for everything, such as by putting try-finally in the for statement. In addition, use try-with-resources for Java 7 and above. Well, I think this is detected by static analysis tools.
Indentation is important. Sources with proper indentation are really hard to read. Especially when if statements and for statements are nested. I often saw terrible code, such as indenting in an empty space, or not indenting where it was needed. It was really hard to read in combination with the above [The code in the if statement is too long](The code in the #if statement is too long). Perhaps the person who wrote it is also confused. Hundreds of lines of code are the culprit ... But that's not all. There is a problem [Comment out the corrections and leave them all](#Comment out the corrections and leave them all). Most members commented out by adding "//" to the first line, but if you do this, the commented out code will be slightly behind. This was also a source of confusion. In addition, I would like to fix it as much as indentation, but I felt that it was difficult to fix it because of the rules left in the comment out. Somehow, multiple causes were intertwined and it was terrible, and I could only sigh.
It's a deep-rooted problem, but the first thing we need to do is eliminate comment-out rules and reduce hundreds of lines of code. If I can do that, I think the indentation will be corrected naturally.
I've often seen code that uses StringBuffer instead of StringBuilder to concatenate strings, where thread safety isn't required. This is also the same as [Do not use extended for statement](#Do not use extended for statement), and you probably do not know the existence of StringBuilder. Well, that didn't cause any performance issues, so maybe it doesn't matter.
==This is a problem that can lead to nasty bugs. Since the references are compared between wrapper classes, basically, in the case of different instances, false is returned even if they have the same value. Up to this point, it is the same as not comparing Strings with ==.
However, the trouble is that depending on the value, true may be returned. Specifically, if the values are -128 to 127, true will be returned even if they are different instances if they have the same value. Even if you test with a small value, it may hurt if you release the code compared with ==. Be careful.
Let's compare using ʻequals` as well as String. By the way, I think this issue will be detected by static analysis tools.
I have seen it. It seems to be thrown in the definition of the method, but in the logic, exceptions that can never be thrown are declared throws, so the exception handling code explodes ... It was already very proliferating because it was an important method called from here and there. The exception is never thrown. The code to handle exceptions that can never be thrown can only be called garbage. This made the code uselessly bloated again. ~~ I was crumpled and erased without permission. It was a lot of fun. ~~
Carefully design the definition of the throws clause. Make sure that the exception can really be thrown. Java checked exceptions have the powerful power of forcing the caller to handle exceptions. Think twice about whether it's really appropriate to force the caller to handle the exception.
iBATIS is a convenient OR mapper, but it has been out of support long ago, and its specifications are old, such as returning a Raw type List. You should move to the subsequent MyBatis.
In Excel, you can group specific rows and columns, and you can show or hide the grouped parts with a single click. This function is used to leave the uncorrected content in a hidden state. This is a substitute that has a lot of harm and only one profit. First, it leaves all the modifications, which makes the file heavier, takes longer to display, and reduces productivity. Also, although it is normally hidden, it causes search noise because it exists, and when creating a serial number etc. with the autofill function, if there is a group in the middle, a serial number is also created in the contents. It's very disturbing. It's similar to [Comment out the corrections and leave them all](#Comment out the corrections and leave them all). Basically, having a fix history in the file itself may be a bad idea. In addition, although I wrote that there is a profit, it is convenient to be able to browse the contents before correction with one click. But I can only think of the advantages, and there are more disadvantages.
I think you should manage the version of the design document with SVN etc. Of course, Excel files are binary files, so I'm not very happy to manage the version as it is. So, I think you should create the design document with Markdown or AsciiDoc instead of Excel. In that case, it is a text file, so it is easy to manage the version, and if you really like Excel, you can convert Markdown or AsciiDoc to an Excel file. (I haven't checked it properly, but I think I can probably do it)
The coverage measurement tool that was instructed to use in the project was a rather bold method of copying the entire original code and embedding the source code for coverage measurement. (I'm not familiar with coverage measurement tools, but I think it's common to see them at the bytecode level ...) When performing a test (manual test instead of UT), the rule was to build the source code generated by the tool and execute the test, but the tool will copy and place it. Since only Java files are used, all other XML, JSP, etc. had to be manually copied, and it took a long time before the tests could be performed, which was very inconvenient. Besides, there are two sources, the original source and the source generated by the tool, so you have to be careful to keep the two files in sync. After modifying the source a little, I forgot to reflect the modification in the measurement source, and I had to redo the test several times. In addition, the mysterious Swing application had to be running to measure coverage, which was heavy and annoying. Furthermore, even if the Swing application is not launched, it works normally (it works, but the coverage is not measured), so I noticed it later and tried the test again. This forced the use of inconvenient tools and reduced productivity.
To be honest, I'm not familiar with coverage measurement tools, but I think I should have used JaCoCo or a well-known tool instead of the oleore tool that is used only in-house. The coverage measurement tool I use at my workplace right now is JaCoCo, but I don't think there was any accident-inducing process such as embedding the measurement code at the source code level. In fact, coverage measurement is generally measured during UT execution, not during manual testing. In the first place, it may be a problem that UT is not written at all. [^ no_ut] [^ no_ut]: The lack of a UT is definitely a problem, but from a coverage measurement perspective as well.
This is also a mystery, but you came, that kind of person. It's a hassle to look at the fix history, because even though it's a single fix, the commits are spread over multiple (not two or three, but many). Perhaps you don't really understand the significance of a version control system, and you just think of it as a mechanism that allows everyone to share the source. That's still a good thing, and some fierce people leave their seats even though they haven't committed everything. If you check out in the meantime, of course, you'll get half-baked and you'll suffer from persistent compilation errors.
I have no choice but to tell you what a version control system is and how the above behavior will affect you. It's a hassle because I'm from another company or a senior at my company ... (* Not at my current workplace)
Although it is said to be a code, it has become a mess because it contains contents other than the code, but I would be happy if there is any item that was helpful.
Recommended Posts