10

Is this secure? Well, nothing is ever really secure, but yes. Generally speaking it is. You're using parameterized queries, which is good. Malachi is right though, stored procedures would be even better. What you can do to make this a little more secure is specify the datatype and size of each parameter. I'm not sure where this code resides, but you should ...


8

Make your code DRY (don't repeat yourself). Extract out your whole code of data retrieving to helper class CommandBehavior.CloseConnection will avoid closing the connection so you need to explicitly close the connection. so use using keyword for VB. it will close the connection automatically you are using parameterized query so it will make your SQL safer ...


8

You should not hard code queries into your VB code if you can help it. if you were using SQL Server I would say that you use Stored Procedures on the database instead of hard coded queries. by passing parameters to a Stored Procedure from your code and having the database send you information back you are allowing the database to take care of a lot of ...


8

Unless I misunderstand, you are using unfiltered user input from txtEntryID. Never trust user input. What if the user fills txtEntryID with 123 OR 1=1? The query will delete everything: DELETE FROM PersonalData WHERE DataID = 123 OR 1=1 What if, then, the user inputs 123; UPDATE Account SET credit = 1000 WHERE userId = me: DELETE FROM PersonalData WHERE ...


8

Start with proper indentation. There shouldn't be anything (other than subroutine/line labels) at the same level of indentation as a Private Sub statement, other than the End Sub. Code blocks are more readable and code as a whole is easier to follow when code blocks are indented. General rule of thumb, if there's an End X for a statement, you're in a code ...


7

I don't have lots of time, so I'll just make a couple of points: Drop the "Hungarian" notation altogether, it's not doing you any good. Instead declare variables close to their usage, and strive to keep procedures < 25 lines. If you have to stick to that notation, seriously reconsider the lowercase "L" prefix. There's practically zero difference between "...


6

Look at ANeves description of SQL injection. I'll just show a solution to the problem: First change the query to include a parameter placeholder, like this: string query = "DELETE FROM PersonalData WHERE DataID = ?"; Later when you're building your query, you add the parameter to the OleDbCommand object: var cmd = new OleDbCommand(query); cmd.Parameters....


6

Would it be cleaner to simply use a Public variable instead? You're not really asking that question. I'm not really telling you that a property is always going to be cleaner than exposing a public field. ...Well look at that, I just did! Point is, yes, any property could also be a public field. Heck, any public field might as well be a global variable ...


6

This is a huge improvement over the last version. I'll try to give you some pointers on how to improve this farther, but I am also going to tell you this. Your database is very poorly designed (if not downright broken) and nothing we do here will fix that. If you were using proper keys on your database, you might be able to develop a sql solution to this. ...


6

There's nothing to optimise here. Your steps are: Open connection Execute about the simplest SQL query possible End There's nothing we can change there that would make any difference. Now, if you want to post a separate question, detailing your Database Structure, your SQL query, and asking how to optimise it, we could help there. That aside, ...


6

Dim current As String = Convert.ToString(Date.Today) Dim year As String = Convert.ToString(Date.Today.AddYears(-1).Year) Dim nextyear As String = Convert.ToString(Date.Today.AddYears(15).Year) Dim period As String current = current.Substring(0, 5) period = nextyear.Substring(2, 2) It's not clear why you would want to work with String variables, given... ...


5

As you already noticed, all the CreateSomething methods have the same repeating pattern. We can extract this pattern and encapsulate it in a method for reuse : Private Sub CreateCommandBar(name As String, ParamArray buttonIDs() As Variant) Dim menu As Office.CommandBar Set menu = CommandBars.Add(name, msoBarPopup, False) For Each ID In ...


5

First and foremost, use Option Explicit in all of your code modules. It forces you to declare all of your variables. You have about 20 declarations at the top of your module, but haven't declared your recordsets at all. These are never used I didn't check the rest of them: Dim strCOMP_NAME As String Dim strCOMP_NAME1 As String Dim strCOMP_NAME2 As String ...


5

I don't know what this is called, so I haven't been able to Google a real implementation, so I've had to make my best guess at it. [...] If anyone can tell me what the bit flagging thing used in MsgBox is called, that would be really useful too. They're called Flag enums in .NET (see this SO question), and apparently the naming is also appropriate for VB6 ...


5

Making duplicate testing just to avoid a method call doesn't make much sense. Method calls are not that expensive. On the contrary, they can help make the code cleaner. You can make a method that helps you clean up the strings, just pass in how long each string may be. I think that maked the code as DRY as possible: private string TruncateString(string ...


5

I can't see any SQL vulnerabilities, but I see a few crypto mistakes. You're using the same password for the database login and the contents. You shouldn't reuse passwords, because it increases the risk of leakage. AES has two parameters: the key and the initialisation vector (IV). Even when you use the same key, you should really be using a different IV to ...


5

First off, if you're not using Python 3.x, you need to explicitly inherit from object like this: class MyClass(object). If you are using Python 3.x, then you're fine. If you're going to format your queries based on user input, then the following piece of code may have certain issues, like SQL injection. See this for more information. SQL = """ CREATE TABLE ...


5

There are a couple of micro-optimizations to be made. They probably won't give you the boost you're looking for, but it's worth trying. First, you can explicitly define the return type. When you don't define one, the compiler defines it as a Variant type, which incurs a lot of overheard compared to the "regular" data types. For clarity, this Public ...


5

Namespace scope and pollution is often a problem in programming, and when reading code, if there are two usages of a particular identifier, a reader may not understand exactly which one you mean, and the complier may have different scope rules than you expect, so it is worthwhile to not re-use names. In this particular case, consider using query instead of ...


5

Properties in Standard Modules Standard modules can have Properties, but as @Rubberduck pointed out, they're not necessarily intuitive (although they do force you to qualify a call with the module name, so there's that as a redeeming quality). You could write public methods instead, but you could also create a class, and if you wanted to avoid the need to ...


