Why is my code not working?

Question about the task

Under discussion

The correct solution suggests we use try-with-resources to complete the task. I understand that's a more beautiful way, but I'd like to use the try-catch-finally formula just in order to better understand how all this works. So, the validator implies that I'm not using the newInputStream and newOutputStream methods of the Files class to read and write files (while I think I am using them in lines 19 & 20). Another requirement my code doesn't pass is that streams for reading and writing must be closed. Isn't that exactly what my final block is doing?

Write a program that reads the path to file1 and the path to file2 from the console. Then it writes all the bytes from file1 to file2, but in doing so it swaps them according to this rule: swap the first with the second, the third with the fourth, and so on.
If the last byte in file1 is odd, then write it to file2 as is. To read and write files, use the newInputStream and newOutputStream methods of the Files class.

Requirements:

The program should read file paths from the console.
The program must rewrite bytes from one file to another in accordance with the task conditions.
To read and write files, the newInputStream and newOutputStream methods of the Files class should be used.
Streams for reading and writing must be closed.
Do not use the following classes from the java.io package: File, FileInputStream, FileOutputStream, FileReader, FileWriter.

package en.codegym.task.pro.task15.task1504; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Paths; import java.util.Scanner; /* Mixed-up bytes */ public class Solution { public static void main(String[] args) throws IOException { Scanner console = new Scanner(System.in); var path1 = Paths.get(console.nextLine()); var path2 = Paths.get(console.nextLine()); InputStream inputStream = Files.newInputStream(path1); OutputStream outputStream = Files.newOutputStream(path2); try { byte[] inputBytes = inputStream.readAllBytes(); byte[] outputBytes = new byte[inputBytes.length]; int lastIndex = inputBytes.length - 1; if (inputBytes.length % 2 == 0) { for (int i = 0; i < lastIndex; i += 2) { outputBytes[i + 1] = inputBytes[i]; outputBytes[i] = inputBytes[i + 1]; } } else { for (int i = 0; i < lastIndex - 1; i += 2) { outputBytes[i + 1] = inputBytes[i]; outputBytes[i] = inputBytes[i + 1]; } outputBytes[lastIndex] = inputBytes[lastIndex]; } outputStream.write(outputBytes, 0, outputBytes.length); } catch (IOException exIO) { System.out.println("Something went wrong: " + exIO); } finally { inputStream.close(); outputStream.close(); } } }

Comments (7)

Popular
New
Old

You must be signed in to leave a comment

Guadalupe Gagnon

Level 37 , Tampa, United States

1 June, 03:13

Codegyms correct solution is not "the" only solution. I highly recommend to everyone i have talked to that they do not download codegyms solutions until they have failed a lot, asked for help, and still are unable to figure it out on their own. I think Thomas is correct in wondering about handling the possibility of a null Stream. Here some of my thoughts/tips: 1) don't forget to close the Scanner 2) you don't need 2 separate byte[] arrays and you don't need to check if the byte[] arrays length is evenly divisible by 2. See this code that should be able to replace 23-37:

byte temp;
for (int i = 0; i < inputBytes.length; i += 2) {
   temp =  inputBytes[i-1];
   inputBytes[i-1] = inputBytes[i];
   inputBytes[i] = temp;
}
outputStream.write(inputBytes);

