2

This method is supposed to take a string and detect if the brackets '(' '{' '[' in the string are closing properly with the corresponding (opposite) brackets.

First, is there a more elegant, compact way to write this bit without using all the "or"s (||):

            split_array.each do |i| 
              if (i == "{" || i == "(" || i == "[")
                  left.push(i)
                else (i == "}" || i == ")" || i == "]")
                  right.push(i)
                end
             end

My second question is, is this code terrible (see below)? It seems I should be able to write this in way fewer lines, but logically, I haven't come up with another solution (yet.) The code works for most tests, but it returns false for this test (see all driver tests at bottom): p valid_string?("[ ( text ) {} ]") == true

Any critique would be greatly appreciated! (also, if there is a better section to post this, please let me know) Thanks!

def valid_string?(string)

    opposites = { "[" => "]", "{" => "}", "(" => ")", "]" => "[", "}" => "{", ")" => "(" }

        left = Array.new
        right = Array.new
        return_val = true

        split_array = string.split(//)
        split_array.delete_if { |e| e.match(/\s/) }

          split_array.each do |i| 
          if (i == "{" || i == "(" || i == "[")
              left.push(i)
            else (i == "}" || i == ")" || i == "]")
              right.push(i)
            end
          end

        # p left
        # p right

        left.each_index do |i|
          if left[i] != opposites[right[i]]
              return_val = false
          end
        end  
        return_val
    end 

    p valid_string?("[ ] } ]") == false
    p valid_string?("[ ]") == true
    p valid_string?("[  ") == false                 
    p valid_string?("[ ( text ) {} ]") == true    
    p valid_string?("[ ( text { ) } ]") == false  
    p valid_string?("[ (] {}") == false 
    p valid_string?("[ ( ) ") == false

-------Updated: After trying some different approaches, my refactor is this:-----------

def valid_string?(str)

    mirrored = { "[" => "]", "{" => "}", "(" => ")" }
    open_brackets = Array.new

    split_str_array = str.split("")

    split_str_array.each do |bracket| 
      if bracket.match(/[\[|\{|\(]/) then open_brackets.push(bracket)
      elsif bracket.match(/[\]|\}|\)]/)
        return false if mirrored[open_brackets.pop] != bracket
      end
    end
    open_brackets.empty?
end 
user229044
  • 232,980
  • 40
  • 330
  • 338
Ryan Rebo
  • 1,278
  • 2
  • 13
  • 27
  • 1
    This belongs on [Code Review](http://codereview.stackexchange.com/) – Mark C. Mar 02 '14 at 00:13
  • Thanks for the feedback. Is there a way to move it? – Ryan Rebo Mar 02 '14 at 00:13
  • I flagged it for a Mod.. They should be able to move it for ya – Mark C. Mar 02 '14 at 00:14
  • 1
    You will not get good answers unless you write it more reader-friendly. Don't start with a long chunk of code without explaining what it does. You are writing the usage of your code at the very end. You should write that at the beginning. Until we reach that part at the end, all of your code is just a meaningless junk. – sawa Mar 02 '14 at 00:18
  • @sawa I agreed with you. But I think OP is checking *paren* matching. If open and closed *paren* present, then OP flagged as *valid* / *true* – Arup Rakshit Mar 02 '14 at 00:20
  • @ArupRakshit I had to scroll all the way down to know that. – sawa Mar 02 '14 at 00:21
  • @sawa Good call. I edited it to hopefully be more clear. – Ryan Rebo Mar 02 '14 at 00:23
  • Now that you showed that you wanted to check for balanced parentheses, it turns out that your entire approach is wrong. What you are trying to do could be done with a single regular expression match (a one liner). Unfortunately, you should throw away your entire method `valid_string?`. And for the reader, reading that was just a waste of time. That is why writing your objective first is important. – sawa Mar 02 '14 at 00:24
  • 1
    Why not just pop every time you see an appropriate closing bracket? Why two stacks? – Dave Newton Mar 02 '14 at 00:25
  • @sawa Hmmmm. Yeah? I started trying to do this with RegExp's, but hit a wall when trying to match "nested" or "overlapping" brackets. Ex: "[ ( text { ) } ]" – Ryan Rebo Mar 02 '14 at 00:28
  • possible duplicate of [Matching balanced parenthesis in Ruby using recursive regular expressions like perl](http://stackoverflow.com/questions/6331065/matching-balanced-parenthesis-in-ruby-using-recursive-regular-expressions-like-p). See the regex solution there. – sawa Mar 02 '14 at 00:29
  • @DaveNewton Good point. The two stacks were unnecessary. I've added a refactor above – Ryan Rebo Mar 02 '14 at 18:48
  • https://codereview.stackexchange.com/a/125859 – Nithin Mar 02 '19 at 15:06

8 Answers8

4

My approach is as below :

def valid_string?(string)
  open_paren = ['[','{','(']
  close_paren = [']','}',')']
  open_close_hash = {"]"=>"[", "}"=>"{", ")"=>"("}
  stack = []
  regex = Regexp.union(close_paren+open_paren)
  string.scan(regex).each do |char|
    if open_paren.include? char
      stack.push(char)
    elsif close_paren.include? char
      pop_val = stack.pop
      return false if pop_val != open_close_hash[char]
    end
  end
  open_paren.none? { |paren| stack.include? paren }
end 

valid_string?("[ ] } ]") # => false
valid_string?("[ ]") # => true
valid_string?("[  ") # => false
valid_string?("[ (] {}") # => false
valid_string?("[ ( ) ") # => false
valid_string?("[ ( text { ) } ]") # => false
valid_string?("[ ( text ) {} ]") # => true

Algorithm :

  1. Declare a character stack S.
  2. Now traverse the expression string exp.
    • If the current character is a starting bracket (‘(‘ or ‘{‘ or ‘[') then push it to stack.
    • If the current character is a closing bracket (')' or '}' or ']') then pop from stack and if the popped character is the matching starting bracket then fine else parenthesis are not balanced.
  3. After complete traversal, if there is some starting bracket left in stack then “not balanced”
Arup Rakshit
  • 116,827
  • 30
  • 260
  • 317
3

The shortest regex solution is probably:

def valid_string? orig
  str = orig.dup
  re = /\([^\[\](){}]*\)|\[[^\[\](){}]*\]|\{[^\[\](){}]*\}/
  str[re] = '' while str[re]
  !str[/[\[\](){}]/]
end
pguardiario
  • 53,827
  • 19
  • 119
  • 159
1

How about:

class Brackets
  def self.paired?(s)
    stack = []
    brackets = { '{' => '}', '[' => ']', '(' => ')' }

    s.each_char do |char|
      if brackets.key?(char)
        stack.push(char)
      elsif brackets.values.include?(char)
        return false if brackets.key(char) != stack.pop
      end
    end
    stack.empty?
  end
end


Brackets.paired?("[ ] } ]") # => false
Brackets.paired?("[ ]") # => true
Brackets.paired?("[  ") # => false
Brackets.paired?("[ (] {}") # => false
Brackets.paired?("[ ( ) ") # => false
Brackets.paired?("[ ( text { ) } ]") # => false
Brackets.paired?("[ ( text ) {} ]") # => true
mawaldne
  • 3,919
  • 6
  • 31
  • 37
1

You can try this approach:

def balanced_brackets?(string)
  # your code here
  stack = []
  opening_bracket = ['{','[', '(']
  closing_bracket = ['}', ']', ')']
  string.chars.each do |char|
    if opening_bracket.include?(char)
      stack << char
    elsif closing_bracket.include?(char)
      value = stack.pop
      return false if opening_bracket.index(value) != closing_bracket.index(char)
    end
  end
  
  stack.empty?
end

if you want to understand pseudo code try this link from coursera (starts at 0:56).

0

This should provide the same functionality

def valid_string?(string)
  #assume validity
  @valid = true
  #empty array will be populated inside the loop
  @open_characters = []
  #set up a hash to translate the open character to a closing character
  translate_open_closed = {"{" => "}","["=>"]","("=>")"}
  #create an array from the string loop through each item 
  string.split('').each do |e| 
    #adding it to the open_characters array if it is an opening character
    @open_characters << e if e=~ /[\[\{\(]/
    #if it is a closing character then translate the last open_character to 
    #a closing character and compare them to make sure characters are closed in order
    #the result of this comparison is applied to the valid variable
    @valid &= e ==  translate_open_closed[@open_characters.pop] if e=~ /[\]\}\)]/
  end
  #return validity and make sure all open characters have been matched
  @valid &= @open_characters.empty?
end

You could also do this with inject but it would be a bit less transparent.

engineersmnky
  • 25,495
  • 2
  • 36
  • 52
  • 1
    it is += for boolean objects. so something like this `valid = true; valid &= false` so `valid` will now be `false` – engineersmnky Mar 02 '14 at 01:37
  • Ohkay... It was looking very odd to me, so I couldn't recognize it. :-) – Arup Rakshit Mar 02 '14 at 01:40
  • @engineersmnky That's cool. I especially like the match with RegExp instead of the way I had it. Correct me if I'm wrong, are the `"|"` in the regular expressions basically saying 'match this "OR" this "OR" this? And is `e =~ /[\[|\{|\(]/` the same as say, `e.match(/[\[|\{|\(]/)` ? – Ryan Rebo Mar 02 '14 at 02:24
  • Yes the single pipe `|` in a Regex means "OR". And `=~` actually returns the index of the first match `.match()` returns a MatchData Object but yes they serve a similar purpose. – engineersmnky Mar 02 '14 at 02:27
  • 1
    @engineersmnky Actually, you use the `|` INSIDE a Regexp character class, which processes it literally. Your Regexp will now also match the '|' character. `'|' =~ /[\[|\{|\(]/ # => 0`. You should leave out the `|` inside the character classes. A character class is already an 'OR', i.e., `/[abc]/` means 'a' OR 'b' OR 'c'. Alternatively, you can remove the characters from the character class, i.e., `/\[|\{|\(/` (removed opening `[` and closing `]` which define the character class). – Daniël Knippers Mar 02 '14 at 09:15
  • @DaniëlKnippers you are correct thank you for the education Regex is not my strong point. – engineersmnky Mar 03 '14 at 00:29
0

Another way:

s = str.gsub(/[^\{\}\[\]\(\)]/, '')
while s.gsub!(/\{\}|\[\]|\(\)/, ''); end
s.empty?

Ex 1
str = "(a ()bb [cc{cb (vv) x} c ]ss) "
s = str.gsub(/[^\{\}\[\]\(\)]/, '') #=> "(()[{()}])"
while s.gsub!(/\{\}|\[\]|\(\)/, '') do; end
  s => "([{}])" => "([])" => "()" => "" gsub!() => nil
s.empty? #=> true

Ex 2
str = "(a ()bb [cc{cb (vv) x] c }ss) "
s = str.gsub(/[^\{\}\[\]\(\)]/, '')  #=> "(()[{()]})"
while s.gsub!(/\{\}|\[\]|\(\)/, '') do; end
  s => "([{]})" gsub!() => nil 
s.empty? #=> false
Cary Swoveland
  • 106,649
  • 6
  • 63
  • 100
0

I was given this as part of a simulated interview coding challenge. In my case, there was also a parens map passed in { "(" => ")", "[" => "]" }, meaning types of parentheses could vary.

def balanced_parens(string, parens_map)
  # where we throw opening parens
  opening_parens = []
  i = 0
  while i < string.length
    # if current index is opening paren add to array
    if parens_map.keys.include? string[i]
      opening_parens << string[i]
    # if current index is closing paren, remove last item from opening_array 
    elsif parens_map.values.include? string[i]
      popped_paren = opening_parens.pop
      # checking that closing parens at current index is a match for last open parens in opening_array
      return false if string[i] != parens_map[popped_paren]
    end
    i += 1
  end
  # if opening_parens array is empty, all parens have been matched (&& value = true)
  opening_parens.empty?
end
DNestoff
  • 124
  • 5
0
def valid_string?(exp)
  return false if exp.size % 2 != 0
  curly = "{}"
  square = "[]"
  parenthesis = "()"
  emptystr = ""
  loop do 
   old_exp = exp
   exp = exp.sub(curly, emptystr)
   break if exp == emptystr
   exp = exp.sub(square, emptystr)
   break if exp == emptystr
   exp = exp.sub(parenthesis, emptystr)
   break if exp == emptystr || exp == old_exp
 end
 exp == emptystr
end

user2129856
  • 31
  • 1
  • 6