Update member variable names to avoid shadowing

[oruebel]

Fix #60
Fix #9
Fix #106

• Container
• Data
• NWBFile
• RecordingContainers
• Channel
• VectorData
• ElementIdentifiers (noop)
• HDF5RecordingData
• HDF5IO
• BaseIO
• DynamicTable

To be fixed in read I/O PR #85

• ElectrodeTable
• ElectrodeGroup
• SpikeEventSeries
• ElectricalSeries
• Device
• TimeSeries (already fixed in Add data read #85)

[oruebel]

@stephprince This is a first pass at fixing the naming conventions for member variables to avoid shadowing of names. For the main NWB data types, I propose that I fix those on the read PR #85 since I'll need to modify those members for read anyways and modifying them here as well will only lead to tons of merge conflicts.

[stephprince]

I propose that I fix those on the read PR #85 since I'll need to modify those members for read anyways

Sounds good.

As I'm looking through I had a question. I thought m_ was supposed to replace this-> but in some places both are being used. I wasn't sure if there is a reason to use both of them?

[stephprince]

Added a couple comments/questions! Mainly suggestions to use m_ instead of both this-> and m_ since I think they are redundant? If there is a reason to use both feel free to ignore those.

[oruebel]

Mainly suggestions to use m_ instead of both this-> and m_ since I think they are redundant? If there is a reason to use both feel free to ignore those.

In this case, yes, this->m_ and m_ access the same variable. this is a pointer to the current object of the class so it makes it clear that we are using the object and m_ makes it clear that it is a class member variable. We can just use m_ in most places. I've fixed this in ad7aea4 to replace this->m_ with just m_ throughout.

[oruebel]

@stephprince can you please re-review. I moved some of the simple setter functions.

[stephprince]

Looks good to me!

[oruebel]

@stephprince is it expected that the validate tests are failing or is this new in this PR. Just wanted to check if this is something I need to fix here or whether this is something for a separate PR.

[oruebel]

Ok, comparing the test files generated by the main branch and this branch it looks like something broke with writing the colnames for DynamicTable and/or ElectrodeTable

[oruebel]

@stephprince can you re-review. All tests are passing now.

Turns out, the issue with the invalid colnames in the created NWB files was due to #106 . I had missed that ElectrodeTable defined its own ElectordeTable.colNames which was hiding the inherited member DynamicTable.colNames and because I had only renamed one of them, it was using the empty colNames from DynamicTable instead of from ElectrodeTable.

[stephprince]

ah ok. So from what I understand, #106 is resolved but is a behavior to keep an eye out for in the future

[oruebel]

So from what I understand, #106 is resolved but is a behavior to keep an eye out for in the future

Correct. At least I hope I caught all the places where this is happening.