21
I would assume that this
For Each d As KeyValuePair(Of String, String) In dict
Using outputFile As New StreamWriter("E:\" & Convert.ToString("file.txt"), True)
outputFile.WriteLine(d.Key & "," & d.Value)
End Using
Next
is the bottleneck of your code. You are opening, writing to and closeing the ...
17
COM interop is in itself a performance hit, but even when accessing it through COM (e.g. in VBA), the Outlook type library / object model is notoriously, painfully slow. If performance is a requirement I would consider parsing the PST file without involving the Outlook object model, e.g. with something like PST-Parser (no affiliation, just an OSS project I ...
15
I'm not sure how to optimize your code, but let's talk about those variable names.
Naming is hard. It's doubly hard in vb.net, because it's case insensitive. That fact makes it difficult not to step on keywords like Field. I don't like recommending this, but without a being able to use fields and field to differentiate from the keywords Fields and Field, ...
15
Your throw-away "test code" doesn't need to Dim a variable and introduce YACI (Yet Another Cryptic Identifier) - this:
Dim sw As New Stopwatch
sw.Start()
Dim fd1 As New Field With {.dateTime = Date.Now, .sg = "23:59:00", .os = "00:03:00"}
Dim fd2 As New Field With {.dateTime = Date.Now, .sg = "00:01:00", .os = "00:03:00"}
Dim fd3 As New Field With {....
12
A classic solution to this problem is to use a data table - in your case probably in form of a dictionary. It depends of what type your values are - I assume they are some sort of view models? I'm also going to assume they have a common base class:
One time setup:
Dim dict As New Dictionary(Of String, ViewModelBase)
dict.Add("dashboard_home_vm", _Locator....
11
This code is open to injection. Consider
what happens if I pass sometext'; Drop Table TblTable; -- into
that variable. Use a proper parameterized query instead.
The SQL statement itself is about as efficient as it can be to my
knowledge. A count would add extra overhead. Using an Exists might
garner you a negligible amount of performance at the cost of
...
11
I can't resist mentioning this. These comments hurt me.
'Declaration of Variables
Public Seconds As Integer 'Store Seconds as Integer
Public Minutes As Integer 'Store Minutes as Integer
Public Hours As Integer 'Store Hours as Integer
Public CurHour As Integer 'Store the Current Hour as Integer
Public CurMinute As Integer 'Store the Current Minute as ...
11
Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE
The easiest fix for this is to use binary flags, similar to the following:
[Flags]
...
11
There's probably much more to say about this code, but there's one point that deserves immediate and serious attention: You're handling errors the way vba and vb6 did it... but this code can only be vb.net:
Handles tsbEditNeighbour.Click
In the .NET world, you throw away all this On Error GoTo Madness, and work with Exceptions instead.
Exceptions are a ...
11
This whole function needs a little bit of work:
Dim cmd As SqlCommand = Nothing
Dim conn As SqlConnection = Nothing
Try
Using dt As New DataTable()
conn = New SqlConnection(GetConnectionString())
cmd = New SqlCommand(_command, conn)
cmd.CommandType = _commandType
If (_prmList IsNot Nothing)...
10
You should use Command parameters instead of string concatenation to avoid SQL injection attacks
command.Parameters.Add("@ID", SqlDbType.NVarChar) ' this might not be the appropriate type
command.Parameters("@ID").Value = vVariable
The connection will be closed after the using statement, so you don't need to close it explicitly
connection.Close() '...
10
The function name Convert doesn't seem particularly helpful to me. Convert how? From what to what? In fact, we're not converting anything... we're representing the same unconverted value in a different format.
Dim wnum As Integer = num 'working number
If wnum is the working number, and you feel wnum is so unclear that you need to add a comment to ...
10
This code is bound far too tight to your form. The first thing we need to do is separate the logic for converting an integer to a roman numeral from the event procedures of the form. This will allow us to reuse this code anywhere.
Module IntegerExtensions
Dim ind() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
Dim rom() As String = {"I", "II"...
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 ...
10
In your case it seems like you want to have a re-usable validation method that validates a common part of your form.
Step 1
I wouldn't bind the clicks of the buttons to multiple handlers. This makes the code difficult to debug and can make it unpredictable (which fires first?). Instead, bind the click event to one handler per button, and perform all of ...
10
I will review the tests because they definitely need some work!
GetDataTableTest()
The Test suffix is redundant, they're all tests. What I want to know is what scenario is being tested when the test fails. For this reason I prefer to following the [UnitUnderTest]_[Scenario]_[ExpectedResult] scheme. An example could be ...
10
^P\d{5}-13-TP\d{2}-\d{3}(-(L|R|\d{4})(-\d{3,4})*)?$
Your current regex is WAY too forgiving. First of all, every one of your example starts with a P followed by some numbers, but you accept ANY COMBINATION of letters at the beginning. I'm assuming that ALEX01867-13-TP02-019-L-1137 isn't a valid key, so you should take steps to reject it by using hungry ...
9
Use array, this will reduce by a lot your number of variables.
Name your variable properly ("ten" should be "tenant")
Normaly, variable starts with a lower case in .net
Do not concatenate your strings for queries, this will increase your chances of injections
Your datatable is a bit useless in your case, you can write directly to your variables with a ...
9
Just a few quick remarks (for now):
vbNullString is a remnant of VB6 (which I think would be under the Microsoft.VisualBasic.Constants namespace), and used to be a null string pointer often confused with "", an empty string. In VB.NET vbNullString is Nothing, so when you do this:
If targetFolder = vbNullString Then
You're actually verifying whether ...
8
Now that's some coincidence! No later than this week I wrote an ADODB "wrapper" for some vb6 legacy code (here). This looks very much similar, only for ADO.NET.
I don't like the name. DB is not respecting the PascalCase naming convention for VB.NET classes (same in C#) - I realize it's a short handy name, but it looks like a variable's name. I'd rename the ...
8
You're throwing System.Exception - that's bad, you'll want to throw a meaningful exception. See this answer for more details.
The main problem with this code, is that you're using exceptions to control the execution flow of your program, which is very expensive - I assume you're going to be catching these exceptions somewhere down the call stack.
If a is ...
8
I don't know how much of an optimization it is, but what really jumps out at me is all the conversions between TimeSpan, DateTime, and String. I'd classify it as "code smell" even if it's not bringing down the performance (though you really should run it through a profiler to be sure). I suspect it would be better to use the built-in methods of DateTime and ...
8
Things I like:
You defined the requirements correctly.
Module was probably the right call.
You used index instead of i for your loop. That's Awesome-sauce.
Ok, so what could be done better?
3,5, and 15 are all magic numbers, so it's a good time to introduce some constants. However one of them is not like the others. 3 and 5 are conditions. 15 is the ...
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
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
I was going to edit my first answer, but as I wrote the example code I wanted to show, and refactored, and refactored again, ...the result went so far away from the original code that I thought it warranted a separate answer.
So I copied your code into Visual Studio, and started by implementing the changes I suggested in my other answer. Quickly though, I ...
8
Option Strict
You should always set Option Strict = On.
Short circuit evaluation
You should use the AndAlso operator instead of the And operator. By using And both conditions will be evaluated. By using AndAlso the second condition will only be evaluated if the first returns true.
List(Of FileInfo)
Instead of returning a List(Of FileInfo) you ...
8
Your comments should actually give you a hint.
' Getting data from DB:
' ...
' Populate retValue list with project names:
These sound like the beginnings of good function names to me.
Private Function GetAutocompleteItems(reader as DbDataReader) as List(Of AutocompleteItem)
If reader.HasRows Then
retValue = New List(Of AutocompleteItem)
...
8
So, you have a couple of issues here. First, your question...
What I've done in VB6 (which your question was originally tagged as) is create a module that is responsible for reporting errors and logging them. I would imagine seeing your error handler look something like this.
If Err.Number <> 0 Then
ReportError(Me.Name, "tsbEditNeighbor_Click", ...
8
Before we start, take a look at this comment of yours. It's a prime example of why you shouldn't always trust the comments.
'This gets the text that comes before the :
'Example Company:TextArea would return TextArea.
controlOption = textSelection.Substring(0, textSelection.IndexOf(":"c))
The text before : is in fact Company ;)
Now, the name of the ...
Only top voted, non community-wiki answers of a minimum length are eligible
