-1
\$\begingroup\$

I am trying to make my code as legible as possible, and today I wrote the following snippet and I'm unsure as to whether it can be improved in an obvious way. There's always room for improvement, of course.

private Date extractTestRunDateFromLog() {
    Matcher testDateMatcher = Pattern.compile(regexes.getTestDateRegex()).matcher(log);
    Matcher testTimeMatcher = Pattern.compile(regexes.getTestTimeRegex()).matcher(log);
    String date = testDateMatcher.group(1);
    String time = testTimeMatcher.group(1);
    String dateTime = joinDateAndTime(date, time);
    try {
        return parseLogDate(dateTime);
    } catch (ParseException e) {
        throw new RuntimeException(e);
    }
}

private String joinDateAndTime(String date, String time) {
    return date + " " + time;
}

private Date parseLogDate(String date) {
    DateFormat format = new SimpleDateFormat("yyyy.MM.dd HH:mm:ss.SSS");
    return format.parse(date);
}

I'm not going to explain what it's supposed to do as it should be understandable without any comment - if it isn't, please do state what is unclear, as that is the purpose of this review.

\$\endgroup\$
  • \$\begingroup\$ Welcome to CR! Are you on Java 8? \$\endgroup\$ – h.j.k. Aug 14 '15 at 17:21
  • \$\begingroup\$ @h.j.k. I'm not really sure, to be honest. I do use Java 8 for development but it's unclear what the minimum supported JRE will be. My guess is Java 7. Also, if you vote down, I'd love to know why :) \$\endgroup\$ – autrilla Aug 14 '15 at 17:27
  • 2
    \$\begingroup\$ I'm not going to explain what it's supposed to do as it should be understandable. That's not how this site works. What is it supposed to do and does it work as expected? \$\endgroup\$ – Mast Aug 14 '15 at 17:28
  • 1
    \$\begingroup\$ @zyngawow Perhaps you should explain your code, and why you do this in the way you are. For example, why do you catch ParseException only to rethrow as a RuntimeException? This generally goes against best practices. \$\endgroup\$ – 410_Gone Aug 14 '15 at 17:38
  • 2
    \$\begingroup\$ @zyngawow I agree, but there is a major difference between needing comments to explain code, and being presented with code without knowing what problem it is meant to solve. You can write code that someone can read and see that it produces all the primes that end in 7, but someone reading it will not know why you need this code at all \$\endgroup\$ – spyr03 Aug 14 '15 at 19:45
4
\$\begingroup\$

There's some real nice improvements that can be made here:

Constants should be constant

regexes.getTestDateRegex() looks like a very specific constant. Constants should be kept in a static context and named in SNAKE_SHOUT_CASE. On a related note, regexes should definitely be a class and not an instance.

public static final class Regexes {
    public static final String DATE = /*..*/;
    public static final String TIME = /*..*/;
}

Precompile patterns

It's computationally less intensive for you, if you precompile your regex patterns. The state of the match will be kept in the Matcher instance anyways. instead of compiling the pattern everytime that method is called, you could consider:

public static final class Patterns {
    public static final Pattern DATE = Pattern.compile(Regexes.DATE);
    public static final Pattern TIME = Pattern.compile(Regexes.TIME);
}

Since you probably won't be using the regexes anywhere else, you may as well inline them, effectively replacing a bunch of Strings with a bunch of compiled Patterns.

Don't use systems hungarian

You should really drop the test-prefix in your code. It's completely useless noise. Get over not having a dynamically typed language and use the Typesafety you have.

Documentation != Comments

In your comments you cite Robert C. Martin:

Robert C. Martin disagrees: "Every time you express yourself in code, you should pat yourself on the back. Every time you write a comment, you should grimace and feel the failure of your ability of expression.". I do too, obviously. I feel any comment you could write on this code would be redundant. As in "Extracts the date from a test run log". The function name says it already.

I read that and I can't let that stand. It's extremely important to differentiate between 3 different things here:

  1. API (method name, return type, parameters)
  2. Implementation comments
  3. Documentation

