Finishing Up

I m hoping that we can finish up the file operations, in rudimentary but reliable condition, in one more phase. We need to get the dialogs to open up, and I m thinking that instead of providing the file name, or in addition to providing the file name , we need to set a private flag that suppresses the dialogs for testing purposes. We ll see how that goes. Now that the tests are running, I ll proceed by putting in the dialogs and see whether that implies changes to the tests. I believe it will.

I was thinking also about the cursor-setting trick we saw earlier. What is clearly happening is that the selection is left where it was before the file load. We should change the file load to set it explicitly, probably to zero. We ll keep that in mind as we go forward. Here s the new MenuFileOpenOnClick method:

 void MenuFileOpenOnClick(object obj, EventArgs ea) { 
 OpenFileDialog dialog = new OpenFileDialog(); 
 dialog.Filter = "xml files (*.xml)*.xmlAll files (*.*)*.*"; 
 dialog.FilterIndex = 1; 
 dialog.RestoreDirectory = true ; 
 if(dialog.ShowDialog() == DialogResult.OK) { 
 fileName = dialog.FileName; 
 CallModel(loadAction); 
 } 
 } 

This just brings up an OpenFileDialog and opens the file if you hit OK. I set the FilterIndex to 1 (which is for some reason the first entry in the list) to make it default to .xml files. This code works in the XML Notepad and reads the file in correctly. However, I expect that the tests will not run correctly now. I expect them to open this dialog. If I give it the right file, however, it might still work. Let s run the tests and see. Yes, that s exactly what happens. If I give it customertestxmlfile.xml, the test runs, and if I give it some other file, it does not. Now to change things so that the dialog doesn t come up if we re testing. I m planning to do it with a testing flag in the Form. A better idea might exist, but none comes to mind right now and this should work just fine. In CustomerTest.cs, we add these lines:

 private void SaveFile() { 
 form.SetFileName("customertestxmlfile.xml"); 
  form.SetNoFileDialog();  
   ExecuteMenu("^S"); 
 } 
 private void LoadFile() { 
 form.SetFileName("customertestxmlfile.xml"); 
  form.SetNoFileDialog();  
 ExecuteMenu("^O"); 
 } 

And we implement the method, with a private Boolean flag, and use the flag, as follows :

 void MenuFileOpenOnClick(object obj, EventArgs ea) { 
 if (displayDialog) { 
 OpenFileDialog dialog = new OpenFileDialog(); 
 dialog.Filter = "xml files (*.xml)*.xmlAll files (*.*)*.*"; 
 dialog.FilterIndex = 1; 
 dialog.RestoreDirectory = true ; 
 if(dialog.ShowDialog() == DialogResult.OK) { 
 fileName = dialog.FileName; 
 CallModel(loadAction); 
 } 
 } 
 else { 
 CallModel(loadAction); 
 } 
 displayDialog = true; 
 } 
 
 void MenuFileSaveAsOnClick(object obj, EventArgs ea) { 
 if (displayDialog) { 
 SaveFileDialog dialog = new SaveFileDialog(); 
 dialog.Filter = "xml files (*.xml)*.xmlAll files (*.*)*.*"; 
 dialog.FilterIndex = 1; 
 dialog.RestoreDirectory = true ; 
 if(dialog.ShowDialog() == DialogResult.OK) { 
 fileName = dialog.FileName; 
 CallModel(saveAction); 
 } 
 } 
 else { 
 CallModel(saveAction); 
 } 
 displayDialog = true; 
 } 