5

Case Is = "CheckBox" That syntax is very weird and unusual, surprising even. This would be more common, and simpler: Case "Recordset" TypeName makes a stringly-typed check, which is bug-prone. You say you're not going to use it with any ActiveX controls, but can you be sure no one in the future will ever try to do that? ADODB.Recordset isn't an ActiveX ...


5

Some basics Trivial and not really going to affect your current code, but consider setting DownloadStatuses as flaggable errors (instead of H1,H2,H3, ... use H1, H2, H4, ...). That way you can flag multiple errors at the one time. VBA has limitations on user-defined types (e.g. Private Type TDownloadStatus), particularly how they can be used in Collections....


4

Your code can easily be exploited, as ANeves showed. In your case a small fix can make it safe. By simply parsing the id as an integer, it's totally safe. An integer can not contain any harmful code: int id = Int32.Parse(txtEntryID.Text); string SQL = "DELETE FROM PersonalData WHERE DataID = " + id.ToString(); Concatenating the value into the SQL code is ...


4

Completely unrelated to encryption, I would like to provide feedback on your btnAddEntry_Click and InsertData functions. The biggest problem I see here is that your database intelligence is partially in this button's code, and the insertion function has several checks on the UI. What's a button? It's a control put in an interface that a user can activate to ...


4

First some general remarks about the code, and then an alternative way to go about this. Underscores have a special meaning in VB. They indicate event procedures and implementations of an interface. You should remove them from your namings. It's a bit confusing to look at. Particularly this. Private Sub Form_Current() mCustom_FormCurrent Me End Sub ...


4

Why don't you just put it in the query: Query for employees with invalid managers: Select [Employee_Name],[Employee_Status],[Employee_Manager] FROM employees WHERE [Employee_Status] = 'Active' AND isNull([Employee_Manager]) Query for employees with valid managers: Select [Employee_Name],[Employee_Status],[Employee_Manager] FROM employees WHERE NOT ([...


4

I'm going to leave the ms-access specifics to more proficient ms-access reviewers, as I'm not all that familiar with DAO and Access VBA. If possible, switch to SQL Server ADODB. DEADC0DE Unless I'm missing something, you're inlining the SQL statements and Dim strSQL As String is an unused declaration. These can be hard to spot without an add-in though. ...


4

Variables I like that you're using Option Explicit. It helps prevents "easy" mistakes from typos. I don't like that you declare all your variables in a block and I somewhat dislike the naming scheme you use: I also used to declare all variables in a block at the start of a procedure. It's significantly easier to declare them as close as possible to their ...


4

I'm going to cover other points that aren't covered... Unnecessary recordset movement You have this: If rs.RecordCount Then rs.MoveFirst rs.MoveLast rs.MoveFirst ... End If This is totally unnecessary and also problematic: RecordCount is not the most idiomatic way of checking for an empty. EOF is more common and is usually effective. It ...


3

I think you should change a couple of your loops so they look cleaner Do While Not RS_tmpTblDocsOutList.EOF ' loop through output docs should be a Do Until loop Do Until RS_tmpTblDocsOutList.EOF ' loop through output docs straight to the point, same with this loop as well Do While Not RS_CurrDocTbl.EOF 'loop through records (2nd) Should also be a Do ...


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