Skip to content
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

Add data conversion functions #341

Closed

Conversation

beardbytes
Copy link

Test cases for data validation function

#99

//converting each character to upper case using for loop
for (auto &c : str)
{
c = std::toupper(c); // using the function toupper() for conversion
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than just using std::toupper() and std::tolower(), I think it would be better to implement this manually (as done here, though there are lots of ways to do it). Obviously in "real life" you'd just use the std function, but the point of this repo is (generally) to implement things ourselves (i.e., sorting a list manually rather than just calling std::sort()). More good info here.

So, please implement these utilities without using the std functions. Let me know if you have any questions!

Copy link
Author

Choose a reason for hiding this comment

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

Okay...I will dive right into it !

Copy link
Collaborator

@alxmjo alxmjo left a comment

Choose a reason for hiding this comment

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

This is a good start! A few things to comment on.

I'm subscribed to the PR automatically, so no need to tag me in a separate comment saying it's ready for review.

These are actually conversion functions, so replace file names and comments with data_conversion.

While you've written the unit tests, you need to add them to CMakeLists.txt in order to run them. Add the following to the bottom of that file:

# ============================================================================
# Utilities
# ============================================================================

# Data conversion
add_executable(data_conversion
        test/utils/data_conversion.cpp)
target_link_libraries(data_conversion test_runner)

This will show you that some of your tests aren't actually passing.

Finally, I think that rather than using test cases to break out the tests into base and non-base cases, you should use the test cases for each function, like this:

TEST_CASE("Make upper case", "[string][data-validation][make-upper-case]")
{
    // upper case base cases

    // upper case other cases
}

TEST_CASE("Make lower case", "[string][data-validation][make-lower-case]")
{
    // lower case base cases

    // lower case other cases
}

Finally, a base case should test empty strings and strings with one character in them. Everything else can go under the other comment. And make sure to test other edge cases. Like, what happens with numbers (ints, floats)? What happens with special characters? What happens with spaces?

Let me know if you have any questions.

@alxmjo alxmjo linked an issue May 30, 2020 that may be closed by this pull request
@beardbytes
Copy link
Author

This is a good start! A few things to comment on.

I'm subscribed to the PR automatically, so no need to tag me in a separate comment saying it's ready for review.

These are actually conversion functions, so replace file names and comments with data_conversion.

While you've written the unit tests, you need to add them to CMakeLists.txt in order to run them. Add the following to the bottom of that file:

# ============================================================================
# Utilities
# ============================================================================

# Data conversion
add_executable(data_conversion
        test/utils/data_conversion.cpp)
target_link_libraries(data_conversion test_runner)

This will show you that some of your tests aren't actually passing.

Finally, I think that rather than using test cases to break out the tests into base and non-base cases, you should use the test cases for each function, like this:

TEST_CASE("Make upper case", "[string][data-validation][make-upper-case]")
{
    // upper case base cases

    // upper case other cases
}

TEST_CASE("Make lower case", "[string][data-validation][make-lower-case]")
{
    // lower case base cases

    // lower case other cases
}

Finally, a base case should test empty strings and strings with one character in them. Everything else can go under the other comment. And make sure to test other edge cases. Like, what happens with numbers (ints, floats)? What happens with special characters? What happens with spaces?

Let me know if you have any questions.

Alright . I will keep that in mind .

These are actually conversion functions, so replace file names and comments with data_conversion ->> Which file names to replace ?

@alxmjo
Copy link
Collaborator

alxmjo commented May 30, 2020

  • Replace data_validation.hpp with data_conversion.hpp
  • Replace data_validation.cpp with data_conversion.cpp
  • Fix any comment that references data validation.

@beardbytes
Copy link
Author

Alright ! Got it

@beardbytes
Copy link
Author

See the red circle with the line through it on the line above? Make sure to add a newline at the end of all of your files. See here: https://stackoverflow.com/questions/5813311/no-newline-at-end-of-file

Okay ! Got it .

@beardbytes beardbytes force-pushed the test_data_validation_branch branch from b6bf98c to a106f3f Compare May 30, 2020 19:25
@alxmjo
Copy link
Collaborator

alxmjo commented May 30, 2020

See in this thread under "Changes requested" where it says "All checks have failed?" At least one of your unit tests is currently failing, so please fix that.

Also, remove the old data_validation files.

Finally, please break out the unit tests into base cases (with empty string and single character) and other cases, with everything else. These don't need to be entirely other test cases, just a comment separating, like so:

TEST_CASE("Make upper case", "[string][data-validation][make-upper-case]")
{
    // Upper case base cases
    REQUIRE(...)
    REQUIRE(...)
    
    // Upper case other cases
    REQUIRE(...)
    REQUIRE(...)
    ...
}

@beardbytes
Copy link
Author

See in this thread under "Changes requested" where it says "All checks have failed?" At least one of your unit tests is currently failing, so please fix that.

Also, remove the old data_validation files.

Finally, please break out the unit tests into base cases (with empty string and single character) and other cases, with everything else. These don't need to be entirely other test cases, just a comment separating, like so:

TEST_CASE("Make upper case", "[string][data-validation][make-upper-case]")
{
    // Upper case base cases
    REQUIRE(...)
    REQUIRE(...)
    
    // Upper case other cases
    REQUIRE(...)
    REQUIRE(...)
    ...
}

Yes...I am figuring out why the tests are failing .
Okay i will separate the cases from base cases !

@beardbytes
Copy link
Author

I am getting this error
g++ data_conversion.cpp data_conversion.cpp:1:10: fatal error: third_party/catch.hpp: No such file or directory #include "third_party/catch.hpp" ^~~~~~~~~~~~~~~~~~~~~~~ compilation terminated.

@alxmjo
Copy link
Collaborator

alxmjo commented Jun 9, 2020

You still have data_validation files included in your commit. Remove those. As for your error, I checkouted out all of your files on my machine and I cannot reproduce it. Make sure you're in C++ directory and then run make and then make test. You should see that one of your tests is still failing.

@alxmjo alxmjo changed the title Test data validation branch Add data conversion Jun 9, 2020
@alxmjo alxmjo changed the title Add data conversion Add data conversion functions Jun 9, 2020
@stale
Copy link

stale bot commented Sep 13, 2020

This issue has been automatically marked as inactive because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Inactive label Sep 13, 2020
@stale stale bot closed this Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement data validation
2 participants