backupfilestest.fa

Clone Tools
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
JAL-3210 Barebones gradle/buildship/eclipse. See README

  1. … 1990 more files in changeset.
Test file relocated to test/jalview/io.

Test file relocated to test/jalview/io.

JAL-3141 test file in test folder not examples

  1. … 2 more files in changeset.
May be worth adding temp.deleteOnExit() in case anything goes wrong with renaming later

May be worth adding temp.deleteOnExit() in case anything goes wrong with renaming later

Needs Javadoc; in particular to highlight the side-effect (that a file is written)

Needs Javadoc; in particular to highlight the side-effect (that a file is written)

Use '.' (test/jalview/io) rather than examples (which should be kept for real user data files). Or better if possible, use File.createTempFile to save files to a system-managed temporary folder.

Use '.' (test/jalview/io) rather than examples (which should be kept for real user data files).
Or better if possible, use File.createTempFile to save files to a system-managed temporary folder.

exampleLabel (and label.suffix_example_filenames) removed (had been taken out to save vertical space in the Preferences pane).

exampleLabel (and label.suffix_example_filenames) removed (had been taken out to save vertical space in the Preferences pane).

Is exampleLabel needed? Doesn't seem to do anything.

Is exampleLabel needed? Doesn't seem to do anything.

In BackupFiles it started off as (and remains) simply a boolean flag. Being able to convey that in the user preferences would be too terse so I agree that I wanted to expand that to look like a bin...

In BackupFiles it started off as (and remains) simply a boolean flag. Being able to convey that in the user preferences would be too terse so I agree that I wanted to expand that to look like a binary choice. I still wanted it to behave like a boolean flag though to keep the code readable in GPrefences. Also I knew I wanted to do this more than once, and I suspect that if I'm adding anything similar to Preferences in the future I'll want to use it again, it just seemed tidier to bundle it off, with checks and balances, into another class. If I ever have to change those checks and balances I'll be glad it's in a separate class and not spread around GPreferences in multiple places!

Ah I see, yes that is different behaviour. Though your class is still just wrapping a simple test like if (trueButton.isSelected()) I still think this is not so much a boolean as a binary choice ...

Ah I see, yes that is different behaviour.
Though your class is still just wrapping a simple test like

if (trueButton.isSelected()) 

I still think this is not so much a boolean as a binary choice (both options 'mean' something).
And that doing the exact same thing with two radio buttons directly would be simpler.

