Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a String like "Time left (XX)" or "You need stay here for (XX)" and int as second. I use those two variables to format label like "string (mm:ss)" example "Time left (59:23)".

private String timeDescription(String pDescription,int pTime) {

    final String preformatedTime = secondsToString(pTime);
    final String timeForReturn = putTimeInXX(pDescription,preformatedTime);
    return timeForReturn;
}

private String secondsToString(int pTime) {
    final int min = pTime/60;
    final int sec = pTime-(min*60);

    final String strMin = placeZeroIfNeede(min);
    final String strSec = placeZeroIfNeede(sec);
    return String.format("%s:%s",strMin,strSec);
}

private String placeZeroIfNeede(int number) {
    return (number >=10)? Integer.toString(number):String.format("0%s",Integer.toString(number));
}

private String putTimeInXX(String pDescription,String pTime) {
    String[] apartDescription = pDescription.split("XX");

    StringBuilder descriptionForReturn = new StringBuilder();
    for (int i = 0; i < apartDescription.length; i++) {
        descriptionForReturn.append(apartDescription[i]);
        if (i == 0) {
            descriptionForReturn.append(pTime);
        }
    }
    return descriptionForReturn.toString();
}
share|improve this question

4 Answers 4

Your code looks functional, and, if this was for early versions of Java, it would be quite typical and standard.

The String.format process can do so much more than what you are using it for, though.

Your function:

private String secondsToString(int pTime) {
    final int min = pTime/60;
    final int sec = pTime-(min*60);

    final String strMin = placeZeroIfNeede(min);
    final String strSec = placeZeroIfNeede(sec);
    return String.format("%s:%s",strMin,strSec);
}

can be simplified drastically by using more of the String.format functionality:

private String secondsToString(int pTime) {
    return String.format("%02d:%02d", ptime / 60, pTime % 60);
}

The %02d means: "Format as a decimal number using at least 2 digits, and pad with 0 if less than 2 digits"

Note, that using the modulo 60 pTime % 60 gets the remainder of seconds when dividing by 60. This is short hand for pTime - (ptime / 60) * 60

share|improve this answer
    
+1 Format strings were built for these things. –  JvR Aug 12 at 11:13
    
If you incorporate the format string too - so Time Left: %02d:%02d them you would also get rid of the OP's strange loop. –  Boris the Spider Aug 12 at 19:11
    
@BoristheSpider - you are right. I considered making that change/addition immediately after I posted this answer, but then I thought it would compete too much with ZeroOne's answer too. Feel free to upvote his answer. It recommends exactly what you are suggesting. –  rolfl Aug 12 at 19:28
    
The code wouldn't work in early versions of Java, since it calls String.format(). –  200_success Aug 12 at 23:00

Instead of a custom "placeZeroIfNeeded" method you could just use the ready-made number formatting possibilities of Java:

final String strMin = String.format("%02d", min);
final String strSec = String.format("%02d", sec);

Then, you don't really need your putTimeInXX method either, you can do it like this:

final String timeForReturn = pDescription.replace("XX", preformattedTime);
share|improve this answer

You don't require to loop it through to make a string; you could use a replace method in Java and just replace with your time string.

final int sec = pTime-(min*60);

This is not required; you could take modulo of input number.

private String timeDescription(String pDescription,int pTime) {
    return putTimeInXX(pDescription,secondsToString(pTime));
}

private String secondsToString(int pTime) {
    final int min = pTime / 60;
    final int sec = pTime % 60;

    final String strMin = placeZeroIfNeeded(min);
    final String strSec = placeZeroIfNeeded(sec);
    return String.format("%s:%s",strMin,strSec);
}

private String placeZeroIfNeeded(int number) {
    return (number >=10)? Integer.toString(number):String.format("0%s",Integer.toString(number));
}

private String putTimeInXX(String inputDescription,String pTime) {
    return inputDescription.replace("XX",pTime);
}
share|improve this answer

That entire code could be reduced to these few lines:

private String timeDescription(String description, int seconds) {
    return putTimeInXX(description, secondsToString(seconds));
}

private String secondsToString(int seconds) {
    return String.format("%02d:%02d", seconds / 60, seconds % 60);
}

private String putTimeInXX(String description, String timeString) {
    return description.replaceAll("XX", timeString);
}

@rolfl already pointed out about secondsToString.

The int pTime parameter was not well-named: the Hungarian notation is not recommended, simple English words are better. In this case, the method formats seconds to string, so seconds would be a much more natural name. I changed all the variable names following this logic.

The putTimeInXX method was too tedious: splitting the input string on XX just to replace it? It's a lot easier and natural to use String.replace or String.replaceAll. Also, I would recommend using %s in the input string, that way you could replace the %s template even more efficiently using String.format:

private String putTimeInXX(String description, String timeString) {
    return String.format(description, timeString);
}

In the timeDescription method, the intermediary final variables were really pointless. You can simply insert the method calls in a single expression, and the line is still not too long, and you have a much shorter method.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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