Why was the code painful to read?

Introduction

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.

Premise

Part 1. Situational unrest

There is no positive information

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 execution environment

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.

Rumors that make me feel uncomfortable

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.

Part 2. Source code First impression of anxiety

Long method

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

There are 12 nests

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

Part 3. Welcome to wonderful variables

Some flags declared one after another

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.

A flag of something to play with denial or or or and

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.

Loop counter that seems to have some intention

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.

Something like a loop counter

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?

Kind naming that makes it easy to understand what will be included

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.

Kind naming that makes it easy to understand what will be included (I do not say that it will be entered)

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?

Classic magic number

int[] checkNum = {0, 1, 2, 3};
int[] kubun = {1, 2, 3};

Rather, I'm happy to meet something I'm familiar with.

Misspelled

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.

Part 4. Comments that fuel anxiety

By far the most fluffy

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

The first thing you want to see if it's a lie

//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?

Things that make you think for a moment

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

It ’s not a lie, but it ’s a moody thing.

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 conclusion

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

Why was the code painful to read?
I was addicted to the roll method
I was addicted to the Spring-Batch test
A memorandum to clean up the code Ruby
The code I used to connect Rails 3 to PostgreSQL 10
[Ruby] Code to display the day of the week
Read the official Dagger2 documentation to understand the basics
About the matter that the code to read the C structure member (Char array) that was working in swift 2.3 in swift 3 did not work
I was addicted to the NoSuchMethodError in Cloud Endpoints
I read the readable code, so make a note
I was addicted to the record of the associated model
To implement, write the test and then code the process
[Code golf] Deflate the code and submit it to AtCoder [Compressed golf]
I was addicted to the setting of laradock + VSCode + xdebug
Post back the coupon code you used to the specified endpoint
Correct the character code in Java and read from the URL
What I was addicted to with the Redmine REST API
Try changing to asynchronous processing via MQ without changing the code
Contributed to Gradle and was named in the release notes
The story I was addicted to when setting up STS
How to apply C code format from the command line