Note that I m unconditionally setting the displayDialog flag back to true after each use of it. That s to make sure that the Form doesn t get stuck in test mode somehow. Also, I notice some duplication in this code, and I m inclined to get rid of it if possible. Removing the duplicate CallModel from each of those methods seems unlikely to work well, as it would require a complex conditional, checking the displayDialog flag again and the DialogResult. Is there some way to combine the two methods entirely? It looks like there is. The code is all the same except for the initial dialog setup. Let s extract a method:

 void MenuFileSaveAsOnClick(object obj, EventArgs ea) { 
 if (displayDialog) { 
 SaveFileDialog dialog = new SaveFileDialog(); 
  FileAction(dialog, saveAction);  
   } 
 else { 
 CallModel(saveAction); 
 } 
 displayDialog = true; 
 } 
  private void FileAction(SaveFileDialog dialog, ModelAction action) {  
  dialog.Filter = "xml files (*.xml)*.xmlAll files (*.*)*.*";  
  dialog.FilterIndex = 2 ;  
  .RestoreDirectory = true ;  
  if(dialog.ShowDialog() == DialogResult.OK) {  
  fileName = dialog.FileName;  
  CallModel(action);  
  }  
  }  

This works fine. Now we can change the other method similarly:

 void MenuFileOpenOnClick(object obj, EventArgs ea) { 
 if (displayDialog) { 
 OpenFileDialog dialog = new OpenFileDialog(); 
  FileAction(dialog, loadAction);  
 } 
 else { 
 CallModel(loadAction); 
 } 
 displayDialog = true; 
 } 

That doesn t compile, because the FileAction method expects a SaveFileDialog. We ll use the common abstract superclass, FileDialog:

 private void FileAction(  FileDialog dialog  , ModelAction action) { 
 dialog.Filter = "xml files (*.xml)*.xmlAll files (*.*)*.*"; 
 dialog.FilterIndex = 2 ; 
 dialog.RestoreDirectory = true ; 
 if(dialog.ShowDialog() == DialogResult.OK) { 
 fileName = dialog.FileName; 
 CallModel(action); 
 } 
 } 

That works, but the two methods, MenuFileSaveAsOnClick and MenuFileOpenOnClick, still look too similar. Look at them side by side:

 void MenuFileOpenOnClick(object obj, EventArgs ea) { 
 if (displayDialog) { 
 OpenFileDialog dialog = new OpenFileDialog(); 
 FileAction(dialog, loadAction); 
 } 
 else { 
 CallModel(loadAction); 
 } 
 displayDialog = true; 
 } 
 void MenuFileSaveAsOnClick(object obj, EventArgs ea) { 
 if (displayDialog) { 
 SaveFileDialog dialog = new SaveFileDialog(); 
 FileAction(dialog, saveAction); 
 } 
 else { 
 CallModel(saveAction); 
 } 
 displayDialog = true; 
 } 

Let s extract all that as a method. We ll wind up creating the dialog unconditionally, but I m not deeply concerned about that because in the normal running of the program, we always do anyway. Here s the extraction, in both places:

 void MenuFileOpenOnClick(object obj, EventArgs ea) { 
 FileOperation(new OpenFileDialog(), loadAction); 
 } 
 void MenuFileSaveAsOnClick(object obj, EventArgs ea) { 
 FileOperation(new SaveFileDialog(), saveAction); 
 } 
 private void FileOperation(FileDialog dialog, ModelAction action) { 
 if (displayDialog) { 
 FileAction(dialog, action); 
 } 
 else { 
 CallModel(action); 
 } 
 displayDialog = true; 
 } 
 private void FileAction(FileDialog dialog, ModelAction action) { 
 dialog.Filter = "xml files (*.xml)*.xmlAll files (*.*)*.*"; 
 dialog.FilterIndex = 2 ; 
 dialog.RestoreDirectory = true ; 
 if(dialog.ShowDialog() == DialogResult.OK) { 
 fileName = dialog.FileName; 
 CallModel(action); 
 } 
 } 

That s pretty good. I m inclined to rename the FileAction method to DialogFileAction and then call it a night.

Lesson  

One of my technical editors was particularly fond of the refactoring above, pointing out that it is rare to see code that talks to GUI components turn out to be well- factored . I share the observation that it s rare, and looking at it now, it did turn out rather nice. At the time I just went after it because I observed the duplication and it seemed like a good idea to hammer home the notion of eliminating duplication wherever we find it.

Removing duplication almost always produces very nice code, and it has the benefit of putting each decision in one place, for easy modification later. If, as sometimes happens, a later change undoes the convergence, so be it. It happens less often than you might think, and the code, even as changed, is usually better than what you started with.