Kamis, 23 April 2015

Re: [ExcelVBA] Auto-populate a cell based on several other values

 

Cheers Dave. Good comments provided and I'll revise the code.

Don't worry about the length of the e-mail...I did ask! 😁

Duncs

On 24 Apr 2015 06:00, "'David Smart' smartware.consulting@gmail.com [ExcelVBA]" <ExcelVBA@yahoogroups.com> wrote:
 



Comments comment - and I do understand that you've commented more than you might normally.  Pretty much all of these comments are just describing the action of the following line of code, and can be omitted.  Also, gotta be careful not to write a comment that isn't accurate (e.g. "Set worksheet value" where the code is actually storing a reference to "Sheet1").
 
-------------------------
 
Don't overuse "With".  E.g.
 
    With WS.UsedRange
        NumRows = .Rows.Count
    End With
should just be
 
        NumRows = WS.UsedRange.Rows.Count
which is easier to read and two lines shorter.  By all means use With when it serves two or more clauses in the code.
 
-------------------------
 
Naming case.  You have variable names starting with capital letters (e.g. StartRow) and names starting with lower-case letters (e.g. tmpPriority and i).  You need to be consistent.  (Note that I'm not including the "p" prefix, which is quite reasonable as lower-case.)
 
-------------------------
 
Column constants (e.g. Column_G = 7).  Just use "G" in the "Cells" call.  If you are looking to give a name to the column, then "Column_G" is not meaningful anyway.  By all means call it OutcomeColumn and then it will increase the readability.
 
-------------------------
 
' Does this text = "Pass"
If ResultText <> "Pass" Then
  ' Cell contains "Pass", ...
Ummm ... it is super important that comments be correct (or simply omit them).
 
-------------------------
 
' Cell contains "Pass", so check the value and set the Priority variable
' as appropriate.
 
Now this is a good place to have a comment, but this isn't it.  By all means remind of the strategy - e.g. a "de-scoped" overrides a "pass"; a "blocked" overrides both of these; and a "fail" overrides everything.
 
-------------------------
 
' Is the current priority value less than the temporary priority?
' Has a "higher" priority been recorded
If tmpPriority < Priority Then
Two problems here ... firstly, the first comment line contradicts the code.  Second is more subtle.  Although you have "higher" in quotes, it will still be read as meaning higher numerically.  Best solution is to actually rank the action statuses in ascending order, which is probably more logical anyway.  The other way is to avoid "higher" and perhaps say "worse".
 
-------------------------
 
' Yes, so change the temporary priority to the new priority
Priority = tmpPriority
Again the comment is wrong (and fairly meaningless anyway).
 
-------------------------
 
As a matter of principal I try to avoid use of "temporary" as part of variable names.  You can usually arrive at more descriptive names that will aid readability.  For instance, in this case you have Priority and tmpPriority, which really doesn't help much (and could be the reason you have errors in the comments).  As I see it, Priority is the priority result of the test and tmpPriority is the priority result of the step in the test.  So TestPriority and StepPriority would seem more appropriate.
 
Also in this case, I'd wonder about your naming it as "Priority" in the first place.  Surely it's "Result".  I suspect that change would make the code easier to read.
 
-------------------------
 
Case 3:
 
You have defined constants for the various values, so use them.  Don't use the actual integers here.
 
-------------------------
 
Case 3:
Case 2:
Case 1:
Case 4:
Unless there's a good reason for it, keep your case statements in numerical or significance order.
 
-------------------------
 
For i = 9 To NumRows
...
RowNum = RowNum + 1
 
should just have been
 
For RowNum = 9 To NumRows
or drop RowNum completely and just use i.
 
-------------------------
 
' Have we reached a new section?
' i.e. "Sequence" = 1
(BTW this is a good comment.  Even the second line of it, as it's a reminder that the 1 is significant.)
 
However, this whole bit of code is far too late in the For loop.  I think I mentioned this before.  This whole bit of code should come directly after the For statement, I think.
 
-------------------------
 
Next
 
Please always put the loop variable in the Next statement, in this case
 
Next i
 
If you've got several nested For loops, it becomes very important for readability.
 
-------------------------
 
Apologies for such a long reply, but you did ask.  :-)  :-)
 
I'll try to grab a spare few minutes later and write my version of it.

