Skip to content

Passio Practice Exercise implemented #2970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

shreyasgosavi
Copy link
Contributor

pull request

Hello, raising PR for paasio exercise implementation. Issue ID : #1813


Reviewer Resources:

Track Policies

@shreyasgosavi shreyasgosavi marked this pull request as draft July 5, 2025 05:17
@kahgoh kahgoh changed the title Passi Practice Exercise implemented Passio Practice Exercise implemented Jul 5, 2025
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should just contain enough of the stub for students to start. Have a look at the stub for the Poker for example. It only contains the bare methods for students to fill.

When the student starts the Paasio exercise, the student will start with the contents of this file. They should not start with the solution.

Comment on lines 23 to 34
void checkReadOperationCount() {

try (Paasio customFileReader = new Paasio(this.dataInputStream, this.dataOutputStream)) {
customFileReader.read();
customFileReader.read();
customFileReader.read();
assertThat(customFileReader.getReadOperationCount()).isEqualTo(3);
} catch (IOException ioException) {
System.out.println("Exception occured");
}

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't catch the exception in the tests, othewise the tests will pass if an exception is thrown. Instead, let propagate the exception so that JUnit also sees the exception.

Suggested change
void checkReadOperationCount() {
try (Paasio customFileReader = new Paasio(this.dataInputStream, this.dataOutputStream)) {
customFileReader.read();
customFileReader.read();
customFileReader.read();
assertThat(customFileReader.getReadOperationCount()).isEqualTo(3);
} catch (IOException ioException) {
System.out.println("Exception occured");
}
}
void checkReadOperationCount() throw Exception {
try (Paasio customFileReader = new Paasio(this.dataInputStream, this.dataOutputStream)) {
customFileReader.read();
customFileReader.read();
customFileReader.read();
assertThat(customFileReader.getReadOperationCount()).isEqualTo(3);
}
}

Comment on lines 1 to 7
import org.junit.jupiter.api.*;

import java.io.*;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;

import static org.assertj.core.api.Assertions.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use * imports, as per other tests (poker for example).


byte[] readByteData = new byte[5];
fileOperations.read(readByteData);
assertThat(fileOperations.getBytesRead()).isEqualTo(9);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be interesting to assert on the contents readByteData.


}

@Test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, only the first test should be "enabled" in practice exercises. Students enable the test as they work through the exercise.

Suggested change
@Test
@Disabled("Remove to run test")
@Test

Comment on lines 178 to 179
System.out.println("haha ");
System.out.println(fileOperations.getReadOperationCount());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all System.out.println.

Suggested change
System.out.println("haha ");
System.out.println(fileOperations.getReadOperationCount());
System.out.println("haha ");
System.out.println(fileOperations.getReadOperationCount());

config.json Outdated
"uuid": "81177978-2ed3-47c2-a6a0-fef316d94c0b",
"practices": [],
"prerequisites": [],
"difficulty": 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really shouldn't be a one (it's not that easy). Perhaps we could use the Go track as a guide - they have used 4 as the difficulty.

@@ -0,0 +1,140 @@
import java.io.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use * imports. For example:

Suggested change
import java.io.*;
import java.io.Closeable;
import java.io.InputStream;
import java.io.OutputStream;

public class PaasioTest {

private ByteArrayInputStream dataInputStream;
private ByteArrayOutputStream dataOutputStream;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since every test creates and uses a Paasio object, we could also use a Passio field and set it in setUpTest and then close it after every test in a @AfterEach method.

private static final String MESSAGECONSTANT = "This is additional Content.";

@BeforeEach
public void setUPTest(TestInfo testInfo) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void setUPTest(TestInfo testInfo) {
public void setUpTest(TestInfo testInfo) {

@shreyasgosavi
Copy link
Contributor Author

Thanks for time and that's really good explanation will do it 🤘

@shreyasgosavi shreyasgosavi marked this pull request as ready for review August 5, 2025 19:58
@shreyasgosavi
Copy link
Contributor Author

Heyyy I have made the requested changes could you please verify that

@@ -0,0 +1,98 @@
import java.io.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned earlier, please don't use * for import

@@ -0,0 +1,149 @@
import java.io.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for this!

assertThat(paasioReaderWriter.getBytesRead()).isEqualTo(4);
}

@Test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every test except the first one should have @Disabled("Remove to run test")

@@ -1900,6 +1900,14 @@
"lists"
],
"difficulty": 10
},
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move this exercise up with the other ones that have a difficulty of 4, and make sure it stays in alphabetical order within that section?

Comment on lines +33 to +38
if (!Objects.isNull(this.paasioReaderWriter.getInputStream())) {
this.paasioReaderWriter.getInputStream().close();
}
if (!Objects.isNull(this.paasioReaderWriter.getOutputStream())) {
this.paasioReaderWriter.getOutputStream().close();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be simplified to simply calling passioReaderWriter.close() and might allow you to remove getInputStream and getOutputStream methods from the Passio class.

Comment on lines +75 to +76


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be extra spacing here.

Suggested change

byte[] initialMessage = "Hello! ".getBytes();
byte[] messageArray = Arrays.copyOf(initialMessage, 50);

int length = messageArray.length - initialMessage.length - 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure - why do we need to subtract 2 here?

@Disabled("Remove to run test")
void checkByteValueReadFromTheFile() throws IOException {

byte[] fileData = new byte[100];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this fileData name came from an earlier iteration where we had a distinction between file and network data. However, we no longer have this distinction, so I'd suggest renaming it. Perhaps just data?

byte[] dataRead = Arrays.copyOf(helloArray, 100);

int bytesRead = paasioReaderWriter.read(dataRead, helloArray.length, 40);
String finalVAlue = new String(dataRead, 0, helloArray.length + bytesRead, StandardCharsets.UTF_8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String finalVAlue = new String(dataRead, 0, helloArray.length + bytesRead, StandardCharsets.UTF_8);
String finalValue = new String(dataRead, 0, helloArray.length + bytesRead, StandardCharsets.UTF_8);

byte[] byteData = new byte[50];
int bytesRead = paasioReaderWriter.read(byteData, 0, 10);

assertThat(bytesRead).isEqualTo(10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also assert the contents byteData as it should write only 10 bytes to the array. This should be less than the available content?

Should there also be a test for when the length (or the "10") is more than what is available?

void validateReadNBytesReadOperationCount() throws IOException {

paasioReaderWriter.readNBytes(5);
paasioReaderWriter.readNBytes(5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are also missing tests that assert on the byte[] that is returned by the readNBytes method.


private ByteArrayInputStream dataInputStream;
private ByteArrayOutputStream dataOutputStream;
private static final String MESSAGECONSTANT = "This is additional Content.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is more idiomatic to use screaming snake case for constants in Java. Could you please also move this to just under the public class PaasioTest { line? (its more idiomatic that way)

Suggested change
private static final String MESSAGECONSTANT = "This is additional Content.";
private static final String MESSAGE_CONSTANT = "This is additional Content.";

paasioReaderWriter.write(writeFileContent);
paasioReaderWriter.write(writeFileContent);
paasioReaderWriter.write('s');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is missing asserting on the contents that are written to the dataOutputStream.

@Disabled("Remove to run test")
void verifyBytesWrittenFromOffsetIntoTheOutputStreamAlongWithTheCount() throws IOException {

String fileContentToBeWritten = " This is additional Content.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to another comment, this isn't "file" specific, so suggest renaming the variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants