Tag Info

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

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

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

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

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

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

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

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

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

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

Using Val The Val function can run into problems when converting string values, depending upon the locale of the user. It is generally safer to use CDbl, but it will depend on the input. See Internationalization Tips Indenting/Line spacing Your compiler directives follow on the immediate line after the function signatures, which looks a little cluttered,...

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

Adding the final version of the code here for anyone interested. Option Explicit ' *************************************************************** ' Class: Form_ProgressBar ' Popup progress bar ' ' Author: Christopher J. McClellan ' http://christopherjmcclellan.wordpress.com/ ' Significant input from Mat's Mug ' http://codereview.stackexchange....

3

Disclaimer: I don't know much about DAO. I typically use ADODB. The expensive part of this code is opening and closing the recordset repeatedly. So, instead of opening and closing it every time this is triggered, open it once when the form is first loaded. Private dbTemp As DAO.Something 'I don't know the right type Private rsTemp As DAO.Recordset Private ...

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

3

This should do it: SELECT U1.Unit, U1.[Date], U1.Mileage FROM tbl_unit U1 WHERE U1.id IN ( SELECT MAX(U2.id) FROM tbl_unit U2 GROUP BY U2.Unit ); This uses a simple subquery correlated on the tbl_Unit.Unit. This approximates your GROUP BY and LAST() functions in Access by SELECTing the most recent row for each Unit. LAST() in Access returns ...

3

Yes, I'd say your approach looks fine. If you want to see how someone else would do it, I've rewritten the code below. The differences are just style preferences, but sometimes it's nice to see how other people write the same code. I put all my variables at the top. If your code is so long that you can't see your variables, you probably need to split into ...

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