TOPIC:

Bit of code:

METHOD fbLoadData(sSQL AS STRING) AS VOID
LOCAL daHistoria AS FbDataAdapter
LOCAL dsHistoria AS DataSet
LOCAL bsHistoria AS BindingSource

dsHistoria := DataSet{}

daHistoria := FbDataAdapter{sSQL, SELF:fbConn}
daHistoria:Fill(dsHistoria)

bsHistoria := BindingSource{}
bsHistoria:DataSource := dsHistoria:Tables[0]

SELF:oBindingNavigator1:BindingSource := bsHistoria
SELF:odgvHistoria:DataSource := bsHistoria


RETURN

I have one question. Is this code correct? The variables da.., ds.., bs.. are local, but I am not sure, whether they should not be the members of the class.

Jacek

Hi Jacek,

If you are asking if this code is "dangerous", then the answer is no, it is perfectly safe. Even though those object are declared as LOCALs, they will not be destroyed after the execution of the method ends, the garbage collector will keep track that they are used in members of other objects (for example in oBindingNavigator1), so they will stay "alive" until they are not used anywhere anymore.

If you are asking if this is well designed code, then in my opinion it depends on personal preferences. It's up to the programmer to decide if he wants to make those objects LOCALs or members of the CLASS. My personal opinion is that it's fine either way, although I am absolutely certain others will disagree

Chris

Without knowing the rest of the context, from my point of view I'd say it was a good way to handle the variables.

If daHistoria, dsHistoria and bsHistoria are only used so that they can eventually be assigned to the BindingSource and DataSource properties, then it seems to me that it's better that they're local. That way they can only be changed afterwards by accessing them through SELF:oBindingNavigator1:BindingSource or SELF:odgvHistoria:DataSource. Otherwise if they were class properties, you could potentially change them without realizing that this may affect your BindingSource and DataSource. As it is you'd have to consciously access them through SELF:oBindingNavigator1:BindingSource, etc., so you'd be aware of what you were doing.

Just another comment - and this one is going to open up a big debate I suspect - I'd suggest changing the way you name variables... Hungarian notation just doesn't help anymore (with good Intellisense). For example if you name daHistoria as historia_dataadapter or something like that, it becomes much more readable in your code. There are so many hundreds and thousands of types and classes in .Net that 2 or 3 letter prefixes just become confusing after a while. I find it better just to be long-winded and descriptive.

Nick

Hi Nick,

OK, let's be it myself who starts the debate!

To me, Hungarian notation is fine and very helpful, I still use it all the time as it makes my code (to my own eyes) easier to read and understand, without needing help from the editor and intelissense or needing very long var names. Actually I wish also the standard .Net libraries were in some way using Hungarian notation

But that's just me and I fully understand it if you prefer to not use Hungarian notation! I don't think that either way is better or worse, it all comes down to personal preferences and what suits everyone best.

Chris

Another point for the hungarian notation from me!

So I see immediatly the difference between

and

In some way Microsoft itself seems to use it in the "I" for the interfaces.

Wolfgang

I understand the attraction of Hungarian notation, but all I'll say is that I found it hugely liberating when I ditched it.

I no longer worry about trying to abbreviate things.... I use property and class names as long as necessary to be descriptive (auto-complete sorts it out), don't need to invent prefixes, and my code becomes easier to understand.

Give it a try.... you can always decide not to use it, but I'm betting that if you try it for a while you'll never look back.

Nick

Hi Nick,

Oh, I tried it! Actually when I was implementing new things, or made fixes to the vulcan runtime, I wasn't using Hungarian notation (at least not in public members), because the code would be used/maintained also by people who are not used to using Hungarian notation. But when writing code for my own personal stuff, I felt liberated that I was able to use it again! Again, a matter of personal preferences..

Chris

Why do you connect "hungarian" with "shortening"? Not really related, imho.
So, i for myself will use cVar happily as i will cThisStringsContendshouldnotbeshortend

You're quite right, there's no direct relation, but I brought it up because in the original example the naming was eg. daHistoria where da is short for DataAdapter.

A lot of people will use oHistoria, but that doesn't really help as Object could be just about anything... if you then change it to oHistoriaDataAdapter, well the o is now superfluous.

As Chris says, it's down to personal preference, just giving my take on it.

Nick

Hi Nick,

I tend to agree with most/all of your sentiments.

After years of working with people - the general public - and reading endless News Group postings and web articles, it seems to me that 'folk just don't like CHANGE'. "Why have new when the old still works?"

However, there are a few people who seem to seek out change, thrive on it perhaps, but also want change just for the sake of it - good or bad. These can be just as misdirected ;-0)

Strange old world !
Regards,
Phil.

Hi Jacek,

in additiopn to the comments on Var naming, I would refactor to more story telling method names. It feels like working with a hammer, but you will like it later when rereading.

The first step is: init LOCAL's when just before needed:

METHOD fbLoadData(sSQL AS STRING) AS VOID

LOCAL dsHistoria AS DataSet
dsHistoria := DataSet{}

LOCAL daHistoria AS FbDataAdapter
daHistoria := FbDataAdapter{sSQL, SELF:fbConn}
daHistoria:Fill(dsHistoria)

LOCAL bsHistoria AS BindingSource
bsHistoria := BindingSource{}
bsHistoria:DataSource := dsHistoria:Tables[0]

SELF:oBindingNavigator1:BindingSource := bsHistoria
SELF:odgvHistoria:DataSource := bsHistoria

This way you clearly see when and why they are needed.

Then replace all! instantiation of objects/classes with factory method's.

You may put additional logic in and do method names like comments.
Try to avoid class names in var names. It's not easy here.

You see: the method name say, this method is responsible to two things: loading data and binding to GUI. It has an 'and' in it's name. That's too much. Loading data and connecting to GUI are two completly different things. Do this in two decent steps.

For now I think this shows better what you are doing. It has short method's, is testable, story telling methods and meaningful names.

Just m 2 ct's

Frank

METHOD LoadHistoriaAndBindToGUI( HistoriaDataBaseName AS STRING) AS VOID

LOCAL HistoriaData AS DataSet
HistoriaData := GetDataSet() // Never ever instantiate objects directly !!!

FillHistoriaWithData( HistoriaDataBaseName, HistoriaData )

LOCAL BindingSourceToHistoriaFirstTable AS BindingSource
BindingSourceToHistoriaFirstTable := CreateBindingSourceToHistoriaFirstTable( HistoriaData )

SetBindingToNavigatorAndDataGrid( BindingToHistoriaFirstTable )

INTERNAL METHOD GetDataSet() AS DataSet // Factory Method
RETURN DataSet{}

INTERNAL METHOD FillHistoriaWithData( HistoriaDataBaseName AS STRING, HistoriaDataSet AS DataSet ) AS VOID
LOCAL HistoriaDataAdapter AS FbDataAdapter
HistoriaDataAdapter := FbDataAdapter{ HistoriaDataBaseName AS STRING, SELF:fbConn AS ???}
HistoriaDataAdapter :Fill(HistoriaDataSet )

INTERNAL METHOD CreateBindingToHistoriaFirstTable( HistoriaDataSet AS DataSet ) AS VOID
BindingToHistoriaFirstTable := BindingSource{}
BindingToHistoriaFirstTable :DataSource := HistoriaDataSet :Tables[0]

INTERNAL METHOD SetBindingToNavigatorAndDataGrid( BindingToHistoriaFirstTable AS BindingSource) AS VOID
SELF:HistoriaBindingNavigator:BindingSource := BindingToHistoriaFirstTable
SELF:HistoriaDataGrid:DataSource := BindingToHistoriaFirstTable