Regards, Dave S
 
----- Original Message -----
Sent: Friday, April 24, 2015 6:52 AM
Subject: Re: [ExcelVBA] Auto-populate a cell based on several other values

Sorry for nor having replied to this sooner.  Busy, busy, busy…why can't we have a four day week and a three day weekend?

Anyway, here's the code.  I commented it to help me get a grip with what it was supposed to be doing, and ensuring that my logic made sense.  Feel free to critique.

Indentation may be a bit mixed up…it's how it looked after a copy and paste.

Cheers

Duncs

Sub PopulatePass()

    ' Constants representing action status
Const pFail = 1
Const pBlocked = 2
Const pDeScoped = 3
Const pPass = 4

    ' Column number constants
Const Column_E = 5
Const Column_G = 7
Const Column_H = 8

    ' Keep track of the row number being processed
Dim RowNum As Integer

    ' Represents the worksheet being processed
Dim WS As Worksheet

    ' Hold the priority value for action selection and row processing
Dim Priority As Byte

    ' Setup the starting row, i.e. first row of data on sheet
    ' StartRow used to identify the row that contains the '1' for a new section
    ' RowNum used to work through each row of the sheet
StartRow = 9
RowNum = 9
    
    ' Set worksheet value
Set WS = Worksheets("Sheet1")

    ' Get number of rows of data in the worksheet
With WS.UsedRange
    NumRows = .Rows.Count
End With
    
    ' Set Priority / temp Priority to default to Pass
Priority = pPass
tmpPriority = pPass
    
    ' Loop through all rows on the worksheet
    ' From the first row of data, row 9, to the last row of data
For i = 9 To NumRows

        ' The content of the Outcome result cell is recorded
    ResultText = WS.Cells(RowNum, Column_G)
        
        ' Does this text = "Pass"
    If ResultText <> "Pass" Then
        
            ' Cell contains "Pass", so check the value and set the Priority variable
            ' as appropriate.
            ' This is defaulted to Pass

            Select Case ResultText
                Case "Fail"
                    tmpPriority = pFail
                Case "Blocked"
                    tmpPriority = pBlocked
                Case "De-Scoped"
                    tmpPriority = pDeScoped
            End Select
        
    End If
        
        ' Is the current priority value less than the temporary priority?
        ' Has a "higher" priority been recorded
    If tmpPriority < Priority Then
        
            ' Yes, so change the temporary priority to the new priority
        Priority = tmpPriority
    End If

        ' Check the vqalue now held in the priority variable, and set the contents of the "Action Outcome" 
        ' cell appropriately
     Select Case Priority
            Case 3:
                WS.Cells(StartRow, Column_H) = "De-Scoped"
            Case 2:
                WS.Cells(StartRow, Column_H) = "Blocked"
            Case 1:
                WS.Cells(StartRow, Column_H) = "Fail"
            Case 4:
                WS.Cells(StartRow, Column_H) = "Pass"
     End Select
    
            ' Move to the next row
        RowNum = RowNum + 1
        
            ' Set the temporary priority to the current value
        'tmpPriority = Priority
    
            ' Have we reached a new section?
            ' i.e. "Sequence" = 1
        If WS.Cells(RowNum, Column_E) = 1 Then
        
                ' Yes, so set:
                ' StartRow to the current row...Action result goes here
                ' Priority to pass...Action result defaults to Pass
                ' tmpPriority to Pass...Priority values default to Pass
            StartRow = RowNum
            Priority = pPass
            tmpPriority = pPass
        
        End If

    Next
    
End Sub


From: Duncan Edment <duncan.edment@gmail.com>
Date: Wednesday, 22 April 2015 08:17
To: <ExcelVBA@yahoogroups.com>
Subject: Re: [ExcelVBA] Auto-populate a cell based on several other values

Sorry I haven't replied. Is it OK to post the code in a message?

Duncs

On 20 Apr 2015 19:04, "Green 1z@compuserve.com [ExcelVBA]" <ExcelVBA@yahoogroups.com> wrote:
 

Hi Dave S,


This is soooo great!!! I love this stuff! It's so nice to see how other people code!

I posted something similar at the code cage some time ago.

Here are Jan Karel Pieterse' answers from there...
> No Comments before the procedure declaration line
Agreed

