Hot answers tagged

21

I see a few, actually, a number of problems in this code. I am afraid this will be something of a scathing review, in part because you mention this is intended to be for a tutorial.... I see a number of call them 'critical' issues. Then also a number of lesser issues. The critical ones first: Use Case Why not use an Iterator? Since an Iterator would do ...


19

Although there's nothing wrong with your code, it is generally easier to fall back on framework classes for this kind of thing. Namely UriBuilder and HttpValueCollection. public static Uri BuildUri(string root, NameValueCollection query) { // omitted argument checking // sneaky way to get a HttpValueCollection (which is internal) var collection ...


16

You asked for comments on coding style, so here goes: It's Python style to use CapitalizedNames for classes and lower_case_names for functions, methods and variables. Functions should have docstrings that describe their arguments and what they do. Names starting with "get" are normally used for functions that return the value they get. Your function prints ...


16

You could use a ternary operator to inline the condition into a single return statement: private String checkTrailingSlash(String website) { return website.endsWith("/") ? website : website + "/"; }


14

Use UriBuilder and set Port to -1 Uri oldUri = new Uri("http://myhost:443/Home/Index"); UriBuilder builder = new UriBuilder(oldUri); builder.Port = -1; Uri newUri = builder.Uri; In case you want to remove the port part only when it is default, e.g. 80 for http and 443 for https, use snippet below (credit goes to Chris for the idea) static Uri ...


14

