60

The second sample is definitely better. I would be inclined to continue to improve the code by separating mechanisms from policies. The policy is the code that actually expresses the meaning of the program; the mechanism is the code that expresses what specific operations implement the policy. This idea comes from security design; you don't want the code ...


53

Dislcaimer: I'm not a security researcher and the following answer is compiled from my own, humble knowledge. The math is very basic and there are many things to consider, if in doubt, pay Security a visit. Also there are many factors that can kill password security completely, for example the user themselves or social engineering. In this case I'm not ...


50

If you want performance, don't create objects in the inner loop: static string generate(int len) { Random rnd = new Random(); // creating a new object string alpha = "abcdefghijklmnopqrstuvwxyz"; int letterInt; StringBuilder sb = new StringBuilder(); // creating a new object .... } ...


49

Concept Obligatory XKCD comic, before I begin: Enforcing password strength by requiring human-unfriendly characters is no longer considered good practice. Nevertheless, I'll review the code as you have written it. "Obvious" simplifications Any code with the pattern if bool_expr: return True; else: return False should be written simply as return ...


39

The biggest problem I see here is that you are printing a warning message to System.out and then using a default value for something which sounds like an Exception. Why allow negative values at all? if (initialAge < 0) { throw new IllegalArgumentException("Initial age cannot be negative, was specified as " + initialAge); } age = initialAge; This ...


35

I remember having read somewhere (possibly in another Code Review answer) that for an e-mail address, the simplest and most effective validation you can do is to make sure it contains an @. Making it more restrictive than that can often be a risk of invalidating some valid e-mails. You'd be surprised at some examples of valid e-mail addresses. As an ...


32

"@"@example.com and "\ "@example.com both fail, but they are valid. " "@example.com passes, but it is, in fact, invalid.* You probably missed the idea to confirm your knowledge with the relevant RFCs, as a conforming implementation should abide by the rules described therein. While Wikipedia is quite reliable nowadays, it is by ...


31

