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.

This is my code which receives a file to read and converts binary values to ASCII text.

The code works fine, so I am looking for a review on what I have written, and suggestions on how I could do it better, perhaps without the use of a StringBuffer.

import java.io.File;
import java.util.Scanner;

/**
 *
 * @author Tumpi
 */
public class DN6 {

public static void main(String[] args)
        throws Exception {

    File file = new File("sporocilo.txt");

    Scanner sc = new Scanner(file);
    String lastString = "";

    while (sc.hasNextLine()) {
        String line = sc.nextLine();
        lastString = lastString + line;
    }
        StringBuffer result = new StringBuffer();
        for (int i = 0; i < lastString.length(); i += 8) {
            result.append((char) Integer.parseInt(lastString.substring(i, i + 8), 2));
        }
        System.out.println(result);
    } 

}
share|improve this question
2  
Why not use a StringBuffer? –  Uri Agassi Apr 2 at 18:02
1  
Your question is not a review question, it is a question on about how to do something with code, if you read the codereview.stackexchange.com/about page you can better familiarize yourself with what questions to ask here. Your question would be better asked on StackOverflow –  BenVlodgi Apr 2 at 18:02
3  
Is this code single thread or not ? If it's single thread you should use StringBuilder instead of StringBuffer.StringBuffer is use when there are multithreading. –  Marc-Andre Apr 2 at 18:02
    
I wouldn't ask this on SO. It is possible to use simple string concatenation str = str + "more"; but I wouldn't recommend that, a StringBuilder is really the best way to go. –  Simon André Forsberg Apr 2 at 18:13
    
Or you could use a simple char[] of course. –  Simon André Forsberg Apr 2 at 18:14

5 Answers 5

up vote 10 down vote accepted

Aside from turning StringBuffer into StringBuilder, there are a few more things I'd like to note about your code.

Compound assignment operator

This line:

 lastString = lastString + line;

Can be rewritten as this one:

 lastString += line;

It's short and it complies with what many people would expect: a concatenation. By explicitly mentioning the original variable again, many people might think it is a different variable before looking at it more closely.

Additionally, you might also want to add a StringBuilder to the reading of the file. Even if the inputfile isn't that big, it would still be consistent.

Magic values

Consider this code:

for (int i = 0; i < lastString.length(); i += 8) {
     result.append((char) Integer.parseInt(lastString.substring(i, i + 8), 2));
}

Can someone other than the authro tell with certainty what the value 8 is? An educated guess would lead to the bit representation of 2 bytes, but we shouldn't have to guess that. Get rid of this so called "magic value" by adding a variable which clears it up:

int amountOfBits = 8;
for (int i = 0; i < lastString.length(); i += amountOfBits ) {
     result.append((char) Integer.parseInt(lastString.substring(i, i + amountOfBits ), 2));
}

Exception handling

You're completely bypassing exception handling. I'm assuming this is just so this contrived sample wouldn't get cluttered with code, but just in case it isn't:

throws Exception

is always a bad idea in an actual program. You should catch the exact exception and handle it appropriately.

share|improve this answer

You should be using StringBuilder instead of StringBuffer, since StringBuffer has synchronization for thread safety that you don't need.

To make the code more modular and reusable, consider writing it as a FilterInputStream instead.

share|improve this answer
    
+ 1 since I can't agree more with what I've said in comments :D ! I would suggest using a second StringBuilder for the first for loop too ! (If you want to add it!) –  Marc-Andre Apr 2 at 18:17
1  
@Marc-Andre Concrete suggestions for improvements should be answers, not comments, even if they are short. –  200_success Apr 2 at 18:36

In adition to everything else being said, I would like to point out that your tabbing is done incorrectly, and would look much sexier like this

import java.io.File;
import java.util.Scanner;

/**
 * @author Tumpi
 */
public class DN6 {
    public static void main(String[] args)
            throws Exception {

        File file = new File("sporocilo.txt");

        Scanner sc = new Scanner(file);
        String lastString = "";

        while (sc.hasNextLine()) {
            String line = sc.nextLine();
            lastString = lastString + line;
        }

        StringBuffer result = new StringBuffer();
        for (int i = 0; i < lastString.length(); i += 8) {
            result.append((char) Integer.parseInt(lastString.substring(i, i + 8), 2));
        }
        System.out.println(result);
    }
}
share|improve this answer

Because of the result String being of a size that can be pre-determined, and you are handling chars on the way, I would use a char[] (char array), and transforming the char array to a String using a String constructor.

I believe this is more efficient than using a StringBuilder or any other approach.

String lastString = "0100100001100101011011000110110001101111001000000101011101101111011100100110110001100100";
char[] result = new char[lastString.length() / 8];
for (int i = 0; i < lastString.length(); i += 8) {
    String sub = lastString.substring(i, i + 8);
    result[i / 8] = (char) Integer.parseInt(sub, 2);
}
System.out.println(new String(result));

You could also store the resulting string length in a variable, and switch the i variable to only iterate from 0 to the resulting string length.

String lastString = "0100100001100101011011000110110001101111001000000101011101101111011100100110110001100100";
int resultLength = lastString.length() / 8;
char[] result = new char[resultLength];
for (int i = 0; i < resultLength; i++) {
    String sub = lastString.substring(i * 8, (i + 1) * 8);
    result[i] = (char) Integer.parseInt(sub, 2);
}
System.out.println(new String(result));
share|improve this answer
  • You absolutely must close that Scanner.

  • This snippet

The scanner loop

while (sc.hasNextLine()) {
    String line = sc.nextLine();
    lastString = lastString + line;
}

creates a new String Object every time it loops. I also have no idea of how large you file is, but this will ultimately read the entire file into memory. In fact if you want to fully build lastString as you do here, you're better of using a StringBuilder.

  • Can you avoid StringBuffer (and StringBuilder)? Well, certainly. You can handle the file character by character, and print everything to the System.out character by character as well :

example :

public static void main(String[] args) {

    File file = new File("sporocilo.txt");

    try (Reader reader = new InputStreamReader(new FileInputStream(file))) {
        int c = 0;
        int count = 0;
        for (int readChar = reader.read(); readChar != -1; readChar = reader.read()) {
            switch (readChar) {
                case '0':
                case '1':
                    c <<= 1;
                    c |= readChar - '0';
                    count++;
                    break;
                default:
            }
            if (count == 8) {
                System.out.print((char) c);
                c = 0;
                count = 0;
            }
        }
        System.out.println();
    } catch (IOException e) {
        e.printStackTrace();
    }
}
share|improve this answer
    
Filling a char[] array is generally more efficient than calling .read() to fetch a character at a time. Correctly implementing the array version is tricker, though. –  200_success Apr 3 at 7:55
    
True, if performance turns out to be problematic, that may be a possible refactoring. –  bowmore Apr 3 at 16:10
    
I wouldn't call that refactoring — just rewriting. –  200_success Apr 3 at 16:13

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.