Return to Answer

Public Sub Class_Initialize()
End Sub

Avoid empty members; this initializer serves no purpose, remove it.

Although I could infer r and c are meant for row and column, these single-letter parameters should probably be called row and column, for clarity. Likewise, Cols should probably be called Columns.

This is unfortunate:

Public Property Let Value(r As Long, c As Long, val As Double)

I'd consider calling the property ValueAt, and the val parameter could then be called value - and since parameters are passed ByRef by default, I'd be explicit about them being passed ByVal - there's no need to pass them by reference:

Public Property Let ValueAt(ByVal rowIndex As Long, ByVal columnIndex As Long, ByVal value As Double)

In the case of LoadMatrixString, I'd consider changing the signature from this:

Public Sub LoadMatrixString(str As String)

To that:

Public Sub LoadMatrixString(ByVal values As String)

And for the members that take a m As Matrix parameter, I'd go with ByVal value As Matrix and avoid single-letter identifiers. I find "value" remains the most descriptive name in these contexts.

There's an inconsistency in the way you're naming "Dimensions": you have CheckDimensions, but then you also have GetDims - I'd rename the latter GetDimensions.

I like how the class is self-contained, but then it seems to me like the ToString implementation would be a perfect excuse to use your wonderful StringBuilder class, and I bet you'd get the string output much, much faster ;)

As for this:

I'm particularly intersted to know whether I should split the parser off into a separate class to be used independently, or maybe be called by the Matrix class itself.

I think you could simply move the parsing code to a MatrixParser class, and be done with it! ...Actually, I'd copy the LoadMatrixString procedure there, and rename it Parse, make it a Function and have it return a Matrix. Then LoadMatrixString could be modified to call this new function.

Public Sub Class_Initialize()
End Sub

Avoid empty members; this initializer serves no purpose, remove it.

Although I could infer r and c are meant for row and column, these single-letter parameters should probably be called row and column, for clarity. Likewise, Cols should probably be called Columns.

This is unfortunate:

Public Property Let Value(r As Long, c As Long, val As Double)

I'd consider calling the property ValueAt, and the val parameter could then be called value - and since parameters are passed ByRef by default, I'd be explicit about them being passed ByVal - there's no need to pass them by reference:

Public Property Let ValueAt(ByVal rowIndex As Long, ByVal columnIndex As Long, ByVal value As Double)

In the case of LoadMatrixString, I'd consider changing the signature from this:

Public Sub LoadMatrixString(str As String)

To that:

Public Sub LoadMatrixString(ByVal values As String)

And for the members that take a m As Matrix parameter, I'd go with ByVal value As Matrix and avoid single-letter identifiers. I find "value" remains the most descriptive name in these contexts.

There's an inconsistency in the way you're naming "Dimensions": you have CheckDimensions, but then you also have GetDims - I'd rename the latter GetDimensions.

I like how the class is self-contained, but then it seems to me like the ToString implementation would be a perfect excuse to use your wonderful StringBuilder class, and I bet you'd get the string output much, much faster ;)

As for this:

I'm particularly intersted to know whether I should split the parser off into a separate class to be used independently, or maybe be called by the Matrix class itself.

I think you could simply move the parsing code to a MatrixParser class, and be done with it! ...Actually, I'd copy the LoadMatrixString procedure there, and rename it Parse, make it a Function and have it return a Matrix. Then LoadMatrixString could be modified to call this new function.

Public Sub Class_Initialize()
End Sub

Avoid empty members; this initializer serves no purpose, remove it.

Although I could infer r and c are meant for row and column, these single-letter parameters should probably be called row and column, for clarity. Likewise, Cols should probably be called Columns.

This is unfortunate:

Public Property Let Value(r As Long, c As Long, val As Double)

I'd consider calling the property ValueAt, and the val parameter could then be called value - and since parameters are passed ByRef by default, I'd be explicit about them being passed ByVal - there's no need to pass them by reference:

Public Property Let ValueAt(ByVal rowIndex As Long, ByVal columnIndex As Long, ByVal value As Double)

In the case of LoadMatrixString, I'd consider changing the signature from this:

Public Sub LoadMatrixString(str As String)

To that:

Public Sub LoadMatrixString(ByVal values As String)

And for the members that take a m As Matrix parameter, I'd go with ByVal value As Matrix and avoid single-letter identifiers. I find "value" remains the most descriptive name in these contexts.

There's an inconsistency in the way you're naming "Dimensions": you have CheckDimensions, but then you also have GetDims - I'd rename the latter GetDimensions.

I like how the class is self-contained, but then it seems to me like the ToString implementation would be a perfect excuse to use your wonderful StringBuilder class, and I bet you'd get the string output much, much faster ;)

Public Sub Class_Initialize()
End Sub

Avoid empty members; this initializer serves no purpose, remove it.

Although I could infer r and c are meant for row and column, these single-letter parameters should probably be called row and column, for clarity. Likewise, Cols should probably be called Columns.

This is unfortunate:

Public Property Let Value(r As Long, c As Long, val As Double)

I'd consider calling the property ValueAt, and the val parameter could then be called value - and since parameters are passed ByRef by default, I'd be explicit about them being passed ByVal - there's no need to pass them by reference:

Public Property Let ValueAt(ByVal rowIndex As Long, ByVal columnIndex As Long, ByVal value As Double)

In the case of LoadMatrixString, I'd consider changing the signature from this:

Public Sub LoadMatrixString(str As String)

To that:

Public Sub LoadMatrixString(ByVal values As String)

And for the members that take a m As Matrix parameter, I'd go with ByVal value As Matrix and avoid single-letter identifiers. I find "value" remains the most descriptive name in these contexts.

There's an inconsistency in the way you're naming "Dimensions": you have CheckDimensions, but then you also have GetDims - I'd rename the latter GetDimensions.

I like how the class is self-contained, but then it seems to me like the ToString implementation would be a perfect excuse to use your wonderful StringBuilder class, and I bet you'd get the string output much, much faster ;)

As for this:

I'm particularly intersted to know whether I should split the parser off into a separate class to be used independently, or maybe be called by the Matrix class itself.

I think you could simply move the parsing code to a MatrixParser class, and be done with it! ...Actually, I'd copy the LoadMatrixString procedure there, and rename it Parse, make it a Function and have it return a Matrix. Then LoadMatrixString could be modified to call this new function.