Your regex has many false positives (matches without a valid URL being there), and many false negativs (URLs you don't recognize). You should read the appropriate RFCs which contain relevant parts of the grammar. Here are some failure scenarios: ⊖ You don't match schemes other than HTTP or HTTPS. Consider FTP, SFTP, data URLs, and other widespread schemes ...


14

Here are some concerns/suggestions: you are reading the file twice - once to get the row count and when reading the links. And, you don't need to initialize the csv.reader to get the row count, simply use sum() over the lines in the file. You would probably need to use infile.seek(0) after getting the count and before initializing the csv reader use _ for ...


12

This is a mess: StringBuilder urlBuilder = new StringBuilder("http://"); urlBuilder.Append(IpAddress); urlBuilder.Append("/set.cmd?" + "user=" + Username + "+"); urlBuilder.Append("pass=" + Password + "+"); Why concatenate when there's an AppendFormat method? StringBuilder urlBuilder = new StringBuilder(1024); urlBuilder.AppendFormat("http://{0}/set.cmd?...


11

Ok, so the main problem with your code is that it is vulnerable to SQL injection you can fix that by using prepared statements - http://php.net/manual/en/mysqli.quickstart.prepared-statements.php If you want to remove the ?l= you can look into Mod Rewrite - http://www.sitepoint.com/guide-url-rewriting/ and have the URL generate as gus.netii.net/$hash I ...


10

It can be tough, but pretend you're not the developer who wrote this code and try to understand what it does by reading through it. At a quick glance some questions you might ask could be: What does isValid actually check the validity of? What is strInput? The line isValid(strInput) just tells me that someone has deemed some input string valid for doing ...


10

It appears that you're using a class-level variable (strErrorMSG) to retain your error information. That is a style of programming that I highly discourage, where values are essentially "passed" between methods inside of a class using class-level variables instead of actual parameters. The reason that I discourage this style of programming is that I have ...


10

Extract the chk parameter value from location.search and use a "lookup table" object: var CHK_ID_LOOKUP = { 'aiweb-check': '#ai-for-website', 'anivid-check': '#animated-videos', 'appdevuiuxexp-check': '#app-development', 'augreal-check': '#augmented-reality', 'datacol-check': '#database-collection', 'digbrand-check': ...


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 ...


9

Instead of splitting from the ?, you can use location.search. It returns everything from (and including) the ? until the end of the query string. If there's a hash, it stops right before #. Since it includes the ?, you can do location.search.slice(1) to get the string without the ?. Then you can do: var queries = location.search ....


8

how about: QueryBuilder.For("guide/search.aspx") .Query("i", item) .Query("f", found) .Query("wt", what) .Query("wr", where) .ToString(); public class QueryBuilder { public static QueryBuilder For(string page) { return new QueryBuilder(page); } private readonly StringBuilder sb; private QueryBuilder(string ...


8

Firstly, you should use rsplit and next rather than split and last, as it starts at the more appropriate end: fn basename<'a>(path: &'a str, sep: char) -> Cow<'a, str> { let pieces = path.rsplit(sep); match pieces.next() { Some(p) => p.into(), None => path.into(), } } Secondly, you shouldn’t be using ...


8

You are in dire need of a lookup table. What you want to do is define a mapping somewhere else, and then just percent-encode all characters you have mapped. This gets rid of that unwieldy, huge and btw. hacky switch-statement. consider the following: StringBuilder sb = new StringBuilder(toEncode.length()); for (Character c : toEncode.toCharArray()) { ...


7

First off, congrats on making your own, that's the best way to learn and usually you don't need all the added overhead that a prefab provides. In the implementation of MVC I like to use, there is a router between the view and controller, so its more of a MVRC rather than MVC. Just another level of abstraction, but I like it. It seems like what you are ...


7

The slowest part will be the request to the remote server. There is not much you can do to make this faster. You may want to cache the results you get back from hostip, or you may want to have a local database. Furthermore, the API seems to return XX if it does not know the country, you may want to check for that. Also, you do not close the InputStream or ...


7

I have witnessed the UriBuilder .port = -1 fix elsewhere. Frankly guys, it is a hack that really isn't a good production ready solution. The right solution here is to use UriBuilder as follows. var uri = new UriBuilder(baseUri); string newUrl = uri.Uri.ToString();


7

Using regex to match a URI is never a good solution... as you have discovered, you now have two problems (the base problem, and the regex problem). In this case, you should use the language features to help you.... (bear in mind I am a Java person, so I may have some syntax errors).... try { Uri uri = new Uri(stringvalue); // use your favourite C# ...


7

First of all, the `cmd` form of running commands and capturing their output is deprecated. Use the recommended, modern way everywhere: $(cmd) Safety Be careful with spaces in filenames. These commands will break: message=`cat $1` shorten=`sed -ne 's/.*\(http[^"]*\).*/\1/p' $1` It will be safer like this: message=$(cat "$1") shorten=$(sed -ne 's/.*\(http[...


7

Just a rant, someone else will do the real work, but for me it's hard to read. durl = url; When I see it..... what to say? So slowly... "url" is fine, everybody understands it. "durl" could be maybe "dural" with a typo. Or "dull". There's no reason not to use the same name for a member and constructor argument. The naming in general is just terrible: "...


7

Python's naming case is snake_case (makes sense, right?); you are writing your identifiers and functions is camelCase. For example, this: connectionParameters becomes: connection_parameters And, as another example, this: cleanUpDownloadFolder becomes this: cleanup_download_folder However, for classes, the naming case is PascalCase, which you did ...


7

It would be less verbose to write website += "/"; instead of website = website + "/"; So, yeah, you totally overcomplicated it. =) Also, instead of checkTrailingSlash, it would be clearer if renamed to ensureTrailingSlashExists.


7

Honestly, I'm not sure how much simpler you can make this. I'd suggest a slightly clearer name though - maybe ensureTrailingSlash? checkX sounds more like a predicate. My other suggestion would be to add some spaces in your if-conditional, depending on your taste: if (!website.endsWith("/")) { This could be a static method, as well.


7

Your complexity analyses look correct to me. I would just say, in short, that this function works in O(n2) time. Note that this is an awkward exercise to do in Python, or any language in which strings are immutable. Since you can't perform the substitutions in place, you have to return a copy, which means that the minimum space requirement is O(n). In ...


7

First of all, it would help tremendously if you named the URL parameter values and the form element IDs consistently. Why do you have chk=webdes-check corresponding to #website-design? How did you even arrive at the weird abbreviation "webdes"? And why do you need the redundant "-check" suffix? The element IDs make much more sense, I think. If you use ...


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 ...


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