Skip to main content
Code Review

Return to Answer

replaced http://programmers.stackexchange.com/ with https://softwareengineering.stackexchange.com/
Source Link

This is pretty good and optimal already. Just a few hints here and there

I would take the

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

and throw that into the Profiler class due to the class being the only one needing it. Trust me it's easy to forget to declare that on a new project in standard module and annoy the sh** out of yourself.

You seem inconsistent about naming your members. Some subs are camelCase, others are PascalCase. Decide which one you're going to use and apply throughout the code.

Generally, your project encapsulation is very loose. I mean, everything depends on everything if that makes any sense; (*see interesting post here* see interesting post here*)

I would think that ProfileManager should include members like Reset and GetProfileManager. BUT since your Profiler needs to access the manager in its Class_Terminate() event and you can't pass parameters nor can you raise an event to notify the manager that the object is about to get destroyed there isn't any other way to achieve this as far as I know due to your current design...

I played with the code for about 2 hours but my conclusion is based on an assumption that you want to minimize it down to just creating an instance of Profiler and calling one Let property without having to explicitly destroy the instance (2 lines of code currently) ie. relying on the Class_Terminate event - you've already got the best approach and there is not much room for further optimizations.

I am still not quite sure what the purpose of your ResetProfileManager() sub is so currently I find it a bit redundant since you could just do this once:

Private manager As ProfileManager
Sub MainProfilerTest()
 Set manager = New ProfileManager
...

Oh btw. I would have changed the mProfileManager to just manager.. makes things simpler.

And further to the above if you explicitly assign a new ProfilerManager to your manager you could improve the performance a bit by modifying the GetProfileManager by removing unnecessary check:

Public Function GetProfileManager() As ProfileManager
 Set GetProfileManager = manager
End Function

Also, in ProfileManager consider using simpler names like:

Private times As Dictionary
Private calls As Dictionary

And in the Profiler:

Private initTime As Double
Private method As String

I would also add the Class_Terminate event to the ProfileManger and free refs to the Dictionaries

Private Sub Class_Terminate()
 Set times = Nothing
 Set calls = Nothing
End Sub

I think it's matter of preference but generally I don't prefix Dictionary with Scripting. Any decent person who's even a bit familiar with VBA will know that Dictionary is Scripting.Dictionary.

I also considered shortening the declaration to one line using imitation of a static class in combination with parametarised constructors but I've seen your chat message from last night saying that you wouldn't want to do that.

I guess that's about all I can say about your code. Like I said if you didn't mind to explicitly destroy your Profile instances that would completely change the story :)

This is pretty good and optimal already. Just a few hints here and there

I would take the

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

and throw that into the Profiler class due to the class being the only one needing it. Trust me it's easy to forget to declare that on a new project in standard module and annoy the sh** out of yourself.

You seem inconsistent about naming your members. Some subs are camelCase, others are PascalCase. Decide which one you're going to use and apply throughout the code.

Generally, your project encapsulation is very loose. I mean, everything depends on everything if that makes any sense; (*see interesting post here*)

I would think that ProfileManager should include members like Reset and GetProfileManager. BUT since your Profiler needs to access the manager in its Class_Terminate() event and you can't pass parameters nor can you raise an event to notify the manager that the object is about to get destroyed there isn't any other way to achieve this as far as I know due to your current design...

I played with the code for about 2 hours but my conclusion is based on an assumption that you want to minimize it down to just creating an instance of Profiler and calling one Let property without having to explicitly destroy the instance (2 lines of code currently) ie. relying on the Class_Terminate event - you've already got the best approach and there is not much room for further optimizations.

I am still not quite sure what the purpose of your ResetProfileManager() sub is so currently I find it a bit redundant since you could just do this once:

Private manager As ProfileManager
Sub MainProfilerTest()
 Set manager = New ProfileManager
...

Oh btw. I would have changed the mProfileManager to just manager.. makes things simpler.

And further to the above if you explicitly assign a new ProfilerManager to your manager you could improve the performance a bit by modifying the GetProfileManager by removing unnecessary check:

Public Function GetProfileManager() As ProfileManager
 Set GetProfileManager = manager
End Function

Also, in ProfileManager consider using simpler names like:

Private times As Dictionary
Private calls As Dictionary