> Comments directly below the procedure declaration
Agreed

> Space Line
Usually not, except when procedure declaration spans more than one line (to easily separate proc declaration from rest)

> Dims
> Space Line
> Code
> A comment line of stars
Not sure why?

> End of procedure line
> With...
> No Multiple single lines with colons.
Agreed!

> No end of line comments.
I'm not opposed against them

> No If statements on a single line.
Agreed

> All Booleans to say If Boolean = True/False Then rather than If Boolean Then.
Disagree, I would omit = True

> All groups of comment lines preceded by a space line.
> Comment line directly followed by the code they comment.
> All dims together
> Indent of two spaces
Disagree, I find 4 spaces easier, guess this is really a matter of personal preference

> Use continuations a lot to try and keep all of the code visible in the standard VBE code window.
Agreed, but hard to keep up if you work on different setups with different screen resolutions.





It's a shame though that more people haven't responded.. hence my late reply.

Thank you Dave for giving us your "take"!

Some of my style is because it's easier to write code to alter and format the code. I've written a SortDims proc for example which would be pretty silly without all dims being together.... and Of course a proc to put all of the dims together! :-)
My biggest problem with end of line comments is that they tend to be too long!! So I've written a proc to put them on the line above... and so on and so on.

If you want to check what I've done in that area I have an add in available at the code cage or I can send you a zip file. 

I do realize though that you are probably too busy so please don't feel obliged.

Hugs
Lisa

To: ExcelVBA <ExcelVBA@yahoogroups.com>
Sent: Thu, Apr 16, 2015 1:28 am
Subject: Re: [ExcelVBA] Auto-populate a cell based on several other values



Love the differences, my take on yours ...
> No Multiple single lines with colons.
Agreed.

