Good evening. I'm now on a legacy system maintenance team with VB on the front, Java 1.4 on the server, and CVS on the version control tool (but I'm not saying it's in control). We are here.
It is not uncommon for the help desk to read the code for investigation at the request of the help desk who received the user's inquiry. This time, I started by asking "Why did I get this error when I did this kind of operation?", And I read the VB code a little unusually.
Since I have almost no VB experience, I looked at the code for the time being, but it was too "I'm not talking about the language ...". However, if you just complain, it seems that someone somewhere will be angry if you are not productive, and it will be useful for reconfirmation including self-admonition that "this is difficult to understand", so this article I would like to write.
Each one is not a big deal, and it seems that I can do it myself, but I realized once again that the readability and maintainability would be terrible if they overlap.
There are no requirements definition documents, design documents, test codes, etc., and no one knows "what is the right thing", neither the user who made the inquiry, the help desk who received the inquiry, nor the members in the field. .. No one even knows if the error is correct. So you're really purely looking for "why did you get that error?" (At this point, it seemed like a lot of fun)
There is no output that seems to be useful in the application log, and there is no environment that can be debugged like Visual Studio. It seems that the environment can be used if you apply and get permission, but I remember that it took 3 weeks from applying for the user ID of the internal environment to getting it, so for the time being I opened the source code with a text editor I will read it.
The initial development seems to be about 10 years ago, and I hear rumors that it seems that there was a big flame at that time. I hear that everyone involved in the initial development, let alone the person who wrote this source, has left. Well, I have a bad feeling.
The methods that you should read in the main are about 600 lines and about 200 lines, respectively. Well, it's still a short category for me, who has been in the field where there are thousands of lines of methods forbidden to make methods ...! (However, there were a lot of comments and documents at the site of thousands of lines, and many members understood the data structure exactly.)
The left side of the center was Sukasuka. I like shallow nests unless there is a particular reason, and I couldn't think of any reason why I had to make them 12 layers ...
boolean ariFlg;
boolean flg;
boolean checkFlg;
boolean okFlg;
Even if the variable name is a little long, it is easier to read if it is easy to imagine when the flag for what will be true. I would like you to add a comment at least. By the way, some flags were not used anywhere.
if (!flg || okFlg && !checkflg) {
if (!ariFlg) {
}
}
Of course, it's actually a 12-fold nesting & 600-row method, so it's a lot more complicated. Personally, I don't like to use negation indiscriminately, or to combine flags that are used for different purposes here and there, as it can be very confusing.
for (int k = 0; k < list.size(); k++) {
for (int h = 0; h < list.size(); h++) {
for (int g = 0; g < keys.size(); g++) {
}
}
}
The loop counters aren't in alphabetical order, and they don't seem to be an acronym for anything, and they may or may not have end conditions.
int ii;
for (int i = 0; i < list.size(); i++) {
if (Conditional expression) {
ii = i;
map.put(ii, i);
}
}
i and ii ... I think it's a nested loop process, but the key / value of the map seems to be ii and i. what's that?
int mainasuIchi = -1;
I didn't think it was "Mainasuichi" in Roman alphabet, but when I first saw it, I thought it was some kind of English word. I was worried that it wasn't a constant and that it wouldn't be possible to enter -2 or -3, but as a result it wasn't. It was -1 until the end.
arrLst.put(key, value); //arrLst is a HashMap type. Of course, this comment is not actually available.
I think it's easier to understand if there are fewer variables that you have to worry about in one method. It's a method that has hundreds of lines, and I can't keep up with it without specifications, designs, and a debugging environment ... Isn't a super engineer a flatulence like this?
int[] checkNum = {0, 1, 2, 3};
int[] kubun = {1, 2, 3};
Rather, I'm happy to meet something I'm familiar with.
String maccingMsg;
Maybe if something matches something, put a message here. Anyone can make a misspelling. However, when I encountered this after seeing various confusion variables, "I think this is not just a misspelling, there is also matchingMsg? I think I made this to make another type on top of that?" I became anxious and would search the source code with match.
//Set to 0
//Do more
//It's an error
//Confirm
//Count
//check
why? what? I don't know, so I'm afraid to dive into the next nest. Of course, it's a compact method, it's easy to imagine that you check this in a class or method, it's your own tool, and I think there are some comments like this. However, I think it is safe to avoid it in a complicated, large-scale system where members are replaced because it is a release product and it is known that it will be maintained for many years to come. I have had the experience of being in flames and rushing to "must move anyway", but if you pay attention to comments and variable names a little, you will be saved when a bug occurs, and as a result it will not spread.
//Process when variable a is 10 and variable b is 20
if (a == 10) {
//processing
}
The variable b is not judged. Nesting is still going on in the process, so will we do it later? Or is it that the variable b is 20 to reach here? Is the comment wrong? Is there a bug that the variable b is not checked because it was not implemented as commented?
//OK if there is only one OK
I understand, I understand. Maybe it's OK to check something and if there is one that is OK. I'm not wrong. I'm not wrong.
for (Conditional expression) {
//If flg is true, process execution
if (!flg) {
continue;
}
//processing
}
"If flg is false, it will be negative with !, so it will be true and continue, and if it is true,! Flg is false, so processing will be executed without entering the if statement, that is, the comment is correct ... right?" You will think in agony. If this is Nest, Nest, Nest ..., then your head will be full and you will want to throw everything out and take a nap in a grassland hammock.
In a project I participated in for a moment, I once thought that it was really good because the wiki said, "A mistake is the result of the person in charge doing his best at that time. Don't blame it." .. The company I first entered was a place where the training was done in-house, and the main focus was on in-house production of in-house products and contract projects. However, if the company didn't do the training for me and was suddenly skipped by the customer and there was no follow-up or review ... I might have written worse code, and I was already doing a job unrelated to IT. maybe. It was painful when I was reading it, but this time I encountered this kind of code, which made me write an article, and it also gave me a chance to think about readability in my own way. Let's be productive!
Recommended Posts