Rules: You should create a function that receives a string with brackets, parentheses, or curly brackets, an example:
test('{A}')
The function must check that the opening and closing of the brackets are correct. If they are correct it should return true
, else it should return false
.
Examples:
'{A}' // true
'[A]' // true
'(AB)' // true
'({A})' // true
'((A))' // true
'[(A)]' // true
'[A}' // false
'(A]' // false
'({A])' // false
'([A))' // false
This is the code I did to resolve this problem:
const bracketList = ['{', '[', '('];
// This function return the reverse bracket. For example '[' returns ']';
const getReverseBracket = (bracket) => {
if (bracket == '{') return '}';
if (bracket == '[') return ']';
if (bracket == '(') return ')';
}
// This function return the initial bracket. For example if the string starts with '{', then it returns '{'.
// In case the string does not start with any bracket, then return null
const starterBracket = (string) => {
for (brak of bracketList) {
if (string[0] == brak) return brak
}
return null
}
// This function is called when any starter bracket has been detected.
// if the string start with '(', then it checks if the reverse bracket is in the end of the string.
// If this is true, then returns true, else returns false
const bracketOk = (string, bracket) => {
let reverseBracket = getReverseBracket(bracket);
if (string.slice(-1) == reverseBracket) return true
else return false;
}
// This function removes the brackets of the strings
const removeBrackets = (string, bracket) => {
let reverseBracket = getReverseBracket(bracket);
return string.replace(bracket, '').replace(reverseBracket, '');
}
// This function checks if the string contains any reverse or normal bracket. Depending of the result it return a number that means:
// ! 0: if the string contains reverse bracket but no Starter
// * 1: if the string contain starter bracket
// ? 2: if the string does not contain any bracket
const containAnyBracket = (string) => {
const rbracketList = ['}', ']', ')'];
for (brak of bracketList) {
if (string.includes(brak)) return 1
}
for (rbrak of rbracketList) {
if (string.includes(rbrak)) return 0
}
return 2;
}
// this function calls all other functions to find out if the string meets the requirements.
function testString(string) {
let bracket = starterBracket(string)
if (bracket) {
if (bracketOk(string, bracket)) {
let noBracketString = removeBrackets(string, bracket);
let containBracket = containAnyBracket(noBracketString);
if (containBracket == 2) return true;
else if (containBracket == 1) {
bracket = starterBracket(noBracketString);
if (bracketOk(noBracketString, bracket)) return true;
else return false;
}
else if (containBracket == 0) return false;
} else return false;
} else return false;
}
How would you improve the solution code?. I honestly think i could do it better. How you would solve this?
Do you have any advice for me about good practices?
1 Answer 1
Correctness
Your code falsely identifies (hi)there
as having unbalanced brackets.
Formatting
The else return false
clauses can be replaced with a single return false
at the end of the function. There is no need to treat the 0 as a special case. testString()
is not a very descriptive name. getReverseBracket()
could be unecessary if you restructured the bracket data. The magic numbers would better be named constants, but they are not necessary anyway.
Alternative Solution
A stack is all you need to solve this. If you see an opening bracket, add to the stack. If you see the wrong closing bracket, fail. If you see the correct closing bracket, pop the stack. When you get all the way through the string, there should be no brackets left to close.
const bracketPairs = { '[':']', '{':'}', '(':')' }
const closingBrackets = new Set(Object.values(bracketPairs))
function bracketsAreBalanced(text) {
const open = [] // stack of (closing) brackets that need to be closed
for (char of text) {
if (closingBrackets.has(char)) {
if (char === open[open.length-1]) open.pop()
else return false;
}
if (char in bracketPairs) open.push(bracketPairs[char])
}
return open.length === 0
}
console.log(bracketsAreBalanced('(hi)there'))
-
7\$\begingroup\$ This is the classic problem for introducing stacks, either explicitly (in an imperative language) or implicitly (using tail recursion in a functional language, keeping information on the call stack). If the solution doesn't use a stack, it is pretty much guaranteed to be more complex than necessary. (In the simpler case where there is only one type of brackets, you don't even need the stack, you only need its depth.) So, I would argue that this is not just an alternative solution but THE solution. \$\endgroup\$Jörg W Mittag– Jörg W Mittag2021年11月29日 11:05:50 +00:00Commented Nov 29, 2021 at 11:05
-
1\$\begingroup\$ Why
const open = []
? You intend to modify it, so wouldn'tvar
be more appropriate? \$\endgroup\$Andrakis– Andrakis2021年11月29日 11:55:35 +00:00Commented Nov 29, 2021 at 11:55 -
6\$\begingroup\$ @Andrakis
const
doesn't mean you can't modify the object referenced by the variable, but rather that the reference will not change (e.g.open = []
later would be illegal). If it were meant to be a read-only object, you'd useObject.freeze
for shallow modifications, or a read-only Proxy for deep modifications. \$\endgroup\$phyrfox– phyrfox2021年11月29日 12:57:43 +00:00Commented Nov 29, 2021 at 12:57 -
3\$\begingroup\$ The content of the array is modified, but the variable won't be assigned a new array,
const
is more appropriate. \$\endgroup\$Cédric– Cédric2021年11月29日 13:04:24 +00:00Commented Nov 29, 2021 at 13:04 -
\$\begingroup\$ Your code also has the issue that the original post has: global variables in the
for
loops and inconsistent use of semicolons. See my main comment on the post for more details \$\endgroup\$Samathingamajig– Samathingamajig2021年11月29日 16:47:39 +00:00Commented Nov 29, 2021 at 16:47
getReverseBracket
with a plain object that holds the opening bracket as the key, and the closing as the value, like this:const reverseBracket = {'(': ')'}
(of course, you'd need to add all the missing mappings) \$\endgroup\$for (name of list) {...}
, you're creating a global variable calledname
(name
is just a placeholder for your multiple times of doing this). Usefor (const name of list) {...}
. Another thing is you're inconsistent with your use of semicolons. In yourbracketOk
andcontainAnyBracket
functions, you don't use;
afterreturn
statements, but you do everywhere else. In yourgetReverseBracket
function, I would either make itelse if
for the second and thirdif
, or make it aswitch
statement. \$\endgroup\$