And in the Profiler:

Private initTime As Double
Private method As String

I would also add the Class_Terminate event to the ProfileManger and free refs to the Dictionaries

Private Sub Class_Terminate()
 Set times = Nothing
 Set calls = Nothing
End Sub

I think it's matter of preference but generally I don't prefix Dictionary with Scripting. Any decent person who's even a bit familiar with VBA will know that Dictionary is Scripting.Dictionary.

I also considered shortening the declaration to one line using imitation of a static class in combination with parametarised constructors but I've seen your chat message from last night saying that you wouldn't want to do that.

I guess that's about all I can say about your code. Like I said if you didn't mind to explicitly destroy your Profile instances that would completely change the story :)

This is pretty good and optimal already. Just a few hints here and there

I would take the

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

and throw that into the Profiler class due to the class being the only one needing it. Trust me it's easy to forget to declare that on a new project in standard module and annoy the sh** out of yourself.

You seem inconsistent about naming your members. Some subs are camelCase, others are PascalCase. Decide which one you're going to use and apply throughout the code.

Generally, your project encapsulation is very loose. I mean, everything depends on everything if that makes any sense; (*see interesting post here*)

I would think that ProfileManager should include members like Reset and GetProfileManager. BUT since your Profiler needs to access the manager in its Class_Terminate() event and you can't pass parameters nor can you raise an event to notify the manager that the object is about to get destroyed there isn't any other way to achieve this as far as I know due to your current design...

I played with the code for about 2 hours but my conclusion is based on an assumption that you want to minimize it down to just creating an instance of Profiler and calling one Let property without having to explicitly destroy the instance (2 lines of code currently) ie. relying on the Class_Terminate event - you've already got the best approach and there is not much room for further optimizations.

I am still not quite sure what the purpose of your ResetProfileManager() sub is so currently I find it a bit redundant since you could just do this once:

Private manager As ProfileManager
Sub MainProfilerTest()
 Set manager = New ProfileManager
...

Oh btw. I would have changed the mProfileManager to just manager.. makes things simpler.

And further to the above if you explicitly assign a new ProfilerManager to your manager you could improve the performance a bit by modifying the GetProfileManager by removing unnecessary check:

Public Function GetProfileManager() As ProfileManager
 Set GetProfileManager = manager
End Function

Also, in ProfileManager consider using simpler names like:

Private times As Dictionary
Private calls As Dictionary

And in the Profiler:

Private initTime As Double
Private method As String

I would also add the Class_Terminate event to the ProfileManger and free refs to the Dictionaries

Private Sub Class_Terminate()
 Set times = Nothing
 Set calls = Nothing
End Sub

I think it's matter of preference but generally I don't prefix Dictionary with Scripting. Any decent person who's even a bit familiar with VBA will know that Dictionary is Scripting.Dictionary.

I also considered shortening the declaration to one line using imitation of a static class in combination with parametarised constructors but I've seen your chat message from last night saying that you wouldn't want to do that.

I guess that's about all I can say about your code. Like I said if you didn't mind to explicitly destroy your Profile instances that would completely change the story :)

added 84 characters in body
Source Link
user28366
user28366

This is pretty good and optimal already. Just a few hints here and there

I would take the

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

and throw that into the Profiler class due to the class being the only one needing it. Trust me it's easy to forget to declare that on a new project in standard module and annoy the sh** out of yourself.

You seem inconsistent about naming your members. Some subs are camelCase, others are PascalCase. Decide which one you're going to use and apply throughout the code.

Generally, your project encapsulation is very loose. I mean, everything depends on everything if that makes any sense; (*see interesting post here* )

I would think that ProfileManager should include members like Reset and GetProfileManager. BUT since your Profiler needs to access the manager in its Class_Terminate() event and you can't pass parameters nor can you raise an event to notify the manager that the object is about to get destroyed there isn't any other way to achieve this as far as I know due to your current design...

I played with the code for about 2 hours but my conclusion is based on an assumption that you want to minimize it down to just creating an instance of Profiler and calling one Let property without having to explicitly destroy the instance (2 lines of code currently) ie. relying on the Class_Terminate event - you've already got the best approach and there is not much room for further optimizations.

I am still not quite sure what the purpose of your ResetProfileManager() sub is so currently I find it a bit redundant since you could just do this once:

