# Tag Info

18

You should take a look at the possibility to name groups in regex patterns; your match pattern can then be a oneliner: const string pattern = @"^(?<name>\w+)$(?<index>\d+)$$"; Match match = Regex.Match(command[2], pattern); if (match.Success) { string featureName = match.Groups["name"].Value; int index = int.Parse(match.... 15 Not duplicating any of @Peilonrayz's code review points ... Stop reading entire files into memory when you can process the file line by line in one pass, and stop creating huge lists in memory which are then iterated over exactly once. Both of these things creates a huge unnecessary memory pressure which can be avoided by looping and/or using generator ... 12 I think ask_file_name looks fine without using pathlib. The difference between the two comes down to LBYL vs EAFP. For the most part the difference between the two is style. Do you prefer using except FileNotFoundError or an if path.exists(). What I do find strange is ask_file_name follows a LBYL approach, but the code inside it follows an EAFP approach. ... 12 One possible place you can improve performance is here: const string pattern = @"$(.*?)$"; var query = command[2]; var matches = Regex.Matches(query, pattern); //Gets anything inside the brackets index = Convert.ToInt32(matches[0].Groups[1].Value); //should be an int featureName = command[2].Substring(0, command[2].IndexOf('[')).ToUpper(); You're ... 11 For starter, congratulations, the code is clean and uses rather good constructs. I just have two nitpicks about the layout: I prefer to indent lists before the first row: patterns = [ # 0) 1-12-1963 r'(\d{1,2})-(\d{1,2})-(\d{4})$', # 1) 1789-7-14 r'(\d{4})-(\d{1,2})-(\d{1,2})$', # 2) '1945-2' r'(\d{4})-(\d{1,... 11 import re regex = re.compile( r'(The|A) ' r'\w+' r'( is )' r'\w+' r'(?:' r'( and I )' r'\w+' r'( it)' r')?' ) def repl(sentence, subject, colour, verb=None): m = regex.match(sentence) new = m.expand(rf'\1 {subject}\2{colour}') if m[3]: new += m.expand(rf'\3{verb}\4') return new def ... 11 public static class TimeSpanConverter { public static TimeSpan Convert(string input) To people who use WPF a lot, the name hints at this class being a TypeConverter or an IValueConverter. I would say that it's really a parser. Similarly, I think that the method should be called Parse. var units = new Dictionary<string, int>() ... 10 I would follow @Mathias answer for the Pythonic comments, however... I think my best bet is not to convert to type datetime, but to try to match strings. I disagree, Don't Reinvent the wheel. Instead of manually cleaning these dates, there are some really good libraries that can do this for you. Check out the python-dateutil library. To avoid the ... 10 Code length is one factor in readability, but it isn't the only factor. "Is this code easy to read and modify" is a more important question to ask. It looks like you've gotten a method skeleton here, which kind of leads you down the path of stuffing everything in one function, which is a bit of a shame. Let's look at what we actually need here. Length of ... 10 According to PEP 8, the official Python style guide, function names should be lower_case_with_underscores. Furthermore, this function parses the URL, rather than creating a parser object, so the function name should be a verb phrase rather than a noun phrase. By RFC 1738, the scheme and host portions of URLs are case-insensitive. Also, it is allowable to ... 10 Bug There is a case that requires NFA instead of simple DFA to recognize string (of course it is possible to convert NFA to DFA): eval("aaab", "a*ab"); Gives false, even though the string matches the regex. Disallowing same character after * fixes the problem too. Style why-is-using-namespace-std-considered-bad-practice. Pass by const reference for ... 10 I like your solution quite a bit. It's clear, easy to read and I don't see any bugs. There are many ways to condense the replace calls as you mention, but I think you're at a point where such changes can easily have a disproportionate impact on readability. That's good--it means the code is already pretty optimal from that standpoint. For example, here's ... 8 Good job on the easily understandable code. Good Good functions, with clear names! Modular approach Unittests Docstrings Improvements Regex with lots of backtracking can produce some major performance loss Consider that this re.search(r'.*[A-Z]+.*', string) is equal to re.search(r'[A-Z]+', string) or even re.search(r'[A-Z]', string) as Toby ... 8 You could save one .replace() call by replacing the last two with: .replace(/(^| ) +/g, "$1") which both removes leading spaces and collapses multiple consecutive spaces to one anywhere else in the string. However, I'm not 100% sure that you should, since it's not really clear which way is more efficient in practice, and your way seems more readable ...

7

If you want to recurse also into sub-folders, you should use os.walk: import os import shutil def get_files_recursively(start_directory, filter_extension=None): for root, _, files in os.walk(start_directory): for file in files: if filter_extension is None or file.lower().endswith(filter_extension): yield os.path.join(...

7

For a first pass, not bad! Your code is pretty easy to follow. Problems: Don't use [] to match different strings. [] matches any set of characters, so [imgur|i.imgur]* will match , g, mgi, etc. You probably wanted a non-capturing group, which is specified with (?: ...), re Docs Name functions with snake_case, as recommended by PEP 8. The challenge as ...

7

You usually want a zero-width word boundary \b on either end of the regex, to avoid matching things like 1a:1a. There's no need to capture [a-z]+ since you are throwing it away. You can use lookbehind (?<=…) assertions to require a match to be preceded by whatever, without including the whatever in the match result. This means no need for capturing ...

7

fileExt="*.js" allFiles=$(find ./ -name$fileExt) This is a bug. The wildcard in $fileExt will be expanded by the shell, and cause a syntax error when the current directory has more than one matching file in it:$ touch a.js b.js $fileExt="*.js"$ find ./ -name $fileExt find: paths must precede expression: `b.js' find: possible unquoted pattern after ... 7 If you call regex_strip(s, ""), you will get: re.error: unterminated character set at position 0 because neither ^[] nor []$ is a valid regular expression. You could avoid this by using if not chars: instead of if chars == None:. There is no need to re.compile() your regular expressions; you aren't saving the compiled patterns anywhere for re-use. You ...

7

Why would you want to use Regular Expressions anyway if the base format of the string is always the same? Name[Number] seems like an easy pattern. Just iterate through the characters one by one and store all characters in the first string until you reach the first bracket. Then store the numbers in the second string (until you reach the closing bracket). ...

6

The patterns list is not doing you any good here: you end up doing tests like if pat == patterns[0] to see which regex was tested, so you may as well have written re.match(…) four times instead of looping. (By the way, use re.fullmatch() to avoid writing $in every regex.) Another misstep that makes your regexes ineffective is that they are not self-... 6 No. The matching for a-zA-Z would be slower than the exact character-set you supply: RNBQKrnbqk. You can observe this behaviour by checking the backtrace it generates. I compared 3 different patterns, two being your own, and the third I found on chess.stackexchange.com: (\d?)[a-z](?=(\d?)(.*))(?=(.* ){5}) has 64 matches generated in 14323 steps (\d?)[... 6 Just a couple of thougths... if (string.IsNullOrWhiteSpace(sitemapText)) yield break; This is a very unusual handling of null/empty argument values and should be documented. A more meaniningful name like GetSitemapUrlsOrDefault would also help. Without it the expected way is to throw an ArgumentException. foreach (Match m in urlRegex.Matches(sitemapText)... 6 Review Your regex can be simplified: There is no need for the multiple [] brackets - this would suffice: r"^(.*?)\.pdf$". Note that I escape the . char (which will match anything) with a backslash \. to only match the specific . char, and the $to be certain that .pdf is at the end of the string. There is no need for Regex at all! You can either use the ... 6 Two problems Your code has two problems. It does not remove punctuation from the words resulting in the same words not matching. Eg text1 has 12 unique words not 13. You count dolor and dolor. as different words. You are ignoring capitalization. You would count Dolor and dolor as different words rather than the same. String.match Update I was not ... 6 I like the simplicity of your function, it's pretty easy to understand the implementation. Couple things I would consider: A function "computeCountUniqueWords" does a very specific task that counts the number of unique words given a string. But depending on the context of this code(for example, where it's being used, maybe just as a utility library), I ... 6 Honestly there's not much code to comment on, it's also a programming challenge, so there's not much incentive to make it any better than it is. I too would have done it your way. The only objective criticism I have is I'd just add a function regex_options to build the non-capturing group. Other than that I'd apply this to the creation of g, o, l and e. As ... 6 DRY. Both branches do identical re.subs. Take them out: if chars is None: strip_left = re.compile(r'^\s*') strip_right = re.compile(r'\s*$') else: strip_left = re.compile(r'^[' + re.escape(chars) + r']*') strip_right = re.compile(r'[' + re.escape(chars) + r']*\$') s = re.sub(strip_left, "", s) s = re.sub(strip_right, "", s) return s I ...

5

1) Control flow clarity if a and b: ... elif a and not b: ... elif not a and b: ... elif not a and not b: ... can be improved to if a: if b: ... else: ... else: if b: ... else: ... 2) Remove unnecessary function calls ' '.join([a, b]) can be simplified to a + ' ' + b. In your case, since you're appending b, you can do a += ' ' + b. 3) ...

5

Repeating the match can be avoided by storing it in a variable first: match = re.search(r'^0[xX][0-9a-fA-F]+', line) if match: event_codes.add(match.group()) Note also that you don't need to strip a trailing \, because it is ignored afterwards anyways and you are only interested in hex codes at the beginnings of the line and all your multilines seem to ...

Only top voted, non community-wiki answers of a minimum length are eligible