Skip to content

Conversation

SharonIV0x86
Copy link

@SharonIV0x86 SharonIV0x86 commented Oct 4, 2024

Pull Request Title: Add Serializer and Deserializer Classes for Binary Serialization of Primitives and Strings

Description:

This pull request introduces a Serializer and Deserializer class for the serialization and deserialization of fundamental types such as int, double, char, and std::string in binary format. The feature supports both serialization of primitive types and strings, ensuring safe handling of binary data.

For primitives like int, float, and double, serialization is straightforward. However, serializing or deserializing std::string is more challenging due to its dynamic size and the absence of a delimiter (like null termination in C-style strings), which means we wouldn’t know how much to read or write.

To address this, the following approach is used:

Serializing:

  • Writing the size of the string immediately before the string data.
  • Adding a delimiter | at the end of the string to signify its boundary.

Deserializing:

  • Reading the size of the string before reading the actual string data to know how much to read.
  • Reading the string until the delimiter | is encountered.

Note: You can replace | with any other delimiter of your choice if needed.

Checklist:

  • Added a description of the change.
  • File name matches File Name Guidelines.
  • Added tests and example; tests must pass.
  • Added documentation to make the program self-explanatory and educational, following Doxygen guidelines.
  • Relevant documentation/comments are changed or added.
  • PR title follows semantic commit guidelines.
  • Searched for previous suggestions before making a new one to avoid duplication.
  • I acknowledge that all my contributions will be made under the project's license.

@realstealthninja realstealthninja added the awaiting review pull request is waiting to be reviewed label Oct 4, 2024
@realstealthninja realstealthninja added awaiting modification Do not merge until modifications are made and removed awaiting review pull request is waiting to be reviewed labels Oct 5, 2024
@realstealthninja
Copy link
Collaborator

Hey, thank you for the contribution question is this for hacktoberfest?

}

/**
* Serializes a string to a binary file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Serializes a string to a binary file.
* @brief Serializes a string to a binary file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

still needs to be resolved

* @param out The output stream (std::ofstream).
* @param data The string to be serialized.
*
* The string is serialized by first storing its length, followed by the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* The string is serialized by first storing its length, followed by the
* @note The string is serialized by first storing its length, followed by the

Copy link
Collaborator

Choose a reason for hiding this comment

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

still needs to be resolved


if (!outFile || !inFile) {
std::cerr << "Error opening files.\n";
return 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

main function should have nothing except
a call to the test function
and a return 0;
thus move all the tests from here to the test function

* Deserializer.
*/

void runTests() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
void runTests() {
void tests() {

Comment on lines 139 to 176
/**
* @brief A test suite to perform extensive testing on the Serializer and
* Deserializer.
*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/**
* @brief A test suite to perform extensive testing on the Serializer and
* Deserializer.
*/
/**
* @brief self test implementation
* @return void
*/

}
};

void runTests();
Copy link
Collaborator

Choose a reason for hiding this comment

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

tests dont really need to be forward declared just move the definition from the tests at the bottom to here

@SharonIV0x86
Copy link
Author

Hey, thank you for the contribution question is this for hacktoberfest?

Yes.

/**
* @file
* @brief A simple Serializer and Deserializer utility for fundamental data
* types and strings.
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should add more details via a @detail tag and your name with @author tag

}

/**
* Serializes a string to a binary file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

still needs to be resolved

* @param out The output stream (std::ofstream).
* @param data The string to be serialized.
*
* The string is serialized by first storing its length, followed by the
Copy link
Collaborator

Choose a reason for hiding this comment

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

still needs to be resolved


int intResult;
// Deserialize expecting binary data, this should fail
Deserializer::deserialize(inFile, intResult);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the failure here?

try {
std::ifstream inFile("non_existent_file.bin", std::ios::binary);
if (!inFile) {
throw std::runtime_error("Error opening non-existent file.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we checking the errors on the user end the only failures to be checked are the ones raised by the class itself.

* @return void
*/
void tests() {
try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this shouldnt be wrapped in a try catch loop. Tests should never fail.

Comment on lines 34 to 36
/** \namespace ciphers
* \brief Classes for binary Serialization and Deserialization
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/** \namespace ciphers
* \brief Classes for binary Serialization and Deserialization
*/
/**
* @namespace serialization
* @brief Classes for binary Serialization and Deserialization
*/

/** \namespace ciphers
* \brief Classes for binary Serialization and Deserialization
*/
namespace Serialization {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
namespace Serialization {
namespace serialization {

Copy link
Contributor

This pull request has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Author has not responded to the comments for over 2 weeks label Dec 12, 2024
Copy link
Contributor

Please ping one of the maintainers once you commit the changes requested or make improvements on the code. If this is not the case and you need some help, feel free to ask for help in our Gitter channel or our Discord server. Thank you for your contributions!

@github-actions github-actions bot closed this Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting modification Do not merge until modifications are made hacktoberfest stale Author has not responded to the comments for over 2 weeks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants