4 ms·

What problems do people see in this code? The hard-coding of the sql statements that differ by only by table name? The code repetition of the while loops? Some

by gcheong 18y ago

What problems do people see in this code? The hard-coding of the sql statements that differ by only by table name? The code repetition of the while loops? Something else? Just interested in seeing if my understanding and analysis of it agrees with what others think. Edit: One more - the "select *" when only the "name" is used?

jodrellblank 18y ago
I see... no variables defined before use (optional). Pet hate: VB fakehungarian names where objects are called oSomething. In the oRS.open line, the "2, 2" are magic numbers - I have no idea what they do without looking them up. (Turns out they are CursorType adOpenDynamic and LockType adLockPessimistic). Since they aren't the defaults, presumably they are necessary, but from the snippet given the defaults might be OK. The presence of database tables Members and Members2 with (in this snippet) no difference between them. If you don't need two, merge them. If you need two, name them more helpfully. If the data will only be in one or the other, could you use a union query? Why might the data be in one or the other and why not use that information directly to decide which to query instead of querying both? What if there are no names in either table? As you say, selecting * but only using one field, and the repetition within the if branches. Unhelpfully named array "myarr" Redimensioning of the array every time through the loop. But most importantly, all the body of the code does is take the names from the recordset and put them in an array. I expect (hope) there is a shorter, more efficient, more idiomatic way to copy a recordset into an array. See other HN topic about the fun, easy and addictive nature of criticism. I shouldn't really - at least they have a product with members and money to employ people to code for them and enough success that their code stayed around long enough to become old and maintainable... and there's plenty of people who'll argue that make it work, ship it, then if necessary rewrite it is much prefereable to make it beautiful, perfect it, tweak it, file for bankruptcy.

by gcheong 18y ago

jodrellblank 18y ago

I see... no variables defined before use (optional). Pet hate: VB fakehungarian names where objects are called oSomething. In the oRS.open line, the "2, 2" are magic numbers - I have no idea what they do without looking them up. (Turns out they are CursorType adOpenDynamic and LockType adLockPessimistic). Since they aren't the defaults, presumably they are necessary, but from the snippet given the defaults might be OK. The presence of database tables Members and Members2 with (in this snippet) no difference between them. If you don't need two, merge them. If you need two, name them more helpfully. If the data will only be in one or the other, could you use a union query? Why might the data be in one or the other and why not use that information directly to decide which to query instead of querying both? What if there are no names in either table? As you say, selecting * but only using one field, and the repetition within the if branches. Unhelpfully named array "myarr" Redimensioning of the array every time through the loop. But most importantly, all the body of the code does is take the names from the recordset and put them in an array. I expect (hope) there is a shorter, more efficient, more idiomatic way to copy a recordset into an array. See other HN topic about the fun, easy and addictive nature of criticism. I shouldn't really - at least they have a product with members and money to employ people to code for them and enough success that their code stayed around long enough to become old and maintainable... and there's plenty of people who'll argue that make it work, ship it, then if necessary rewrite it is much prefereable to make it beautiful, perfect it, tweak it, file for bankruptcy.