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 made a method which takes two parameters: the value to convert and the base. Based upon those two parameters my method is adjusted (with several helper methods) to change the desired decimal number to a number with a different base.

Is this code practical and clean? If not, what should I change to make it better?

public String baseConversion(int base, int value){
    String finalResult = "";
    String tempResult = "";
    int countLength = 0, countFit = 0; // countLength counts the length of the tempResult string in order to know how many '0' to add. countFit counts how many times the tempValue fits into the current parameter 'value'
    int placeValue; // Records the place value. ex 245: '2' is in the hundreds place value
    int tempValue = 0;

    while (value > 0){
        placeValue = 1;
        // calculates the highest power that fits in the value (takes desired base into account)
        while (value > placeValue){
            placeValue*=base;
            countLength++;
        }
        if (value!=placeValue){
            countLength--; // subtract to account for the value in the tempResult that is not '0'
            placeValue/=base; // divide back as the previous while loop went one placeValue higher than needed in order to break out
        }
        //figures out how many times the placeValue can fit into the value
        while (tempValue+placeValue <= value){
            tempValue+=placeValue;
            countFit++;
        }
        if (base==16){
            tempResult += numConverter(countFit);
        }
        if (base==8 || base == 2 || base == 10){
            tempResult += String.valueOf(countFit);
        }
        // adds zeros to the rest of the string
        while (countLength > 0){
            tempResult+="0";
            countLength--;
        }

        // takes the current tempResult and combines with previous. ex combine 10000 with 100 = 10100
        finalResult = combineResults(finalResult, tempResult);

        value -= tempValue;            
        countFit=0;
        tempValue=0;
        tempResult="";
    }
    return finalResult;
}

Helper methods:

private String numConverter(int number){
    // only for hexadecimal
    if(number == 10){
        return "A";
    }else if (number == 11){
        return "B";
    }else if (number == 12){
        return "C";
    }else if (number == 13){
        return "D";
    }else if(number == 14){
        return "E";
    }else if (number == 15){
        return "F";
    }else {
        return String.valueOf(number);
    }
}

private String combineResults(String a, String b){
    String result = "";
    int indexA = 0, indexB = 0;
    int aTempLength = a.length(), bTempLength = b.length();
    if (a.length() > b.length()){
        while (aTempLength > bTempLength){
            result+=a.charAt(indexA);
            indexA++;
            aTempLength--;
        }
    }else {
        while (bTempLength > aTempLength){
            result+=b.charAt(indexB);
            indexB++;
            bTempLength--;
        }
    }
    while (aTempLength > 0){
        if (String.valueOf(a.charAt(indexA)).equals("0")){
            result+=b.charAt(indexB);
            indexA++;
            indexB++;
        }else {
            result+=a.charAt(indexA);
            indexA++;
            indexB++;
        }
        aTempLength--;
    }
    return result;
}
share|improve this question

3 Answers 3

up vote 8 down vote accepted

Disclaimer: I'm new here, so this might not be the type of review people give, but I'm trying to match it as best as I can.

Formatting

A comment explaining the purpose of combineResults(String a, String b) next to the method would be helpful. (Yes, there's one in the main method, but documentation next to the helpers is good, too.)

The same could be said for numConverter(int number)...

Other than that, indentation looks good, braces look fine, etc. :)

Algorithm

In general: You should be passing around StringBuilders or StringBuffers (or chars) instead of Strings. The former are mutable, while the latter isn't mutable. This will save some performance. (e.g. appending is better on memory with the builders and buffers, rather than with raw Strings.)

Main Algorithm

Ok... You took kinda an odd approach to converting bases. Rather than using all those while loops, you should focus on determining the next least significant digit by using the modulus operator followed by division.

For example, in pseudocode:

while(number > 0) {
  nextDigit = number % base; //Get the next digit using modulo
  number /= base; 
  // prepend nextDigit to result String Buffer
}

This makes the combineResults() method unneeded, as prepend is built-in to the StringBuilder.

NumConverter

In the spirit of StringBuilders vs. String, this should return a char rather than a String.

Your current code assumes that the only number ever fed to this method will be less than 16. This could be a problem if it's a larger library, or if you want to expand the bases to more than hexadecimal.

You could use a switch/case construct instead of the else if's...

Or, if you really want to shorten the code, you could do something like (note: this also implements the char return instead of String):

char numConverter(int digit) {
   if (num >= 16 || num < 0) //If we're not given a valid digit
     return 0; //Let the output know somehow...  could throw an exception instead.
   else if (digit <= 10) //If we're dealing with a "normal" digit
     return ((char)num + '0'); //... return the numeric equivalent
   else //We're in extended digit range: we need letters!
     return ((char)digit + 'A' - 10); //Return hex equivalent of 10-15 (A-F)
}
share|improve this answer
    