> No end of line comments.
I use end of line comments when they say something about the actual line - e.g. the units of calculation - if I think it avoids confusion (especially when using a built-in function that isn't named as well as it might be).  In languages that allow it, I will actually put this comment in the middle of the code.

> No If statements on a single line.
Agreed, but I do sometimes break this rule, when I feel it helps the readability of the whole block of code.

> All Booleans to say If Boolean = True/False Then rather than If Boolean Then.
Nope.  Always makes me wonder whether the variable is inadequately named.  And if I want to test for a False, I will use the "Not" construct.

> All groups of comment lines preceded by a space line.
Agreed, and followed by a space line too.

> Comment line directly followed by the code they comment.
If you mean "no space line", then don't agree.

> All dims together
No.  In fact, I Dim then colon then assignment statement on the one line, where possible.  Or Dim as late as possible.
This comes from using languages that allow (recommend) declaration of variables on the line of initial use and take their scope from the code block indent level they're declared at.

> Indent of two spaces
Agreed.  4 is far too big.  Also, tabs converted to spaces.

> Use continuations a lot to try and keep all of the code visible in the standard VBE code window.
Don't agree.  I only continue if I think it adds to the readability of the block of code it's in.  Otherwise, I go for the readability of the whole block, rather than the readability of individual lines.  (I.e. as many lines as possible on the screen, even if I need to scroll to see the remainder of the line.)
> Comments directly below the procedure declaration
Yes, I tend to do this (with a space line top and bottom) but I don't comment procedure declarations much anyway.  (1) I make the procedure name self-explanatory if possible.  (2) I keep procedures small (aka single-purpose) and have lots of them.  (3) In Excel, I put the code in the sheet modules as much as possible and use classes when they're useful, which means that the procedure names are associated with the sheet involved, and thereby obvious to what they're working on.
My different approach is doubtless associated with the fact that OO languages (Java, C++, Ada, etc) account for at least 95% of programming I do.
One very strict rule I impose on myself and anyone working with me is that the style used must be consistent through a whole module.  Either new code must be added in the existing style of the module, or the style of the whole module must be changed.  (I don't feel a need for different modules to be in the same style, though.)

Regards, Dave S
 
----- Original Message -----
Sent: Wednesday, April 15, 2015 9:44 PM
Subject: Re: [ExcelVBA] Auto-populate a cell based on several other values

> Styles vary so much.
Oooh boy!! Or in my case... girl!

> "g" when a global variable is defined in a separate "globals" module
At least you have some sort of naming convention.. minimalist though it is!   :-)

IMHO ... Wormy or not and even if it has been discussed torn apart and put together dissected zillions of times it still bears looking at. 

FOr example... just to put the the cat out there... Arguably the greatest coding innovation ever has been indentation!

My personal preferred format is....

No Comments before the procedure declaration line
Comments directly below the procedure declaration
Space Line
Dims
Space Line
Code
A comment line of stars
End of procedure line

With...
No Multiple single lines with colons.
No end of line comments.
No If statements on a single line.
All Booleans to say If Boolean = True/False Then rather than If Boolean Then.
All groups of comment lines preceded by a space line.
Comment line directly followed by the code they comment.
All dims together
Indent of two spaces
Use continuations a lot to try and keep all of the code visible in the standard VBE code window.


LIsa










To: ExcelVBA <ExcelVBA@yahoogroups.com>
Sent: Mon, Apr 13, 2015 5:25 am
Subject: Re: [ExcelVBA] Auto-populate a cell based on several other values



Oops - I've probably opened a can of worms.  :-)  My thinking was around coding techniques, rather than styles.
Coding style discussions are fun, of course.  Styles vary so much.  For instance, you use prefixes (sub, fnc, lng, etc) - I never do (except for a "g" when a global variable is defined in a separate "globals" module). 

Regards, Dave S
 
----- Original Message -----
Sent: Monday, April 13, 2015 5:13 AM
Subject: Re: [ExcelVBA] Auto-populate a cell based on several other values

post the code and ask for coding style suggestions
Actually a great idea!!!
Everyone has different styles so this would be a terrific "excersize" for the group to chip in!

Example.... I use a naming convention for entities of my own which is...
Sub procedures always preceeded bur lowercase "sub" and starting with a capital after that.
ex... subInitialize()

Function procedures likewise but with "fnc" instead of sub.

I include a type and a scope in variable prefixes....
Dim lnglN as Long... lng = long l = local.

The single letter l can be p=parameter m=module level g=global

I also format my code in a particular manner. If people are interested I'd be happy to share.

Any other offers?? Anyone use a personal set of "rules"? What are they?




I'd also be very interested to know how adding comments solved your probbie Duncan?

Lisa


-----Original Message-----
From: 'David Smart' smartware.consulting@gmail.com [ExcelVBA] < ExcelVBA@yahoogroups.com>
To: ExcelVBA < ExcelVBA@yahoogroups.com>
Sent: Sat, Apr 11, 2015 1:37 am
Subject: Re: [ExcelVBA] Auto-populate a cell based on several other values



Glad you got it working.
If you'd like "improvement" commentary, by all means post the code and ask for coding style suggestions.

Regards, Dave S
 
----- Original Message -----
Sent: Friday, April 10, 2015 10:54 PM
Subject: Re: [ExcelVBA] Auto-populate a cell based on several other values

Guys, many thanks for the help, tips & pointers.
After commenting the code, it became clear what it should have been. I now have it populating as it should!
Still looking at it to see if there's anything that can be improved.
Once again, cheers.
Duncan
On 8 Apr 2015 19:49, "Green 1z@compuserve.com [ExcelVBA]" < ExcelVBA@yahoogroups.com> wrote:
 
No sorrys ness Duncan... It *seems* like an interesting problem.

And don't forget that it's poss to upload a file for the group!

Lisa


No virus found in this message.
Checked by AVG - www.avg.com
Version: 2015.0.5863 / Virus Database: 4331/9541 - Release Date: 04/15/15


No virus found in this message.
Checked by AVG - www.avg.com
Version: 2015.0.5863 / Virus Database: 4334/9609 - Release Date: 04/23/15

__._,_.___

Posted by: Duncan Edment <duncan.edment@gmail.com>
Reply via web post Reply to sender Reply to group Start a New Topic Messages in this topic (19)
----------------------------------
Be sure to check out TechTrax Ezine for many, free Excel VBA articles! Go here: http://www.mousetrax.com/techtrax to enter the ezine, then search the ARCHIVES for EXCEL VBA.

----------------------------------
Visit our ExcelVBA group home page for more info and support files:
http://groups.yahoo.com/group/ExcelVBA

----------------------------------
More free tutorials and resources available at:
http://www.mousetrax.com

----------------------------------

.

__,_._,___

Tidak ada komentar:

Posting Komentar