current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I'm writing a sub which is going to determine which type of business is being detailed on a particular line of data. It's going to use column positions to check if the data exists, and then output the corresponding string if it does.

I've decided that the best way to organise a group of my arguments is using a 2D array. Now, for readability, clarity and general good coding practice, which is better:

This:

arrAmountColumns(1, 1) = lngInvestmentAmountColumn: arrAmountColumns(1, 2) = "Investment Amount"
arrAmountColumns(2, 1) = lngSinglePremiumColumn: arrAmountColumns(2, 2) = "Single Premium"
arrAmountColumns(3, 1) = lngMonthlyPremiumColumn: arrAmountColumns(3, 2) = "Monthly Premium"

or this: 

arrAmountColumns(1, 1) = lngInvestmentAmountColumn
arrAmountColumns(1, 2) = "Investment Amount"

arrAmountColumns(2, 1) = lngSinglePremiumColumn
arrAmountColumns(2, 2) = "Single Premium"

arrAmountColumns(3, 1) = lngMonthlyPremiumColumn
arrAmountColumns(3, 2) = "Monthly Premium"
share|improve this question

1 Answer 1

up vote 4 down vote accepted

I'd go with the second snippet. They're instructions, and instructions read more naturally from top to bottom, rather than from left to right.

Cramming multiple instructions into a single line is a bad habit IMO - it's not because the language supports it that it's necessarily a good idea; the array has 2 columns.. what if it were 10? Would you consider cramming 10 instructions on the same line?

Your indices seem to go (row,column), which amounts to (y,x), which is a bit counter-intuitive, and if you wanted to write that array into a worksheet range, you'd have to transpose it:

Range("A1:B" & UBound(arrAmountColumns,2)) = arrAmountColumns

wouldn't produce the expected result.

By using the first index as your "column" (X) and the second as you "row" (Y), you eliminate that ambiguity and the code would read (x,y), which is a much more intuitive way of visualizing 2D arrays.

share|improve this answer
    
ah, Those values aren't columns per se, this was just a convenient way for storing the position of certain columns in another array, along with their associated descriptors. It felt more intuitive in my head to have: .... | ColumnPositionInArray | Desciption of Column | –  Zak Aug 31 at 18:52
    
You can expect an upvote just as soon as we get Reloaded :) –  Zak Aug 31 at 18:55
    
(this is why context matters!) In that case, I'd suggest to ditch the array and use a Scripting.Dictionary instead. –  Mat's Mug♦ Aug 31 at 18:59
1  
Scripting Dictionaries (along with pivot tables) are next on my list of "Learn how to use these" just as soon as my current project's completed. –  Zak Aug 31 at 19:04
    
I've already re-written it once in the last fortnight, my boss might actually start asking when it's going to be finished if I take much longer –  Zak Aug 31 at 19:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.