1. API
The first is a very viable indicator. It gives the programmer using your code a general gist of what happens. It still can leave a lot of questions open, though.

private Date extractTestRunDateFromLog() {

Okay... This means I get a date from a logfile. Question Time:

  1. What is the Timezone of the Date?
  2. What logfile is meant?
  3. Is that the start of a testrun or the end?
  4. What the crap is meant by testrun anyways??

Sure this might all be clear to you, and it can become clear from surrounding code in usage, but this is some serious (and seriously interesting) questions.

2. Implementation Comments

These are the comments that Martin is referring to here. Keep in mind, that this is not documentation (JavaDoc), but inline comments (or inline block comments).

These comments are intended to give clear knowledge of what you thought when you implemented something differently from what would be expected. They are there to convey thought processes behind nonobvious code. They are there to give someone else (like... you in half a year) necessary understanding to fully understand and accordingly expand code.

If you need these comments, that may be a sign of a code-smell. (iow. your code is not expressive enough). On the other hand, you shouldn't turn this maxim around.
Such comments show the (very human) inability to fully express thoughts in simplified constructs like language. As such they are a necessary crutch. And throwing away your crutch when you cannot walk sounds like a really dumb idea to me.

Don't fall away into thinking that comments are bad per se.

3. Documentation

This is the place to answer the questions I asked. This is the place to mention other overloads, to refer to related methods, to give usage examples. This is the place to amend your API into something that becomes dead simple to use.

Your API (just like comments and code in general) cannot express all implications and the little quirks that a sufficiently high abstraction has.

Use documentation to amend your API, not reiterate it. I know from my own painful experience, that API is simply not enough. Or do you know from offhand what the heck a method called like this does?

Collection<T> persons = something.getPersons();
Collection<T> allPersons = service.getPersons();
Utils.getSelected(persons, allPersons);
\$\endgroup\$
  • \$\begingroup\$ The regexes are configuration and they're loaded dynamically. They could be static, but that makes it less testable, global state is bad. Precompiling patterns is a fair point, but I only use them once right now, but yes, good point. I'm not using Hungarian notation - it doesn't specify the type, it specifies what each field belongs to: testRunCount gives way more meaning than count, and more than runCount. The date has no specified timezone. The answer to the rest of the questions is known from the rest of the class. Martin refers to both JavaDoc and implementation comments. (...) \$\endgroup\$ – autrilla Aug 14 '15 at 18:58
  • \$\begingroup\$ The thing is I don't think I need the comments. Unneeded comment are bad per se. \$\endgroup\$ – autrilla Aug 14 '15 at 19:04
  • \$\begingroup\$ There is a very significant difference between "I don't need these comments" and "I in 2 years don't need these comments"... \$\endgroup\$ – Vogel612 Aug 14 '15 at 19:08
3
\$\begingroup\$

One (small) bug: parseLogDate() needs to include the throws ParseException declaration as ParseException is a checked Exception.

I feel that the joinDateAndTime() method is... superfluous, as it should be quite straighforward to simply use that concatenation when calling parseLogDate(), instead of using this additional method.

And the (not small) bug: I believe you need to call a match operation, e.g. matcher.matches(), first, before Matcher.group(int) works.:

The explicit state of a matcher is initially undefined; attempting to query any part of it before a successful match will cause an IllegalStateException to be thrown.

\$\endgroup\$
  • \$\begingroup\$ Oops, yeah, I missed the throws, I copied it manually from a sheet of paper :), it's on it but I forgot to copy it over. Regarding the concatenation, keeping the same level of abstraction in the function is good practice. String concatenation is a lower level than group matching, in my opinion. The last part surprises me, because I don't ever call matches(), I directly call group(1), and no exception is thrown. \$\endgroup\$ – autrilla Aug 14 '15 at 17:41
  • \$\begingroup\$ @zyngawow are you sure the caller of this method is not catching IllegalStateException then? \$\endgroup\$ – h.j.k. Aug 15 '15 at 6:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.