New answers tagged

1

Welcome to Code Review! Bad naming The method doesn't actually check that a URL is valid, as it allows values that are not URLs. Thus it should not be named as "isValidURL". It should be isValidAddress or similar, but not URL. URLs have a very well defined syntax. Unnecessary variables There is no need to store the return value of specialCharactersExists(...


2

I'll show an example implementation first, and then describe it: from typing import Iterable import re def case_rank1_naming(proteins_available: Iterable[str], best_match_protein_name: str) -> str: # extract the three-letter pattern protein_pattern = re.search(r"[A-Z][a-z]{2}", best_match_protein_name).group() # extract the numbers ...


4

Simplify the code The guard statement and the following if let can be combined into a single statement: let key = "Signature" let signatures = iconURL.matches(for: "\(key)[^&]+") if let firstSignature = signatures.first, let range = firstSignature.range(of: "\(key)=") { let signature = String(firstSignature[range.upperBound...]) print(...


1

There is only one thing that jumps out at me as whacky... [a-z\d\-_\W] Regex101 breakdown I think this means to match a lowercase substring, but that's not what it is doing. Since \W is the inverse of \w and because \w represents A-Za-z0-9_, I think it is strange that the subpattern is used to replace the lowercase placeholder. As is, your pattern can ...


0

Call your sort method natural_sort -- or even better natsorted to comply with Python's sorted. The reason is that among programmers it is widely known under that name, so I recommend sticking with it. Jeff Atwood writes at the linked article: It isn't called "Alphabetical sort"; it's collectively known as natural sort. Furthermore, rather use existing ...


3

Style All the other answers are quite good and you should definitely follow their recommendations. They do however gloss over a stylistic point I find particularly striking about your code: blank lines. The way you use them makes them almost obsolete. There would not be a major difference in the readability of your code if you left them out, since there ...


4

I'd suggest a couple minor stylistic tweaks: Keyword arguments don't typically have spaces around the =, so we'd have sorted(l, key=alphanum_key) instead of sorted(l, key = alphanum_key) You have a couple fairly long lines that could be broken up or reduce a little Don't create a variable in one line if you're just going to return it in the next: just ...


5

Docstrings You should include a docstring at the beginning of every function, class, and module you write. This will allow documentation to identify what your code is supposed to do. This also helps other readers understand how your code works. I see that you already have a couple for your functions, but stay consistent. Parameter Names Parameter names ...


4

Not looking bad for as far as I can see. If the example file is accurate for the lengths of the input files, then I don't forsee any real problems, though others may of course disagree. Naming: cmdline is quite a... short name for a function. I'd think cmd_line for snake_case convention. However, what it does is create what's basically a text file in a ...


2

Naming: According to PEP8, you should make all these names snake_case. This is good, but the important bits here are: Do the same thing consistently Have a clear word seperator. Basically, this means you can use snake_case, camelCase, or even TitleCase, but the last one is generally used only for classes. I'm personally partial to camelCase for functions/...


3

Naming I have two problems with your naming: First, you don't use underscores to separate words. Instead of uniquepattern, use unique_pattern. Second, your names don't actually tell me anything! Functions, particularly non-method functions, should include a verb unless the verb is implicitly get or is. And implicit is needs to be used with some caution. ...


5

Your concept of "string" seems incomplete. I would look to firm up that definition and precisely match the need. Is "" a string? Do you want "valid identifiers" (/^[^\d\W]\w+$/) or "plausible ASCII words" (/^[A-Za-z]+$/) or just "not numbers" (!/ ^ ( [+-]? \d* \.? \d+ (?:[Ee][+-]?\d+)? ) $/x)? I like use warnings FATAL => 'all'; so that I don't ...


2

overall looks like a great job. Still, I wonder if you could save some CPU cycles with replacing Regex in favour of string methods var subExpressions = input.Split(' ', StringSplitOptions.RemoveEmptyEntries) .Zip(SubExpressionFactories, (field, factory) => new { field, factory }) .Select(x => x.factory(x....


3

(self-answer) I'll post another question when I made some more signifficant changes and for now I'll just summarize your feedback: Suggestions by @VisualMelon ✔ - no public tuples (but one small extension) (you need to forgive me) ✔ - I must use the \G anchor more often; this simplfied the Regex matching ✔ - no more Making the compiler happy - removed ...


2

A couple of tiny tidbits: You could easily make Token immutable (removing the property setters) by passing type and index into the constructor as such: public Token((bool Success, string Token, int Length) match, TToken type, int index) { (bool success, string token, int length) = match; this.Length = success ? length : 0; this.Text = success ? ...


3

Unnecessary switch-statements switch statements are nice as a way of avoiding long chains of if (){} else if(){} .... else {} statements. Switching on a bool doesn't make much sense, as is much more unclear than using if statements. So replace this switch (Escapables.Contains(c)) { case true: // Remove escape char. token.Length--; ...


4

General thoughts You have managed to create a somewhat elegant API that balances between a state machine pattern and a regex engine. This is reusable for small and context-free use cases, but will get to haunt you if you need to tokenize more complex and context-bound grammars. I can only add to VisualMelon's spot-on review: Tuples are fantastic ...


8

MatchDelegate Much as I love .NET's nominal delegates, I almost always regret using a delegate rather than an interface, so I would introduced an IMatcher (which MatcherAttribute can implement directly) in its place. Granted delegates usually go wrong because I need to serialise them, which won't be an issue here, but the ability to attach meta data could ...


2

It is simply not feasible to just parse the full set of (X|HT)ML with regex. I can't provide too much feedback on your solution as it simply isn't the correct solution for the problem, but I can provide plenty of examples where it fails to match valid input. <tag attr='"'></tag> <tag attr=""></tag> <tag attr=attr></tag> - ...


4

Maintainability No matter the possibilities of a regex, it's not intended for parsing complex languages. Nobody (no sane person) is able to make changes to this regex (even if provided with comments): ^((?:(?:(?:[^"'/\n]|/(?!/))*)(?("(?=(?:\\\\|\\"|[^"])*"))(?:"(?:\\\\|\\"|[^"])*")|(?('(?=(?:\\\\|\\'|[^'])*'))(?:'(?:\\\\|\\'|[^'])*')|(?(/)|.))))*) In ...


4

I suggest you get a linter such as Prospector or flake8, this can tell if your code is un-Pythonic. Some people prefer hinters like black. Your code doesn't conform to PEP 8 which is the standard when it comes to Python. Your comprehension is hard to read because it doesn't conform to best practices. I'd recommend moving your code into a main function and ...


Top 50 recent answers are included