Private manager As ProfileManager
Sub MainProfilerTest()
 Set manager = New ProfileManager
...

Oh btw. I would have changed the mProfileManager to just manager.. makes things simpler.

And further to the above if you explicitly assign a new ProfilerManager to your manager you could improve the performance a bit by modifying the GetProfileManager by removing unnecessary check:

Public Function GetProfileManager() As ProfileManager
 Set GetProfileManager = manager
End Function

Also, in ProfileManager consider using simpler names like:

Private times As Dictionary
Private calls As Dictionary

And in the Profiler:

Private initTime As Double
Private method As String

I would also add the Class_Terminate event to the ProfileManger and free refs to the Dictionaries

Private Sub Class_Terminate()
 Set times = Nothing
 Set calls = Nothing
End Sub

I think it's matter of preference but generally I don't prefix Dictionary with Scripting. Any decent person who's even a bit familiar with VBA will know that Dictionary is Scripting.Dictionary.

I also considered shortening the declaration to one line using imitation of a static class in combination with parametarised constructors but I've seen your chat message from last night saying that you wouldn't want to do that.

I guess that's about all I can say about your code. Like I said if you didn't mind to explicitly destroy your Profile instances that would completely change the story :)

This is pretty good and optimal already. Just a few hints here and there

I would take the

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

and throw that into the Profiler class due to the class being the only one needing it. Trust me it's easy to forget to declare that on a new project in standard module and annoy the sh** out of yourself.

You seem inconsistent about naming your members. Some subs are camelCase, others are PascalCase. Decide which one you're going to use and apply throughout the code.

Generally, your project encapsulation is very loose. I mean, everything depends on everything if that makes any sense;

I would think that ProfileManager should include members like Reset and GetProfileManager. BUT since your Profiler needs to access the manager in its Class_Terminate() event and you can't pass parameters nor can you raise an event to notify the manager that the object is about to get destroyed there isn't any other way to achieve this as far as I know due to your current design...

I played with the code for about 2 hours but my conclusion is based on an assumption that you want to minimize it down to just creating an instance of Profiler and calling one Let property without having to explicitly destroy the instance (2 lines of code currently) ie. relying on the Class_Terminate event - you've already got the best approach and there is not much room for further optimizations.

I am still not quite sure what the purpose of your ResetProfileManager() sub is so currently I find it a bit redundant since you could just do this once:

Private manager As ProfileManager
Sub MainProfilerTest()
 Set manager = New ProfileManager
...

Oh btw. I would have changed the mProfileManager to just manager.. makes things simpler.

And further to the above if you explicitly assign a new ProfilerManager to your manager you could improve the performance a bit by modifying the GetProfileManager by removing unnecessary check:

Public Function GetProfileManager() As ProfileManager
 Set GetProfileManager = manager
End Function

Also, in ProfileManager consider using simpler names like:

Private times As Dictionary
Private calls As Dictionary

And in the Profiler:

Private initTime As Double
Private method As String

I would also add the Class_Terminate event to the ProfileManger and free refs to the Dictionaries

Private Sub Class_Terminate()
 Set times = Nothing
 Set calls = Nothing
End Sub

I think it's matter of preference but generally I don't prefix Dictionary with Scripting. Any decent person who's even a bit familiar with VBA will know that Dictionary is Scripting.Dictionary.

I also considered shortening the declaration to one line using imitation of a static class in combination with parametarised constructors but I've seen your chat message from last night saying that you wouldn't want to do that.

I guess that's about all I can say about your code. Like I said if you didn't mind to explicitly destroy your Profile instances that would completely change the story :)

This is pretty good and optimal already. Just a few hints here and there

I would take the

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

and throw that into the Profiler class due to the class being the only one needing it. Trust me it's easy to forget to declare that on a new project in standard module and annoy the sh** out of yourself.

You seem inconsistent about naming your members. Some subs are camelCase, others are PascalCase. Decide which one you're going to use and apply throughout the code.

Generally, your project encapsulation is very loose. I mean, everything depends on everything if that makes any sense; (*see interesting post here* )

