54
Your inner loop appears to be unnecessary. Why not create a lookup:
var knownAccountNumbers = new HashSet<string>(
dataStructure.Tables["AccountData"].Rows
.Cast<DataRow>()
.Select(row => row[0].ToString()));
Now your loop is simply:
foreach (DataRow row in dt.Rows)
{
var accountNumber = row[0].ToString().Replace("\""...
20
You shouldn't be sharing Connection instances, because ADO is already doing the job for you, it uses object pooling behind the scenes. Sharing the Connection object will make it very hard to clean up your resources after you are done with it.
Use interfaces instead of concrete implementation.Use IDbConnection instead of MySQLConnection
You should use ...
17
Strains() {
System.out.println("Select a Strain.");
System.out.println("1. Blue Dream");
System.out.println("2. Sour Diesel");
System.out.println("3. OG Kush");
System.out.println("4. Girl Scout Cookies");
System.out.println("5. Green Crack");
System.out.println("6. Pineapple Express");
System.out.println("7. GrandDaddy ...
14
Note: I don't think this is what you want to hear. But I have to write "the truth". This is my personal opinions about your code.
Give me parameterized SQL or give me death!
tell me if there are any bugs or improvements to be made.
Don't suggest prepared statements.
I'm sorry, but those two requests really don't fit together in the same Code Review ...
14
The code appears to be broken, you're not break;ing after finding a match so all records will likely have row[12] == "N".
You should really be doing a join on accountNumber:
var matchingRows =
from DataRow row in dt.Rows
let rowKey = row[0].ToString().Replace("\"", "")
join DataRow queryRow in dataStructure.Tables["AccountData"].Rows
on ...
13
for (int i = 0; i < type.length; i++) {
if (ch == 7) {
System.out.println("What type of cannabis? " + type[6]);
System.out.println();
break;
}
if (i == ch) {
System.out.println("What type of cannabis? " + type[i - 1]);
System.out.println();
}
}
This loop is completely confusing. And as it turns out,...
12
I see a number of things that may help you improve your code.
Eliminate function prototypes by ordering
Since you put the acadd and menu implementations above main in the source code, you don't need the function prototypes. Generally, I try to order functions from smallest and simplest to most complex so that one can read from the top to bottom. Your ...
11
Security
This code is not secure.
SQL Injection
You are trusting the user input completely (except in the login function where you do use mysqli_real_escape_string, which is not secure enough). Use prepared statements instead. You should also read up on SQL injections in general.
Other
This is all minor in comparison to the SQL injection.
don't ...
11
Reduce duplicated logic by using helper methods
Instead of repeating this condition:
if (file.getName().endsWith(mime)) {
Use a helper method:
private static boolean isValidFile(File file) {
return file.getName().endsWith(mime);
}
The advantage is that this hides the implementation details of what is a valid file to you. Without this, all you ...
11
Just commenting on ... the comments in the source (sorry): I think they add a lot of noise, and don't provide any improved clarity (in this case). They're mostly restating what the following line of code obviously does, or repeating what a variable name already describes.
Such comments "by habbit" are actually harmful, as they have no benefit and could ...
11
The most terse and declarative way to write this is to use C#'s ?? operator. Your line becomes:
new SqlParameter("@Price", (object)items.Price ?? DbNull.Value),
The expression here will either return the non-null value of the left operand (items.Price), or return the operand to the right instead.
If you hadn't guessed, the ?? operator is basically ...
10
You do not need IDisposable in your current implementation as you do not use any unmanaged resources or have any fields or properties which implement IDisposable which your class owns.
Setting the properties to null in the dispose method is unnecessary.
Method names should be Pascal cased and not contain underscores to adhere to standard naming conventions:...
10
Let's start with your priorities: optimization. You should be primarily optimizing for the most expensive and limited resource, which is almost always developer brain cycles. You optimize for that resource by writing clean, readable, well-organized code with few surprises. Your code can be improved on all of these fronts.
As far as CPU or hard-drive ...
10
It looks like passwords are stored in plain text form in the pswd.txt file. Never ever store passwords in plain text form. Store passwords salted and cryptographically hashed. That way, if an attacker gains access to the file, he still has to crack the password, which can be extremely difficult if the password is strong enough.
To verify a password, apply ...
10
Reflection is slow. That line probably doesn't help your performance :
var model = (T)Activator.CreateInstance(typeof(T));
I notice that you always create your instance with parameterless constructors. So you could add the type constraint new() in your class. That means you could do :
var model = new T();
That'd be faster and well... clearer.
You also ...
10
All you have to do is to use the ?: (ternary operator):
new SqlParameter("@Price", items.Price == null ? DBNull.Value : (object)items.Price)
Alternatively you could write an extension for it:
public static object PriceOrDefault(this ItemsType items)
{
return items.Price == null ? DBNull.Value : (object)items.Price;
}
and use it instead:
new ...
9
Self-documenting code
char h1 = { (e.hour / 10) + '0' }; // Extract the first digit and convert to char (if any, else 0)
As you considered the division by ten and the 0 too enigmatic you added a comment, this is much better than leaving the reader wondering so good job.
The even better option though is stating your intention in code:
char h1 = ...
8
You have a static dictionary as backing store which means that all cache instances share it. Not saying that this is wrong and it might be intended but you should be aware of it.
Your cache apparently is not thread-safe. You need to protect access to the dictionary with locks or if you are using .NET 4 or later use ConcurrentDictionary. If you make the ...
8
First, I don't like the concept of mixing "Models" with "Repository".
One principle that, from my point of view, should be followed, is the Single Responsibility (the S in SOLID). It means that one class should have one responsibility, and only one. Therefore yours would be against this principle, simply because you are holding your data, and loading data ...
8
Nitpicks
There is a slight amount of inconsistency in your capitalization of keywords. SUM() / sum() and WHERE / where. Otherwise, it's pretty consistent throughout!
Aliases
You use the following aliases: B C T. There is no meaning or explanation of what those mean. You should use meaningful aliases, especially where your table names don't really help, ...
8
I can understand why you want to remove the clutter from your code - but sharing the instance of MySqlConnection isn't a good idea. As soon as multiple threads are trying to use the connection at the same time, things are going to go wrong.
The connections are already pooled for you (See here) so you creating and disposing IDbConnections is cheap and easy.
...
8
In your function pick_password, you should close the file at the end of the file because this frees up any system resources that were being devoted to working with that file.
For some reason, at the end of that function, you commented out the line
f.close()
You should un-comment this.
In python, it is good practice to open a file using the with keyword....
8
Everything is a String
Welcome to Java and object-oriented programming! You are doing something really strange in Java in your Effects() method (which really should be called effects() or getEffects()):
String BD = "Happy: 10 " + "Euphoric: 9 " + "Releaxed: 8.5 " + "Uplifted: 8 " + "Creative: 6.5 ";
String BDM = "Stress: 10 " + "Depression: 7 " + "Pain: 6....
8
There are some things to improve in this code. Especially when it's underlined that "if it can be maintained later on".
SQL query
The columns where the data will be inserted should always be defined, for example: INSERT INTO TASK(id_task, title, creation_time... etc) VALUES(... . It's the same problem as for SELECT *, that should never be used in serious ...
8
Also said before, but I think it worth mentioning again: there is not need for prefix_ for all columns. As far as I know, Hungarian and Leszynski notations are not exactly recommended. Since column names are almost always used in conjunction with their table, the table alias will make the reader known the table for a column.
It's a matter of taste, but ...
8
Your code will work, of course, but you go to a lot of trouble to grind the result set into an array when that capability is already available. Mysqli would provide the fetch_all() function if you had the mysqlnd drivers available; fortunately, PDO has provided it since PHP 5.1.
You will need to create a PDO connection with
$dsn = "mysql:host=" . $hostname ...
8
For the table definitions, just use an INT for the keys. You probably aren't saving anything by using SMALLINTs. Schema changes tend to be a pain in larger projects, so you might as well try to avoid any possibility of trouble.
CREATE UNIQUE INDEX index_PK ON owner_property(property_id, owner_id); is redundant, since the table already has PRIMARY KEY ...
8
A few comments:
Avoid memory intensive data types
Instead of using a DataTable for dt, just read from the csv directly, one line at a time (readLine() and then split(','). This will greatly cut down on your memory usage, rather than loading all 1.7 million rows at once, when you only use one at a time.
Sorted data is faster
Sort dataStructure by account ...
8
What happens if the user types "s"? Your program says:
Enter Buyer Name:
Oops! It was supposed to be uppercase, wasn't it? I'm not sure why you want it to be case-sensitive, but you should tell the user of his mistake instead of just going on as if you know what he meant. That means, I would change else: to elif not file: and add an else: that prints ...
Only top voted, non community-wiki answers of a minimum length are eligible