Python has a philosophy of "Batteries Included". Don't reinvent what's in the standard library! In particular, this code is much more easily written if we take advantage of IPv6Address: import ipaddress address = input('Please enter an IP address: ') try: addr = ipaddress.IPv6Address(address) except ipaddress.AddressValueError: print(address, '...


28

Buggy behavior First you're checking the length of the passwords, and then if they are null: if(pass1.length() < 1 || pass2.length() < 1 )retVal.append("Empty fields <br>"); if (pass1 != null && pass2 != null) { This is not going to work well: if any of the passwords were null, you would get a NullPointerException when you check ...


27

You have waaaay too many sums you are checking. KISS-Principle --> Keep it Simple {and} Stupid: private boolean checkSudokuStatus(int[][] grid) { for (int i = 0; i < 9; i++) { int[] row = new int[9]; int[] square = new int[9]; int[] column = grid[i].clone(); for (int j = 0; j < 9; j ++) { row[j] = grid[...


27

You can change valid_characters to a string. 'ABCDEFabcdef:0123456789'. You can use for element in elements: rather than: for i in range(len(elements)): element = elements[i] You can use in rather than manually perform the check. if current in valid_characters: is_valid.append(1) You can use a list comprehension, rather than manually build a list....


24

Your code is begging for the use of the Chain Of Responsibility patern (or rather, a variation of it). But first, some observations: You dereference the passwords before you check them for null: public String validateNewPass(String pass1, String pass2){ if(pass1.length() < 1 || pass2.length() < 1 )retVal.append("Empty fields <br>"); ...


24

Said this in chat already, but @ succeeds even although it is not a valid email address. You should require at least 1 character in the local part and 1 character in the domain.


24

I personally find it hard to fault your code. Actually I'm quite surprised about the absence of code. Other than a few PEP8 errors there are three changes that I would recommend. You remove both \\ and \" from your quotes, but you do it in an overly verbose way: stripped = quote.replace('\\\\', '').replace('\\"', '"').strip('".') Instead you can use re....


24

Good work! While there is certainly room for improvement, it is good that you stuck it out and discovered a solution on your own. def monthcheck(month): if month > 0 and month <= 12: ## If month is between 1 and 12, return True. return True else: return False Can be more succinctly written as def monthcheck(month): return ...


23

A simple rule I (and other) try to apply is : define things in the smallest possible scope. For instance, int UserNumber = 0; can be moved inside the loop. Similarly, k can be moved once you've made your loop a for loop : for(int k = 1; k < 11; k++). Then, computer people love counting from 0. If you write : for(int k = 0; k < 10; k++), I am used to ...


22

You have a common bug that will send you into an infinite loop. :-) When reading data (especially user input data) you must validate the read worked correctly. If the read fails and sets one of the error flags on the stream than any subsequent attempt to read will silently be ignored. Thus usually read operations are checked in the loop test. std::string ...


20

I have an answer which also changes the logic of your code but I think there is a good reason why you should consider it: function passtest($pass) { $errors = array(); if (empty($pass)) $errors[] = 'Password field is empty'; if (ctype_alnum($pass)) $errors[] = 'Password has special character'; [...] return '<br />' . implode('<...


20

I don't know python so I'll focus on the c# code. Your program is coded upside down. One would expect void Main at the top, with the more specialized code below. Method names in C# are expected to consistently follow PascalCasing convention. Only your Main method does that, and if you were to adopt a camelCasing convention, userinput would be better off ...


20

To answer the question of why your c# code is so slow. It's this line. According to a profiler ~90% of the execution time is being taken up by recreating the Random object. You'll notice your python code only uses the random rather than recreates it. Random rnd = new Random(); if you change the generate method to: static Random rnd = new Random(); ...


20

As @Simon pointed out, your regular expression might consider some valid addresses as invalid. Per this source which I've found somewhere on this Stack Overflow page, this would be RFC5322-compliant: (?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)* | "(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f] | \\[\x01-\x09\...


20

String Formatting if (!requireValue) Console.Write(string.Format(message + " [{0}]: ", defaultValue)); else Console.Write(message + ": "); This innocent code will break if message contains any String.Format() formatting code. It may be acceptable if it's a private function but to make it reusable you have to address this issue, you have to treat ...


20

For a beginner, that's very good code. There is one thing that almost all C++ programmers get wrong, and that's calling ::tolower with a char argument. This leads to undefined behavior as soon as you enter German umlauts or café. Your program may appear to work, but it is not guaranteed to do so. Any good tutorial on the tolower function (which requires #...


19

I find that negatives in method names lead to confusing code. For instance, if (!ArePerPersonBILimitsInvalid(...)) "Are there no BI limits that are invalid?" becomes much more readable when we write it this way: if (ArePerPersonBILimitsValid(...)) "Are all BI limits valid?" So let's fill in the blanks. public static bool ArePerPersonBILimitsValid(...


19

In the cctype header, you have the std::isdigit(int) function. You can use this instead of your conditions that check if the character is between '0' and '9'. You index into a std::string with an int inside the for loop. Use std::string::size_type as that is the proper type that can index into a std::string (int might be too small). Mark the function as ...


18

I'd rewrite it with three main changes: Keep main() minimal and improve the user interface. By the Single Responsibility Principle, it's a good idea to limit main() to just calling the primary function with the appropriate parameters. In this case, the functionality splits very cleanly. Being a Unix/Linux user, I would prefer to see tools that adhere to ...


17

The short answer is: Yes, there are ways to improve it! Coding conventions First of all, you are not following some Java coding conventions of indentation and where to put }-characters and similar. Error messages "Please try again" is not a message that really describes what went wrong. If I would see that message my first reaction would be to try to ...


17

I disagree with rolfl's assertion that, "There is no practical way to validate an e-mail address by regex alone." He is correct—as illustrated by the somewhat infamous SO answer he linked to—that it's impractical to validate any RFC-5322-compliant email addresses because, to quote the HTML5 spec: RFC-5322 … defines a syntax for e-mail addresses that is ...


17

First, heed what Jeroen Vannevel said; it will save you much grief. Of all the assumptions you've made here, only one is correct: there has to be an @ in the email address. Otherwise: it can have multiple dots, it can have no dots, it can have no TLD, it can have no domain, it can have IP addresses as domain, it can have quotes, it can have ...


17

You should only use == if you work with elementary data types (boolean, byte, char, short, int, long, float, long). If you use == on instances of classes like Integer you check if it is the same instance of the class not if the value is the same. You hardly ever want to check if two references refer to the same instance. That is especially true for the ...


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