I would think that ProfileManager should include members like Reset and GetProfileManager. BUT since your Profiler needs to access the manager in its Class_Terminate() event and you can't pass parameters nor can you raise an event to notify the manager that the object is about to get destroyed there isn't any other way to achieve this as far as I know due to your current design...

I played with the code for about 2 hours but my conclusion is based on an assumption that you want to minimize it down to just creating an instance of Profiler and calling one Let property without having to explicitly destroy the instance (2 lines of code currently) ie. relying on the Class_Terminate event - you've already got the best approach and there is not much room for further optimizations.

I am still not quite sure what the purpose of your ResetProfileManager() sub is so currently I find it a bit redundant since you could just do this once:

Private manager As ProfileManager
Sub MainProfilerTest()
 Set manager = New ProfileManager
...

Oh btw. I would have changed the mProfileManager to just manager.. makes things simpler.

And further to the above if you explicitly assign a new ProfilerManager to your manager you could improve the performance a bit by modifying the GetProfileManager by removing unnecessary check:

Public Function GetProfileManager() As ProfileManager
 Set GetProfileManager = manager
End Function

Also, in ProfileManager consider using simpler names like:

Private times As Dictionary
Private calls As Dictionary

And in the Profiler:

Private initTime As Double
Private method As String

I would also add the Class_Terminate event to the ProfileManger and free refs to the Dictionaries

Private Sub Class_Terminate()
 Set times = Nothing
 Set calls = Nothing
End Sub

I think it's matter of preference but generally I don't prefix Dictionary with Scripting. Any decent person who's even a bit familiar with VBA will know that Dictionary is Scripting.Dictionary.

I also considered shortening the declaration to one line using imitation of a static class in combination with parametarised constructors but I've seen your chat message from last night saying that you wouldn't want to do that.

I guess that's about all I can say about your code. Like I said if you didn't mind to explicitly destroy your Profile instances that would completely change the story :)

Source Link
user28366
user28366

This is pretty good and optimal already. Just a few hints here and there

I would take the

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

and throw that into the Profiler class due to the class being the only one needing it. Trust me it's easy to forget to declare that on a new project in standard module and annoy the sh** out of yourself.

You seem inconsistent about naming your members. Some subs are camelCase, others are PascalCase. Decide which one you're going to use and apply throughout the code.

Generally, your project encapsulation is very loose. I mean, everything depends on everything if that makes any sense;

I would think that ProfileManager should include members like Reset and GetProfileManager. BUT since your Profiler needs to access the manager in its Class_Terminate() event and you can't pass parameters nor can you raise an event to notify the manager that the object is about to get destroyed there isn't any other way to achieve this as far as I know due to your current design...

I played with the code for about 2 hours but my conclusion is based on an assumption that you want to minimize it down to just creating an instance of Profiler and calling one Let property without having to explicitly destroy the instance (2 lines of code currently) ie. relying on the Class_Terminate event - you've already got the best approach and there is not much room for further optimizations.

I am still not quite sure what the purpose of your ResetProfileManager() sub is so currently I find it a bit redundant since you could just do this once:

Private manager As ProfileManager
Sub MainProfilerTest()
 Set manager = New ProfileManager
...

Oh btw. I would have changed the mProfileManager to just manager.. makes things simpler.

And further to the above if you explicitly assign a new ProfilerManager to your manager you could improve the performance a bit by modifying the GetProfileManager by removing unnecessary check:

Public Function GetProfileManager() As ProfileManager
 Set GetProfileManager = manager
End Function

Also, in ProfileManager consider using simpler names like:

Private times As Dictionary
Private calls As Dictionary

And in the Profiler:

Private initTime As Double
Private method As String

I would also add the Class_Terminate event to the ProfileManger and free refs to the Dictionaries

Private Sub Class_Terminate()
 Set times = Nothing
 Set calls = Nothing
End Sub

I think it's matter of preference but generally I don't prefix Dictionary with Scripting. Any decent person who's even a bit familiar with VBA will know that Dictionary is Scripting.Dictionary.

I also considered shortening the declaration to one line using imitation of a static class in combination with parametarised constructors but I've seen your chat message from last night saying that you wouldn't want to do that.

I guess that's about all I can say about your code. Like I said if you didn't mind to explicitly destroy your Profile instances that would completely change the story :)

lang-vb

AltStyle によって変換されたページ (->オリジナル) /