(you also don't need the start and end when passing a byte[] you intend to write in full) 2) you should try to write as little code as possible inside a try block because every line of code inside a try block has the overhead of being checked for thrown exceptions. This will unnecessarily slow down your code, which may not make a difference on these small tasks but could be a very bad habit when you start programming professionally. 3) Streams are also very resource intensive. It is good practice to open them when you need to use them and close them as soon as possible thereafter. In this code, if I programmed it, I would do it in this order: 1 - open stream for console 2 - read necessary data 3 - close console stream 4 - open stream to read bytes 5 - read bytes 6 - close the byte reading stream 7 - run the algorithm to swap bytes according to task 8 - open stream to write result 9 - write the bytes 10 - close the wtriting stream

Thomas

Level 41 , Bayreuth, Germany

1 June, 06:52

I am with you just with the exception of excetptions (haha), your second no. 2. One of course can pack a lot of code inside a try block and that won't slow down the program. The compiler does not add code that checks after each line if there has been an exception. So there is no penalty. The only thing that's time consuming is the process of constructing the excetpion itself. It works more the way that if there is an exception then the code takes another route. Am I in a try block? Is there a catch block? No? Remove the current level from the stack and check now. Am I in a try block... and so on. That is not related to the lines of code inside the try block. In all cases it is just one jmp to the exception handling. I even benchmarked that with a consumer taking some time with and without try-catch

Benchmark                        Mode  Cnt  Score    Error  Units
ExceptionBenchmark.catchBlock    avgt   10  0,018 ±  0,001  ms/op
ExceptionBenchmark.noCatchBlock  avgt   10  0,018 ±  0,001  ms/op

But I also think it is not really the way to go to gather all code in a catch all try block. It always should be reduced to the minimum, but for semantic reasons. So overall we have similar solutions with the difference that I pushed the algorithm into a method. The downside here is (as commented), that, with also return the object, there would be the side effect of changing the original array. Hence the copy. Other way would be to not return the array and instead have a void method. Whatever way you chose I'd always go the way to create a separate method for the logic and just use a meaningful name that I can call inside the main code (no matter if the cost is a millisecond or not). Just if there are performande issues I'd make the main code less readable for the benefit of performance.

Guadalupe Gagnon

Level 37 , Tampa, United States

1 June, 19:16

It was my understanding that try-catch adds overhead; but after some research it seems that Java's JVM has little to no overhead for try/catch statement except when an Exception made (like you said). I read a bunch of different books here and there on many different programming languages while I was initially learning how to program and I probably picked that up from one of those sources (maybe from c++ book?).

Thomas

Level 41 , Bayreuth, Germany

2 June, 10:19

I am very glad that you are not well versed in all aspects of the Java language. It's enough that you have the ambition to get to the bottom of every error. For example, in the other question, the regex looked fishy to me. But I was too lazy to test it and instead I posted the version I thought of when I saw the code. You on the other hand... Long story short, I always enjoy reading your comments ;)

Evgeniia Shabaeva

Level 25 , Budapest, Hungary

3 June, 13:06

Thank you both for your comments. With your help, I eventually wrote quite cumbersome code (as I really wanted to do it without "resources" in order to sort out how the whole algorithm works). So, my version is really clumsy, but it's passed validation:) And now I feel I do understand why try-with-resources is better:))

public class Solution {

    public static void main(String[] args) {
        Scanner console = null;
        InputStream inputStream = null;
        OutputStream outputStream = null;
        try {
            console = new Scanner(System.in);
            inputStream = Files.newInputStream(Paths.get(console.nextLine()));
            outputStream = Files.newOutputStream(Paths.get(console.nextLine()));
            byte[] inputBytes = inputStream.readAllBytes();
            byte[] outputBytes = new byte[inputBytes.length];
            int lastIndex = inputBytes.length - 1;
            for (int i = 0; i <= lastIndex; i += 2) {
                if (i != lastIndex) {
                    outputBytes[i + 1] = inputBytes[i];
                    outputBytes[i] = inputBytes[i + 1];
                }
                else outputBytes[i] = inputBytes[i];
            }
            outputStream.write(outputBytes);
        } catch (IOException e) {
            System.out.println("Something went wrong: " + e);
        } finally {
            try{
            if (console != null)
                console.close();
            if (inputStream != null)
                inputStream.close();
            if (outputStream != null)
                outputStream.close();
            } catch (IOException e1) {
                System.out.println("Something can't close because of " + e1);
            }
        }
    }
}

Guadalupe Gagnon

Level 37 , Tampa, United States

3 June, 13:25

Thanks Thomas! I really appreciate the positive feed back. I have enjoyed readying you're answers as well, going back to when you had your first account (I think that was nouser). You always have good insight and come up with clever solutions to complicated sections.

Thomas

Level 41 , Bayreuth, Germany

31 May, 13:19

OK, I do not know what validation is exactly looking for here, but I can tell you, that closing resources is not correct. This will fail if a) one of the resources is null in the beginning (just force that with eg. InputStream inputStream = null;) b) close() may throw an IOException, too. That you need to handle in the finally block So your finally block is missing null checking for in and out stream and handling the IOException. This all would do try-with-resources for you.

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

    try (BufferedReader console = new BufferedReader(new InputStreamReader(System.in));
         InputStream in = Files.newInputStream(Paths.get(console.readLine()));
         OutputStream out = Files.newOutputStream(Paths.get(console.readLine()))) {

        byte[] bytes = in.readAllBytes();
        out.write(swapBytes(bytes));

    } catch (IOException ioe) {
        throw new UncheckedIOException(ioe);
    }
}

private static byte[] swapBytes(byte[] bytes) {
    // avoid side effects
    byte[] copy = Arrays.copyOf(bytes, bytes.length);
    for (int i = 1; i < copy.length; i += 2) {
        swap(i, copy);
    }
    return copy;
}

private static void swap(int i, byte[] bytes) {
    byte temp = bytes[i];
    bytes[i] = bytes[i - 1];
    bytes[i - 1] = temp;
}

(no clue if that'll validate, just some approach)