Very good point about the modulus! I totally missed that option. Ill do that next time. Thanks! –  ybic Feb 17 at 2:12
    
Given the number of up-votes on your answer, it is pretty much the kind of review we do here :) Well done. Hope to see more posts from you. Welcome to Code Review! –  Simon André Forsberg Feb 17 at 17:41

A few random notes:

  1. It returns an empty string with 0 as input value. I guess it's a bug.

  2. int countLength = 0, countFit = 0;
    

    I'd put the variable declarations to separate lines. From Code Complete, 2nd Edition, p759:

    With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

  3. Horizontal scrolling doesn't make reading easier. You could put the comments in the line before the variables:

    // countLength counts the length of the tempResult string in order to know how many '0' to add. 
    int countLength = 0; 
    //countFit counts how many times the tempValue fits into the current parameter 'value'
    int countFit = 0; 
    

    Anyway, commenting is a smell. Clean Code by Robert C. Martin, p54:

    The proper use of comments is to compensate for our failure to express ourself in code. Note that I used the word failure. I meant it. Comments are always failures. We must have them because we cannot always figure out how to express ourselves without them, but their use is not a cause for celebration.

    The book has lots of hints which help to avoid this smell.

  4. Try to minimize the scope of local variables. It's not necessary to declare them at the beginning of the method, declare them where they are first used. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

    For example, tempResult could be declared inside the while loop:

    while (value > 0) {
        String tempResult = "";
        ...
    
    }
    

    It would make the tempResult = ""; statement unnecessary at the end of the loop.

  5. If you don't know the already existing methods for these conversions in the JDK check them. Integer class has toOctalString(int i), toHexString(int i) and toBinaryString(int i). You can look into their source too.

share|improve this answer
1  
Thanks! Really good point about comments. Also this was for hw so I couldn't have used the already provided methods. But thanks anyways. –  ybic Feb 17 at 2:11

If you are deliberately , please tag your question as such. Since you didn't tag it, I'm obligated to mention that you could just write

Integer.toString(value, base).toUpperCase();

I'll start with the helper methods. I believe that neither helper should be necessary.

private String numConverter(int number) is basically equivalent to Character.toUpperCase(Character.forDigit(digit, 16)). That would be static, and returns a char; both would be a good idea for your numConverter(). Also, you only use numConverter() for hexadecimal, but it could actually work just as well for any base up to and including 16.

combineResults() looks like an adder that operates on strings, without needing to support carrying. That's an odd approach to base conversion. You should be able to build the result by appending a digit at a time without needing this function at all.

Now, on to baseConversion() itself.

The function is a pure function that does not rely on any object state. Therefore, it should be declared static.

Personally, I feel that the parameters would be more natural in the reverse order (value, base).

You don't support negative inputs. That could be reasonable, but returning an empty string for such inputs is not. Throw an exception. Not supporting 0 as a value is a bit more surprising. You should be able to tweak your code to handle 0. Another surprise is that if base is not 2, 8, 10, or 16, the function returns an empty string. Again, either add support or add validation.

The big block of variable declarations at the top is a bad sign. You should declare your variables in the tightest scope possible. The "temp" variables certainly don't belong there.

This if-block, which conditionally undoes one iteration of the preceding while-loop, is a sign that the while-loop's termination condition is wrong.

if (value!=placeValue){
    countLength--; // subtract to account for the value in the tempResult that is not '0'
    placeValue/=base; // divide back as the previous while loop went one placeValue higher than needed in order to break out
}

If asked to keep to the spirit of the original code, here's how I would rewrite it:

public static String baseConversion(int base, int value) {
    if (base <= 0 || value < 0) {
        throw new IllegalArgumentException("Negative bases and values are not supported");
    }
    if (base < Character.MIN_RADIX) {
        throw new IllegalArgumentException("Base too small");
    }
    if (base > Character.MAX_RADIX) {
        throw new IllegalArgumentException("Base too large");
    }

    StringBuilder result = new StringBuilder();
    int placeValue;
    for (placeValue = 1; placeValue * base <= value; placeValue *= base) {}
    for (; placeValue > 0; placeValue /= base) {
        int digit = value / placeValue;
        value -= digit * placeValue;
        result.append(Character.forDigit(digit, base));
    }
    return result.toString().toUpperCase();
}
share|improve this answer
    
I've read that sometimes it is better to declare all variables at the top as it simplifies debugging by not introducing new variables as you go through the process. However I might be wrong with this as I've never debugged a big project myself so I can't say too much about it. Anyways I'm not sure how I could improve the while loop termination. The only reason I have that if statement is because I need to update the variables that caused the loop boundary to turn false. –  ybic Feb 17 at 15:34

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.