6
\$\begingroup\$

I'm building a Java method to sum arithmetically two time periods, using the JodaTime library. My code works fine, but I think that it's possible optimize to reduce the time execution.

import org.joda.time.Period;
import org.joda.time.PeriodType;
import org.joda.time.format.PeriodFormatter;
import org.joda.time.format.PeriodFormatterBuilder;


public class Test {

    public static void main(String[] args) 
    {
        String time1 = "08:00";
        String time2 = "08:00";

        System.out.println(Operation_Sum(time1,time2));
    }

    private static String Operation_Sum(String time1, String time2)
    {
        String output;
        long start = System.currentTimeMillis();

        if (time1.equals("00:00") && time2.equals("00:00"))
        {
            output = "00:00";
        }
        else if (time1.equals("00:00"))
        {
            output = time2;
        }
        else if (time2.equals("00:00"))
        {
            output = time1;
        }
        else
        {
            boolean sign_time1 = false;
            boolean sign_time2 = false;
            String[] time1_out, time2_out;

            boolean negative = false;
            String output_split[];

            if (time1.contains("-"))
            {
                time1_out = time1.split(":");
                time1_out[0] = time1_out[0].replace("-", "");
                time1_out[1] = time1_out[1].replace("-", "");
                time1 = time1_out[0] + ':' + time1_out[1];
                sign_time1 = true;
            }

            if (time2.contains("-"))
            {
                time2_out = time2.split(":");
                time2_out[0] = time2_out[0].replace("-", "");
                time2_out[1] = time2_out[1].replace("-", "");
                time2 = time2_out[0] + ':' + time2_out[1];
                sign_time2 = true;
            }

            PeriodFormatterBuilder builder = new PeriodFormatterBuilder();
            builder.minimumPrintedDigits(2);
            builder.printZeroAlways();
            builder.appendHours();
            builder.appendLiteral(":");
            builder.appendMinutes();
            PeriodFormatter pf = builder.toFormatter();

            Period period1 = pf.parsePeriod(time1);
            Period period2 = pf.parsePeriod(time2);
            Period normalized;
            Period total = null;


            if (sign_time1 && sign_time2)
            {
                total = period1.plus(period2);
                negative = true;
            }

            if (sign_time1 && !sign_time2)
            {
                total = period2.minus(period1);
            }

            if (!sign_time1 && sign_time2)
            {
                total = period1.minus(period2);
            }

            if (!sign_time1 && !sign_time2)
            {
                total = period1.plus(period2);
                negative = false;
            }

            normalized = total.normalizedStandard(PeriodType.time());
            output_split = pf.print(normalized).split(":");

            if (output_split[1].contains("-"))
            {
                output = output_split[0] + ":" + output_split[1].replace("-", "");
            }
            else
            {
                output = (negative ? "-" : "") + output_split[0] + ":" + output_split[1];
            }
        }

        long end = System.currentTimeMillis();

        System.out.println("exec time sum1: " +(end - start) + " ms");
        return output;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You should change the title to tell what your code does, not how you want it reviewed, you will attract more reviewers this way. Check this out : meta.codereview.stackexchange.com/questions/2436/… \$\endgroup\$ – IEatBagels Sep 5 '14 at 12:30
  • 1
    \$\begingroup\$ I tried your code System.out.println(sumTimes("23:51", "02:12")); output25:63. Why is that? \$\endgroup\$ – Hau Le Aug 12 '16 at 16:41
11
\$\begingroup\$

You're really overcomplicating it. The great thing about Joda Time is that it can do all the hard work for you. This simplified method will give the exact same output as your original Operation_Sum:

static String sumTimes(String time1, String time2) {
    PeriodFormatter formatter = new PeriodFormatterBuilder()
            .minimumPrintedDigits(2)
            .printZeroAlways()
            .appendHours()
            .appendLiteral(":")
            .appendMinutes()
            .toFormatter();

    Period period1 = formatter.parsePeriod(time1);
    Period period2 = formatter.parsePeriod(time2);
    return formatter.print(period1.plus(period2));
}

Test cases I used on both your method and this new one:

@Test
public void testSum8Plus8() {
    String time1 = "08:00";
    String time2 = "08:00";
    assertEquals("16:00", JodaDemo.sumTimes(time1, time2));
}

@Test
public void testSum8Plus18() {
    String time1 = "08:00";
    String time2 = "18:00";
    assertEquals("26:00", JodaDemo.sumTimes(time1, time2));
}

@Test
public void testSum8Minus18() {
    String time1 = "08:00";
    String time2 = "-18:00";
    assertEquals("-10:00", JodaDemo.sumTimes(time1, time2));
}

In addition to the simplification, some other improvements:

  • Variables should be named camelCase (don't start with capital letter, don't use underscore)
  • Typical *Builder classes support a fluent interface, so instead of builder.someMethod(); builder.anotherMethod();, you can just chain the calls as builder.someMethod().anotherMethod();
  • Unit tests are good to let you make changes and confirm that everything is still working. It's also easier for code reviewers, so they can easily test refactored version of the code. Try to include more tests to cover all corner cases.

There were glaring logical mistakes in your original implementation. For example in this code:

if (time1.contains("-"))
{
    time1_out = time1.split(":");
    time1_out[0] = time1_out[0].replace("-", "");
    time1_out[1] = time1_out[1].replace("-", "");
    time1 = time1_out[0] + ':' + time1_out[1];
    sign_time1 = true;
}

This is (pretty much) equivalent to the simpler and better:

if (time1.contains("-")) {
    time1 = time1.replaceAll("-", "");
    sign_time1 = true;
}

That is, here, and in many other places in your code, you were doing a lot of operations that can be easily simplified to fewer operations.

\$\endgroup\$
  • 1
    \$\begingroup\$ thanks man!!! you're great!! :) your code works very well and is better than my example... the split and replace was a check only, but after some edit on source code, I think that I will remove this part! :) \$\endgroup\$ – user3449772 Sep 5 '14 at 12:40

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.