-
-
Notifications
You must be signed in to change notification settings - Fork 714
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
base: main
Are you sure you want to change the base?
Conversation
…required files from the resources template to the exercise
… covered | Sample Implementation for some of the read methods is done
…mentation in FileOperations
There was a problem hiding this comment.
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.
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"); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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.
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); | |
} | |
} |
import org.junit.jupiter.api.*; | ||
|
||
import java.io.*; | ||
import java.nio.charset.StandardCharsets; | ||
import java.util.Arrays; | ||
|
||
import static org.assertj.core.api.Assertions.*; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
@Test | |
@Disabled("Remove to run test") | |
@Test |
System.out.println("haha "); | ||
System.out.println(fileOperations.getReadOperationCount()); |
There was a problem hiding this comment.
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
.
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 |
There was a problem hiding this comment.
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.*; |
There was a problem hiding this comment.
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:
import java.io.*; | |
import java.io.Closeable; | |
import java.io.InputStream; | |
import java.io.OutputStream; |
public class PaasioTest { | ||
|
||
private ByteArrayInputStream dataInputStream; | ||
private ByteArrayOutputStream dataOutputStream; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public void setUPTest(TestInfo testInfo) { | |
public void setUpTest(TestInfo testInfo) { |
Thanks for time and that's really good explanation will do it 🤘 |
Heyyy I have made the requested changes could you please verify that |
@@ -0,0 +1,98 @@ | |||
import java.io.*; |
There was a problem hiding this comment.
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.*; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 | |||
}, | |||
{ |
There was a problem hiding this comment.
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?
if (!Objects.isNull(this.paasioReaderWriter.getInputStream())) { | ||
this.paasioReaderWriter.getInputStream().close(); | ||
} | ||
if (!Objects.isNull(this.paasioReaderWriter.getOutputStream())) { | ||
this.paasioReaderWriter.getOutputStream().close(); | ||
} |
There was a problem hiding this comment.
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.
|
||
|
There was a problem hiding this comment.
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.
byte[] initialMessage = "Hello! ".getBytes(); | ||
byte[] messageArray = Arrays.copyOf(initialMessage, 50); | ||
|
||
int length = messageArray.length - initialMessage.length - 2; |
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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."; |
There was a problem hiding this comment.
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)
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'); | ||
|
There was a problem hiding this comment.
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."; |
There was a problem hiding this comment.
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.
pull request
Hello, raising PR for paasio exercise implementation. Issue ID : #1813
Reviewer Resources:
Track Policies