3
\$\begingroup\$

This method allows the user to choose a start and end date in human readable form, then convert it to epoch. The method will return a array of longs composed by the epoch start date and the epoch end date.

I handle some exception so if the user types anything else than the date it will end the iteration and start again. If the end date is before the start date it will start again...

private static long[] rectime_choice_period() {  
    long epochstart = 0;
    long epochend = 0;
    boolean confirm = true;

    String datetimeend = null;
    String datetimestart = null;

    BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
    while (confirm){

        //enter start date 
        System.out.print("\nChoose start date (MM/dd/yyyy hh:mm:ss): ");     
        try{
            datetimestart = br.readLine();
        }catch (IOException e){
            System.err.println(e.getClass().getName()+": "+e.getMessage() + "\n\n\n"); 
            continue;
        }

        //enter end date
        System.out.print("\nChoose end date (MM/dd/yyyy hh:mm:ss): ");     
        try{
            datetimeend = br.readLine();
        }catch (IOException e){
            System.err.println(e.getClass().getName()+": "+e.getMessage() + "\n\n\n"); 
            continue;
        }

        //convert to epoch
        try{
            epochstart = new java.text.SimpleDateFormat("MM/dd/yyyy HH:mm:ss").parse(datetimestart).getTime() / 1000;   
            epochend = new java.text.SimpleDateFormat("MM/dd/yyyy HH:mm:ss").parse(datetimeend).getTime() / 1000;    
        }catch (ParseException e){
            System.err.println("Error entered date cannot be converted!");
            continue;
        }catch (Exception e){
            System.err.println(e.getClass().getName()+": "+e.getMessage() + "\n\n\n"); 
            continue;
        }


        if ((epochend - epochstart) > 0){
            System.out.println("Start time :" + epochstart + "\tEnd time : " + epochend);
            confirm = false;
        }else {System.err.println("Wrong date! Start date should be before end date!");}

    }

    long[] period = {epochstart, epochend};
    return period;
}

But I am looking to optimize this code, maybe avoid the loop or the continue statement, I checked on google way to improve java code but nothing was really interesting. So any idea? Something I made wrong?

| improve this question | | | | |
\$\endgroup\$
5
\$\begingroup\$

The while loop isn't really a problem. You need some way to loop back and re-ask the user input if something went wrong (e.g. they entered wrong data).

However, there are a couple of improvements that you can make:

  • You are opening a stream with

    BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
    

    but you are never closing it. This can lead to a resource leak. Starting with Java 7, you can deal easily with this by using the try-with-resources statement. Also, instead of using a BufferedReader, it would be easier to use a Scanner and its nextLine() method.

    try (Scanner scanner = new Scanner(System.in)) {
        // read user input with scanner.readLine();
    }
    

    This has other advantages: you don't need to catch any IOException so the code is cleaner.

    try (Scanner scanner = new Scanner(System.in)) {
        while (confirm){
            System.out.print("\nChoose start date (MM/dd/yyyy hh:mm:ss): ");     
            String datetimestart = scanner.nextLine();
    
            System.out.print("\nChoose end date (MM/dd/yyyy hh:mm:ss): ");     
            String datetimeend = scanner.nextLine();
            // ...
        }
    }
    
  • You should respect Java naming conventions for your variables and methods. Instead of datetimestart, prefer dateTimeStart (camel-case) and epochEnd. In the same way, instead of rectime_choice_period, prefer recTimeChoicePeriod (arguably, you could have another more descriptive name).

  • In the same way, you should respect the Java braces style

    }else {System.err.println("Wrong date! Start date should be before end date!");}
    

    should be rewritten

    } else {
        System.err.println("Wrong date! Start date should be before end date!");
    }
    
  • Don't hardcode the date format "MM/dd/yyyy HH:mm:ss". You should refactor that into a constant instead:

    private static final String DATE_FORMAT = "MM/dd/yyyy HH:mm:ss";
    

    in this way, if the pattern should ever change, you don't need to update the 4 occurences where it appears.

  • You are constructing twice a SimpleTextFormat when you could only construct one:

    DateFormat formatter = new SimpleDateFormat("MM/dd/yyyy HH:mm:ss");
    epochStart = formatter.parse(dateTimeStart).getTime() / 1000;   
    epochEnd = formatter.parse(dateTimeEnd).getTime() / 1000;
    

    As a side-note also, consider using imports instead of writing explicitely the fully qualified name of the class: import java.text.SimpleDateFormat;.

  • Avoid declarations of all the variables at the start of the method. Declare them only when they are needed. So, replace

    String datetimeend = null;
    // ...
    datetimestart = scanner.nextLine();
    

    with

    String dateTimeEnd = scanner.nextLine();
    
  • You don't need the parentheses inside if ((epochend - epochstart) > 0). A simple if (epochend - epochstart > 0) suffices and avoids extra parentheses.
  • Avoid useless comments: //enter start date adds no value as a comment. Good comments don't repeat the code but clarify its intent.
  • Also, avoid useless temporary variables:

    long[] period = {epochstart, epochend};
    return period;
    

    can just be written return new long[] { epochStart, epochEnd };.

  • In the code parsing the String into long, why are you catching the general Exception since you already have a catch clause for the ParseException? It is not necessary and should be removed.
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ That's perfect, really clear, many thanks Tunaki. The reason I create my variable at the beginning is to have all variable I will use and to initialize them if needed. Then I change the method so don't need all of them to be created there. I don't really see what could go wrong with try (Scanner scanner ... ){...} (there are no exception to catch) But do I need to put this? catch (Exception e) { System.err.println("Something went wrong!"); System.exit(0); } Thanks for your answer (my java is a bit rusted with that I could take the basic again). \$\endgroup\$ – Charly Roch Jan 29 '16 at 14:25
  • \$\begingroup\$ @CharlyRoch No, you don't need it because there are no exceptions to catch. You can just have a try-wih-resources without catch. \$\endgroup\$ – Tunaki Jan 29 '16 at 14:27
  • \$\begingroup\$ And one more question, Can you confirm me that the try-with-resources will close the scanner automatically? Thanks again :) \$\endgroup\$ – Charly Roch Feb 1 '16 at 10:30
  • \$\begingroup\$ @CharlyRoch Yes, it will. That's the big advantage of using a try-with-resources. It will close the Scanner whether there is an exception or not during execution of the try part. \$\endgroup\$ – Tunaki Feb 1 '16 at 10:34
  • \$\begingroup\$ Ok but when the try-with-resources will call the close method, it will also close the System.in linked to it? So I need to use a NoCloseInputStream or a CloseShieldInputStream to be able the reuse it in the next method. Or the easiest way will be to overried the close method. Any suggestion of the good way to do it without causing a resource leak? (in my mind if we don't close it we could have some issue with the memory) \$\endgroup\$ – Charly Roch Feb 1 '16 at 11:09
1
\$\begingroup\$

In the expression

(epochend - epochstart) > 0

the left hand expression can underflow, which makes it greater than 0 even though epochend < epochstart, which is what you actually want to test:

if (epochend > epochstart) {
    // success
} else {
    // failure
}

No over-/underflow issues and a much clearer way to express what you actually mean to test.

| improve this answer | | | | |
\$\endgroup\$

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.