I'm not sure this is the same. jalviewBooleanRadioButtons.isSelected() should return true only if buttonTrue is selected and buttonFalse isn't selected. It will return false if both are selected (w...

I'm not sure this is the same. jalviewBooleanRadioButtons.isSelected() should return true only if buttonTrue is selected and buttonFalse isn't selected. It will return false if both are selected (which is not possible[!] but would be ambiguous), or neither are selected (possible but also ambiguous), or crucially if buttonFalse is selected (unambiguously return false). The two expected scenarios are one xor other are selected, with isSelected() returning either true or false. I assume buttonGroup.getSelection() != null will return true if buttonFalse is selected (sorry not had time to check this in reality)? This is how I was wanting the two button radio group to behave like a checkbox but be more visually explicit in the user preferences.

I don't really like creating work for myself, but my experience (in a different arena, so might not apply here) is that scope-creep is common and future features get asked for a couple of years dow...

I don't really like creating work for myself, but my experience (in a different arena, so might not apply here) is that scope-creep is common and future features get asked for a couple of years down the line, even when they've been specifically ruled out at the design stage! My over-spec compensatory coding stems from that experience, but I'm happy to believe that occurs less here. I guess I also think a method named "lsBackupFiles" should definitely have the ability to return a list! Now that's changed I guess I don't have to!

Golden rule (for me, but not just for me) is not to write code you don't need (and refactor if needed in the future). Why not just have a getBackupFiles that returns TreeMap, then Groovy is free to...

Golden rule (for me, but not just for me) is not to write code you don't need (and refactor if needed in the future).
Why not just have a getBackupFiles that returns TreeMap, then Groovy is free to use as it wishes e.g. files.values().iterator().
More flexible than guessing what view of the files might be wanted in future.

addTrueActionListener can be removed without changing behaviour. addFalseActionListener isn't used. Don't need either.

addTrueActionListener can be removed without changing behaviour.
addFalseActionListener isn't used.
Don't need either.

Or buttonGroup.getSelection() != null (the ButtonGroup is what 'knows' whether anything is selected)

Or buttonGroup.getSelection() != null (the ButtonGroup is what 'knows' whether anything is selected)

I still need convincing. Nothing to stop you adding the six radio buttons directly (in three ButtonGroups) to achieve the same result. Avoids the need for getTrueButton, getFalseButton.

I still need convincing. Nothing to stop you adding the six radio buttons directly (in three ButtonGroups) to achieve the same result. Avoids the need for getTrueButton, getFalseButton.

Yes! This is the main function, good plan to make it more readable https://source.jalview.org/crucible/static/ogdo0b/2static/images/wiki/icons/emoticons/smile.gif Have extracted two bits of it.

Yes! This is the main function, good plan to make it more readable Have extracted two bits of it.

Made some changes from the code review, and attempted to defend the other coding decisions!

Made some changes from the code review, and attempted to defend the other coding decisions!

This is being used for what are really boolean options. I didn't think that a simple checkbox (with label) could explain well enough the purpose of the boolean flag, so wanted to have two clear opt...

This is being used for what are really boolean options. I didn't think that a simple checkbox (with label) could explain well enough the purpose of the boolean flag, so wanted to have two clear options for the user to choose from. I'd initially started putting ButtonGroups in Preferences.java and setting the boolean from there depending but it was heading towards looking messy and unreadable. Knowing I wanted to do this with at least two of the boolean user options (and I suspect I will want to do the same again in the future if adding to the Preferences window), I decided to spin it off as a self-contained class.

You are right it could now be done like this. I'd initially thought it would be useful to retain access to this flag so that success or failure could be easily referred to later than just at the po...

You are right it could now be done like this. I'd initially thought it would be useful to retain access to this flag so that success or failure could be easily referred to later than just at the point of calling rollBackupsAndRenameTempFile()... but it turns out this hasn't been used/needed yet. I still have a (non-quantitative!) "feeling" it might be useful to retain the flag rather than just a transient parameter return.

Not used by anything yet. Thought it might be useful to leave in (it's a more obvious return than a TreeMap) after seeing power of the groovy scripting. ??

Not used by anything yet. Thought it might be useful to leave in (it's a more obvious return than a TreeMap) after seeing power of the groovy scripting. ??

Avoid mixing TestNG and Junit (use TestNG)

Avoid mixing TestNG and Junit (use TestNG)

Or just have a public constant and delete this method

Or just have a public constant and delete this method

Can this be refactored into shorter methods? e.g. extract method for confirmDeletion() etc

Can this be refactored into shorter methods? e.g. extract method for confirmDeletion() etc

Use interface (Map rather than HashMap) for variable type (also at line 512)

Use interface (Map rather than HashMap) for variable type (also at line 512)

Could be package visibility. I find method name confusing (looks like 'isBackupFiles...'), could it be 'getBackupFiles...'?

Could be package visibility.
I find method name confusing (looks like 'isBackupFiles...'), could it be 'getBackupFiles...'?

Is this needed? It is only called by the test class. Just call lsBackupFilesAsTreeMap instead.

Is this needed? It is only called by the test class. Just call lsBackupFilesAsTreeMap instead.

Shouldn't have more than one top level class in one source file

Shouldn't have more than one top level class in one source file

Simpler - remove this method / flag, pass 'success' as a boolean parameter when calling rollBackupsAndRenameTempFile()

Simpler - remove this method / flag, pass 'success' as a boolean parameter when calling rollBackupsAndRenameTempFile()

Not sure of the need for this class. Can't it just be done using a ButtonGroup in 'the usual way'?

Not sure of the need for this class. Can't it just be done using a ButtonGroup in 'the usual way'?