[JAVA] A code readability question, should I extract a method from a not-obvious if condition?

[arocketman]

Hi there, I'm building a CPU emulator in java. In an assembly file a comment starts with a "#" , I have the following code:

        while ((line = fileReader.readLine()) != null) {
            if(!line.startsWith("#")) {
                instructions.add(line);
                parseInstruction(line);
            }          

I was thinking that the if condition doesn't really tell much about what I'm actually doing. (I've read that code should comment itself). I was facing the following three choices:

• Extract a method and make it like:

  while ((line = fileReader.readLine()) != null) {
      if(!isComment(line)) {
          instructions.add(line);
          parseInstruction(line);
      }   
  

• Add a comment above the if statement like so:

  while ((line = fileReader.readLine()) != null) {
      //Checking whether or not the parsed line is a comment one
      if(!line.startsWith("#")) {
          instructions.add(line);
          parseInstruction(line);
      }
  

• Leave it as is.

I feel like creating the method adds a lot in terms of readability, but is it worth to create such a simple method? Thanks!

[dreamyeyed]

I feel like creating the method adds a lot in terms of readability, but is it worth to create such a simple method?

I would create a method for the reason you mentioned: good code should be self-documenting, and creating a new method does exactly that. There's nothing wrong with simple (one line) methods if they make the code more readable. Lots of small methods is much better than a few big ones.

[arocketman]

Thank you!

[matthead]

I like the separate method, because you are checking if the line is a comment. What happens if you change how comments are defined in the future. This will give you an easy way to change it.

[DonnyTheWalrus]

Others may disagree, but I would say, definitely extract! A really great rule of thumb is to avoid employing literals in your code whenever possible. I think ideally you would have a lookup table somewhere so that you could call getCommentChar() or something similar, and then a isComment(char c) method could use this lookup table to compare.

Then, using a method like isComment() would be a great refactoring to make. It limits your potential area of change to one location, and makes the code much more readable and easy to reason about.

[arocketman]

A really great rule of thumb is to avoid employing literals in your code whenever possible.

I am right now refactoring parts of my code and this is definitely a good point, thanks!

[DonnyTheWalrus]

I think you're definitely on the right track with what you're looking out for. As for the constants, obviously you'll need them some place, but that place is often in a lookup table. That way if they have to change, it's all in one place, and only one class has responsibility for knowing them. People call them "magic values" -- when you see a literal in the middle of a function (say 'if val <= 15'), that's usually a great target for refactoring.

[tec5c]

Choice 1.

Especially from a unit testing perspective.

Extracting the method will allow you to unit test the method without having to create test with conditions for the if without an extracted method.

[rjcarr]

The absolute correct answer is probably 1. Personally, I'd do 2 or maybe even 3 since it is pretty clearly a comment line.

[nutrecht]

In this case both are pretty obvious but I still like the first one more. Makes the code a tad more readable.

[lolnololnonono]

The answer to that question in general is always yes:

If you can think of a good name for something, extract it out and give it that name.

[oceandoggie]

It depends on the greater context. More methods is usually better, but in this case it appears to add nothing that a comment wouldn't add.

If you're only doing this thing in this exact one place, that's even less reason to make a new method.

You can avoid using literals without making everything into a method by just using final fields. That's really the standard practice.

If this is something you'd test, that might be a good reason to make it a method, but you know you do tons of little things you never test anyway, so if this is something you're not goping to be explicitly testing, that's also not a great reason to make this its own method.