Skip to content

bracket-push: Add test case "partially paired brackets" #1219

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

Merged
merged 1 commit into from
Apr 6, 2018

Conversation

mrcfps
Copy link
Contributor

@mrcfps mrcfps commented Apr 5, 2018

Some incorrect solutions return true as soon as brackets are matched. Check out this solution in Python which can pass all current tests:

bracket_dict = {"(": ")", "[": "]", "{": "}"}


def brackets_match(opening_bracket, closing_bracket):
    return bracket_dict[opening_bracket] == closing_bracket


def is_paired(input_string):
    bracket_stack = []
    # Add every "opening" bracket to the stack
    # if we encounter a "closing" bracket, it should match the current
    # bracket on top of the stack
    for char in input_string:
        # If we encounter an opening bracket, we add it on top of the stack
        if char in bracket_dict.keys():
            bracket_stack.append(char)
        elif char in bracket_dict.values():
            # Get the bracket on top of the stack
            try:
                top_of_stack = bracket_stack.pop()
            # If the bracket_stack is empty, then we return false, since
            # the closing bracket has no opening bracket pair
            except IndexError:
                return False

            # Check and return if the opening and closing brackets match
            return brackets_match(top_of_stack, char)

    # Once done processing the string, bracket_stack should be empty
    # If empty, this means that all brackets have been matched
    # If not empty, this means that some brackets remain unmatched
    return bracket_stack == []

Clearly return brackets_match(top_of_stack, char) within the for loop is wrong, so it will fail test cases where brackets are partially paired, like {[]).

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I confirm that such a described implementation (early return) passes all tests but not the new one.

Please designate that in the commit message. The PR description was good because it explains why the test case is added; the information should be in the commit message as well so that the information is visible without having to visit GitHub.

Commits should be squashed.

If nothing else, around 24 hours from now I will make both things happen.

* Bump version to v1.3.0
* Guard against solutions returning `true` once brackets are matched,
which will pass all previous tests but the added one
@mrcfps mrcfps force-pushed the bracket-push-new-test-case branch from 1b91f69 to 4b84fed Compare April 6, 2018 12:43
@mrcfps
Copy link
Contributor Author

mrcfps commented Apr 6, 2018

@petertseng Done at your request.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

cool. I could quibble a bit about "once brackets are matched" -> "once any one pair of brackets is matched" to really distinguish it from solutions that check all pairs of brackets as they should, but I haven't the time to write down all my thoughts, and it's good as-is anyway.

@rpottsoh rpottsoh merged commit 9027186 into exercism:master Apr 6, 2018
@mrcfps mrcfps deleted the bracket-push-new-test-case branch April 7, 2018 00:46
petertseng added a commit to petertseng/exercism-ceylon that referenced this pull request Apr 7, 2018
Guards against solutions immediately declaring the entire string matched
as soon as they find any one matched pair.

exercism/problem-specifications#1219
petertseng added a commit to exercism/haskell that referenced this pull request Apr 7, 2018
Guards against solutions immediately declaring the entire string matched
as soon as they find any one matched pair.

exercism/problem-specifications#1219
petertseng added a commit to exercism/ceylon that referenced this pull request Apr 7, 2018
Guards against solutions immediately declaring the entire string matched
as soon as they find any one matched pair.

exercism/problem-specifications#1219
rpottsoh added a commit to exercism/delphi that referenced this pull request Apr 7, 2018
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.

4 participants