# Want to remove the goto in my code

I want to remove my goto's in this code but I'm not sure how. There must be a cleaner way to write my code. Perhaps move the "if" statement to the end of each loop then use break?

Sorry if this is a stupid question, I'm self-taught, and I'm finishing up my first sizable program. Any suggestions as to how I can clean my code is welcomed, even though it isn't stated in my question.

Also this is just my beta release, so please forgive all the sillyness.

    public void BtnRandomizeClick(object sender, EventArgs e)
{
Nowthatitssaved:   //Goto marker -- Loops the saved/not saved stuff based on file dialog
switch (MessageBox.Show(@"Did you SAVE your edited list?", @"Just checking... It's important.",
MessageBoxButtons.YesNo))
{
#region Dialog Yes

case DialogResult.Yes:
if (FileChosen == null)
{
MessageBox.Show(
@"Please select a file to load before trying to randomize. You need some entries to randomize first eh?",
"Oh fiddlesticks...");
return;
}
dataGridAssociates.Rows.Clear();
var masterList = new List<Associate>();
var divertsList = new List<Associate>();
var mheList = new List<Associate>();
var lines = File.ReadAllLines(FileChosen); //Array of all the lines in the text file

foreach (var assocStringer in lines)
{
if (assocStringer == null) continue;
var entries = assocStringer.Split('|');
var obj = (Associate) _bindingSource.AddNew();
if (obj == null) continue;
if (entries.Length < 7) break;
obj.FirstName = entries[0];
obj.LastName = entries[1];
obj.AssocId = entries[2];
obj.AssocRfid = entries[3];
obj.CanDoDiverts = Boolean.Parse(entries[4]);
obj.CanDoMhe = Boolean.Parse(entries[5]);
obj.UniqueRandomID = Guid.NewGuid().ToString();
}
var sortedMaster = masterList.OrderBy(d => d.UniqueRandomID).ToList();
var loaderCountRequested = 0;
var mheCountRequested = 0;
var divertCountRequested = 0;
var validDivertEntry = int.TryParse(txtDivertWorkers.Text, out divertCountRequested);
var validMHEEntry = int.TryParse(txtMHEWorkers.Text, out mheCountRequested);

//See if whomever used the inputs correctly
if (validDivertEntry == false)
{
MessageBox.Show(@"Please use an integer in the 'Divert Workers' textbox and try again");
return;
}
if (validMHEEntry == false)
{
MessageBox.Show(@"Please use an integer in the 'MHE Workers' textbox and try again");
return;
}
if (validLoaderEntry == false)
{
MessageBox.Show(@"Please use an integer in the 'Loaders' textbox and try again");
return;
}

var canDoDivertsQuery = sortedMaster.Where(a => a.CanDoDiverts);
var canDoMHEQuery = sortedMaster.Where(b => b.CanDoMhe);
var loaders = 0;
var mhes = 0;
var diverters = 0;

//Begin the sorting method
{
goto MHEKTHX;

}
MHEKTHX:

foreach (Associate someAssocVar in canDoMHEQuery)
{
if (mhes == mheCountRequested)
goto DivertsKTHX;
mhes++;
listBoxMHE.Items.Add(mheList[mhes - 1].FirstName + " " +
mheList[mhes - 1].LastName);
}

DivertsKTHX:
sortedMaster.RemoveAll(mheList.Contains);

foreach (Associate someAssocVar in canDoDivertsQuery)
{
if (diverters == divertCountRequested)
goto DoneKTHX;
diverters++;
listBoxDiverts.Items.Add(divertsList[diverters - 1].FirstName + " " +
divertsList[diverters - 1].LastName);
}

DoneKTHX:
sortedMaster.RemoveAll(divertsList.Contains);

MessageBox.Show(@"You have " + sortedMaster.Count() + @" associates unassigned.");
break;
#endregion

#region Dialog No

case DialogResult.No:
if (MessageBox.Show(@"Would you like to save your work then?", @"It really does matter...",
MessageBoxButtons.YesNo) == DialogResult.Yes)
{
var saveDialog = new SaveFileDialog
{
InitialDirectory =
Convert.ToString(Environment.SpecialFolder.MyDocuments),
// ReSharper disable LocalizableElement
Filter = "Text (*.txt)|*.txt|All Files (*.*)|*.*",
// ReSharper restore LocalizableElement
FilterIndex = 1
};
if (saveDialog.ShowDialog() == DialogResult.OK)
{
FileChosen = saveDialog.FileName;
var assocToText = DatagridToString(dataGridAssociates, '|');
using (var outfile = new StreamWriter(saveDialog.FileName))
outfile.Write(assocToText);
}
goto Nowthatitssaved;
}
break;

#endregion
}
}

-
In this particular case, when labels are directly after the loops, each of the gotos can be replaced by a regular break from the foreach loop. I'm not posting this as an answer 'cause IMHO it does not really cover the topic fully (the code could probably use some refactoring/rewrite) and I can't really do much more from the phone ;) –  Patryk Ćwiek Jan 13 '13 at 21:41
Dude, your conditions seem to be independent of the loop variables, so you can have 4 if statements. After that you can have 4 different methods. –  Leonid Jan 13 '13 at 22:57
You will get a lot of feedback. I think you accepted an answer too soon. Reminding the user to save a list is a bad idea. You should either auto-save it or disable the Randomize button. Then use an assertion if you must. –  Leonid Jan 14 '13 at 4:32

Here's a variety of refactorings. I didn't start off intending to change the code this much, but I figure I might as well post it now...

You say "I think using methods is something I'm missing from my whole program...", and I agree. Extracting out code into methods is the single best thing you can do to improve this. You will make your code more structured and easier to understand, and be able to remove any redundancies.

Now some individual items:

• Firstly, the Random ID on the Associate class appears to only be needed to shuffle the list. It is not an intrinsic part of the Associate, so it doesn't belong there. Remove it and shuffle the list directly.

• One thing that often helps is to always declare your variables as late as possible. This tends to reveal the structure of your code better. Let's do that with the sorting part of the code:

var loadingList = new List<Associate>();
var loaders = 0;
{
break;

}

// one removed to save space

var divertsList = new List<Associate>();
var canDoDivertsQuery = masterList.Where(a => a.CanDoDiverts);
var diverters = 0;
foreach (Associate someAssocVar in canDoDivertsQuery)
{
if (diverters == divertCountRequested)
break;
diverters++;
listBoxDiverts.Items.Add(divertsList[diverters - 1].FirstName + " " +
divertsList[diverters - 1].LastName);
}
masterList.RemoveAll(divertsList.Contains);


Notice that they all have the same structure. This should indicate to you that this is common code that can be shared. Let's extract one instance into a method:

private List<Associate> AssignLoaders(List<Associate> masterList, int loaderCountRequested)
{
var loaders = 0;
{
break;
}
}


This is called like so:

var loadingList = AssignLoaders(masterList, loaderCountRequested);


Now how can we make this more generic so that we can share it with the other two uses? We need to be able to pass in the condition. With LINQ we can use a Func<Associate, bool> - that is, a function which takes an associate and returns a bool. You already have some of these in your code (the c => c.CanDoLoading in the Where calls).

Let's make the condition a parameter:

private List<Associate> AssignLoaders(List<Associate> masterList, Func<Associate, bool> filter, int loaderCountRequested)
{
var loaders = 0;
{
break;
}
}


This is called like this:

var loadingList = AssignLoaders(masterList, c => c.CanDoLoading, loaderCountRequested);


Now the AssignLoaders knows almost nothing about whether it is looking for loaders, so we can use it for the other types as well. Let's rename the variables to indicate that, then tidy it up (with a little LINQ) and make it able to assign to any destination listbox:

private List<Associate> AssignAssociates(IEnumerable<Associate> associates, Func<Associate, bool> filter, int numberRequested, ListBox destination)
{
var assignedAssociates = new List<Associate>();
foreach (var associate in assignedAssociates)
{
destination.Items.Add(associate.FirstName + " " + associate.LastName);
}
return assignedAssociates;
}


The listbox stuff doesn't really fit right here, it looks a bit ugly. Let's split it into two separate methods:

private void AddAssociatesToListBox(List<Associate> associates, ListBox listBox)
{
foreach (var associate in associates)
{
listBox.Items.Add(associate.FirstName + " " + associate.LastName);
}
}

private List<Associate> AssignAssociates(IEnumerable<Associate> associates, Func<Associate, bool> filter, int numberRequested)
{
return associates.Where(filter).Take(numberRequested).ToList();
}


Now the sorting code looks like:

var loadingList = AssignAssociates(masterList, c => c.CanDoLoading, loaderCountRequested);

// one skipped to save space

var divertsList = AssignAssociates(masterList, c => c.CanDoDiverts, divertCountRequested);
masterList.RemoveAll(divertsList.Contains);


Additionally, if you give the Associate class a ToString override then AddAssociatesToListBox becomes irrelevant (delete it!) and you can just write:

var divertsList = AssignAssociates(masterList, c => c.CanDoDiverts, divertCountRequested);
listBoxDiverts.DataSource = divertsList;
masterList.RemoveAll(divertsList.Contains);

• We can do a similar refactoring to this previous one with the validation code:

int loaderCountRequested;


And the extracted method:

private bool CountIsValid(TextBox textBox, string textBoxName, out int countRequested)
{
var validEntry = int.TryParse(textBox.Text, out countRequested);
if (validEntry == false)
{
MessageBox.Show(string.Format(@"Please use an integer in the '{0}' textbox and try again.", textBoxName));
}
return validEntry;
}


I'd actually recommend you look at using one of the built-in validation mechanisms, but I'm not very well versed in Windows Forms. Maybe someone can point the way in a comment.

• For your main method, I don't think you need to reload the file each time. It makes sense to randomize what is currently in the data grid. As long as you don't clear it your user won't lose any data.

This actually means that your complicated goto logic simply disappears as it is not needed. (You will see this in the code I post below.)

• Separate out the loading and saving code. This makes it a bit cleaner and easier to see what is going on, and allows us to do other things more easily, like:

• Track the state of the data. By checking if the user has modified the data, you can be smarter about asking them when to save their changes. This can be done by attaching to an event on the grid. I've called the variable that tracks this _unsavedChanges. It is also reset when the user loads or saves data.

Here are my modifications in toto. I've guessed at the implementation of DatagridToString but it seems to work:

public partial class Main : Form
{
public Main()
{
InitializeComponent();

_unsavedChanges = false;
}

private string _currentFile;


First, the randomization method and its minions:

    void BtnRandomizeClick(object sender, EventArgs e)
{
if (_currentFile == null)
{
MessageBox.Show(@"Please select a file to load before trying to randomize. You need some entries to randomize first eh?",
"Oh fiddlesticks...");
return;
}

// Get a list of all associates
var masterList = _bindingSource.OfType<Associate>().ToList();
masterList.Shuffle(new Random());

//See if whomever used the inputs correctly
int divertCountRequested;
if (!CountIsValid(txtDivertWorkers, "Divert Workers", out divertCountRequested)) return;

int mheCountRequested;
if (!CountIsValid(txtMHEWorkers, "MHE Workers", out mheCountRequested)) return;

//Begin the sorting method

var mheList = AssignAssociates(masterList, c => c.CanDoMhe, mheCountRequested);
listBoxMHE.DataSource = mheList;

var divertsList = AssignAssociates(masterList, c => c.CanDoDiverts, divertCountRequested);
listBoxDiverts.DataSource = divertsList;
masterList.RemoveAll(divertsList.Contains);

MessageBox.Show(@"You have " + masterList.Count() + @" associates unassigned.");
}

private bool CountIsValid(TextBox textBox, string textBoxName, out int countRequested)
{
var validEntry = int.TryParse(textBox.Text, out countRequested);
if (validEntry == false)
{
MessageBox.Show(string.Format(@"Please use an integer in the '{0}' textbox and try again.", textBoxName));
}
return validEntry;
}

private List<Associate> AssignAssociates(IEnumerable<Associate> associates, Func<Associate, bool> filter, int numberRequested)
{
return associates.Where(filter).Take(numberRequested).ToList();
}


Now, code that loads and saves the files:

    private bool _unsavedChanges;

private bool CheckForUnsavedChanges()
{
if (_unsavedChanges)
{
if (MessageBox.Show(@"Would you like to SAVE your edited list?", @"Save your changes?", MessageBoxButtons.YesNo) == DialogResult.Yes)
{
if (!PromptToSaveFile())
return false; // the user cancelled
}
}

return true;
}

private void LoadFile(string filename)
{
_bindingSource.Clear(); // remove any old entries

var lines = File.ReadAllLines(filename); //Array of all the lines in the text file
foreach (var assocStringer in lines)
{
if (assocStringer == null) continue;
var entries = assocStringer.Split('|');
var obj = (Associate) _bindingSource.AddNew();
if (obj == null) continue;
if (entries.Length < 7) break;
obj.FirstName = entries[0];
obj.LastName = entries[1];
obj.AssocId = entries[2];
obj.AssocRfid = entries[3];
obj.CanDoDiverts = Boolean.Parse(entries[4]);
obj.CanDoMhe = Boolean.Parse(entries[5]);
}
_unsavedChanges = false;
}

string DatagridToString(DataGridView dataGrid, char joiner)
{
var join = joiner.ToString();
var sb = new StringBuilder();
foreach (DataGridViewRow row in dataGrid.Rows)
{
var cells = row.Cells.Cast<DataGridViewCell>();
if (cells.Any(cell => cell.Value == null)) // for now, skip any lines with null cells
continue;

sb.AppendLine(string.Join(join, cells.Select(cell => cell.Value))); // concatenate the cells
}
return sb.ToString();
}

bool PromptToOpenFile()
{
var openDialog = new OpenFileDialog()
{
InitialDirectory = Convert.ToString(Environment.SpecialFolder.MyDocuments),
Filter = @"Text (*.txt)|*.txt|All Files (*.*)|*.*",
FilterIndex = 1
};

if (openDialog.ShowDialog() == DialogResult.OK)
{
_currentFile = openDialog.FileName;
return true;
}

return false;
}

bool PromptToSaveFile()
{
var saveDialog = new SaveFileDialog
{
InitialDirectory = Convert.ToString(Environment.SpecialFolder.MyDocuments),
Filter = @"Text (*.txt)|*.txt|All Files (*.*)|*.*",
FilterIndex = 1,
CheckFileExists = true,
};

if (saveDialog.ShowDialog() == DialogResult.OK)
{
_currentFile = saveDialog.FileName;
SaveFile(_currentFile);
return true;
}

return false;
}

void SaveFile(string filename)
{
var assocToText = DatagridToString(dataGridAssociates, '|');
using (var outfile = new StreamWriter(filename))
outfile.Write(assocToText);

_unsavedChanges = false;
}


And finally the events. You should be able to figure out what they do by their names. (There are two more buttons, Save and Load.)

    private void btnSave_Click(object sender, EventArgs e)
{
PromptToSaveFile();
}

private void btnLoad_Click(object sender, EventArgs e)
{
if (!CheckForUnsavedChanges())
return;

PromptToOpenFile();
}

private void dataGridAssociates_CellValueChanged(object sender, DataGridViewCellEventArgs e)
{
_unsavedChanges = true;
}
}


I've uploaded my whole project here. It should be runnable as-is. It contains the shuffle method, my Associate object, and the form.

-
Holy crap you are truly amazing. I can't thank you enough for such a thorough answer! This is perfect. –  JesterBaze Jan 15 '13 at 21:29
@Porges: I prefer to avoid creating a new instance of Random on each call to BtnRandomizeClick . While it is unlikely that the user will manage to create two instances of Random with the same seed (i.e., by running BtnRandomizeClick twice within the same tick), it still strikes me as bad practice. –  Brian Jan 15 '13 at 22:55
@Brian: Yes, I'd normally stash that somewhere. (The main thing I was trying to show is that the Shuffle method shouldn't create the Random instance itself or a duplicate seed is much more likely to occur in normal usage.) –  Porges Jan 16 '13 at 5:11

Here's another stab at something. Mainly I think I see a bunch of duplicated code in your lists, so what about something along the lines of

private IList<Associate> GetAssociates(IEnumerable<Associate> query,  int loadFrom, int maxItems)
{
var numberRequested = maxItems - loadFrom;
return query.Take(numberRequested).ToList();
}

private void LoadAssociatesInto(ListBox container, IEnumerable<Associate> associates)
{
container.Items.AddRange(associates.Select(p => string.format("{0} {1}", p.FirstName, p.LastName)).ToArray<object>());
}

// ... and your main code
sortedMaster.RemoveAll(associates);

// similiar code for canDoMHEQuery and canDoDivertsQuery

// finally
MessageBox.Show(@"You have " + sortedMaster.Count() + @" associates unassigned.");


The 3 lines for doing each of these could also be moved into a method as well so further refactoring would reduce the core code down to 4 lines.

-
This is cool! Might be hard for a noob to swallow ... –  Leonid Jan 14 '13 at 1:07
@dreza This is very cool, but yeah I'm having a hard time taking it in... I'm new to c#, and extremely new to using LINQ (just read about it on Friday). Using methods like you mentioned, how would that look ultimately? How would I call the method? I think using methods is something I'm missing from my whole program... –  JesterBaze Jan 14 '13 at 2:28
Specifically I don't understand using IList<> and IEnumerable<>, also don't understand creating my own methods very well... Only method I've created has been my DatagridView to String method used in saving my datagrid to a text file. –  JesterBaze Jan 14 '13 at 2:40
@JesterBaze I agree with Leonid that you should probably let your question mature for a bit. By accepting my answer to soon you are less likely to get feedback. So, to get a more variety of help perhaps un-accept mine and see what other ideas/comments you get :) I'll update my answer soon as well. –  dreza Jan 14 '13 at 5:57
Thankyou for the advice! I will mark it later on =) –  JesterBaze Jan 14 '13 at 21:53

As a first pass here is what I got. It is nowhere close to what it should be, but it is a start. If you gave more code, I could perfect it further but right now I do not know where the variables are coming from.

public void Sort()
{
//Begin the sorting method
{
{

}
}

if (mhes != mheCountRequested)
{
foreach (Associate someAssocVar in canDoMHEQuery)
{
mhes++;
listBoxMHE.Items.Add(mheList[mhes - 1].FirstName +
" " + mheList[mhes - 1].LastName);
}
}

sortedMaster.RemoveAll(mheList.Contains);
if (diverters != divertCountRequested)
{
foreach (Associate someAssocVar in canDoDivertsQuery)
{
diverters++;
listBoxDiverts.Items.Add(divertsList[diverters - 1].FirstName +
" " + divertsList[diverters - 1].LastName);
}
}

sortedMaster.RemoveAll(divertsList.Contains);
MessageBox.Show(@"You have " + sortedMaster.Count() + @" associates unassigned.");
}

-
I added the rest of my code from the button click. Hope that helps? This is a really messy/hacky almost embarassing code, but it's the first sizable program I've written as I've mentioned before. Haven't taken any programming classes or anything so go easy on me haha. My whole program is ~630 lines, I'm guessing it could be condensed down to <200 easy. I just don't understand much yet aside from using loops and gotos. I came from Pascal into C#. Big difference... –  JesterBaze Jan 14 '13 at 2:38

I didnt check carefully but really 3 (at least) were because you didnt realize, know or remember break isn't only used in switch statements. The other one is really something suggesting if it should loop again or not.

You should run your code and my code in a diff tool to easily see the few lines that changed.

public void BtnRandomizeClick(object sender, EventArgs e)
{
var DoAgain=true;
while(DoAgain){
DoAgain=false;
switch (MessageBox.Show(@"Did you SAVE your edited list?", @"Just checking... It's important.",
MessageBoxButtons.YesNo))
{
#region Dialog Yes

case DialogResult.Yes:
if (FileChosen == null)
{
MessageBox.Show(
@"Please select a file to load before trying to randomize. You need some entries to randomize first eh?",
"Oh fiddlesticks...");
return;
}
dataGridAssociates.Rows.Clear();
var masterList = new List<Associate>();
var divertsList = new List<Associate>();
var mheList = new List<Associate>();
var lines = File.ReadAllLines(FileChosen); //Array of all the lines in the text file

foreach (var assocStringer in lines)
{
if (assocStringer == null) continue;
var entries = assocStringer.Split('|');
var obj = (Associate) _bindingSource.AddNew();
if (obj == null) continue;
if (entries.Length < 7) break;
obj.FirstName = entries[0];
obj.LastName = entries[1];
obj.AssocId = entries[2];
obj.AssocRfid = entries[3];
obj.CanDoDiverts = Boolean.Parse(entries[4]);
obj.CanDoMhe = Boolean.Parse(entries[5]);
obj.UniqueRandomID = Guid.NewGuid().ToString();
}
var sortedMaster = masterList.OrderBy(d => d.UniqueRandomID).ToList();
var loaderCountRequested = 0;
var mheCountRequested = 0;
var divertCountRequested = 0;
var validDivertEntry = int.TryParse(txtDivertWorkers.Text, out divertCountRequested);
var validMHEEntry = int.TryParse(txtMHEWorkers.Text, out mheCountRequested);

//See if whomever used the inputs correctly
if (validDivertEntry == false)
{
MessageBox.Show(@"Please use an integer in the 'Divert Workers' textbox and try again");
return;
}
if (validMHEEntry == false)
{
MessageBox.Show(@"Please use an integer in the 'MHE Workers' textbox and try again");
return;
}
if (validLoaderEntry == false)
{
MessageBox.Show(@"Please use an integer in the 'Loaders' textbox and try again");
return;
}

var canDoDivertsQuery = sortedMaster.Where(a => a.CanDoDiverts);
var canDoMHEQuery = sortedMaster.Where(b => b.CanDoMhe);
var loaders = 0;
var mhes = 0;
var diverters = 0;

//Begin the sorting method
{
break;

}

foreach (Associate someAssocVar in canDoMHEQuery)
{
if (mhes == mheCountRequested)
break;
mhes++;
listBoxMHE.Items.Add(mheList[mhes - 1].FirstName + " " +
mheList[mhes - 1].LastName);
}

sortedMaster.RemoveAll(mheList.Contains);

foreach (Associate someAssocVar in canDoDivertsQuery)
{
if (diverters == divertCountRequested)
break;
diverters++;
listBoxDiverts.Items.Add(divertsList[diverters - 1].FirstName + " " +
divertsList[diverters - 1].LastName);
}

sortedMaster.RemoveAll(divertsList.Contains);

MessageBox.Show(@"You have " + sortedMaster.Count() + @" associates unassigned.");
break;
#endregion

#region Dialog No

case DialogResult.No:
if (MessageBox.Show(@"Would you like to save your work then?", @"It really does matter...",
MessageBoxButtons.YesNo) == DialogResult.Yes)
{
var saveDialog = new SaveFileDialog
{
InitialDirectory =
Convert.ToString(Environment.SpecialFolder.MyDocuments),
// ReSharper disable LocalizableElement
Filter = "Text (*.txt)|*.txt|All Files (*.*)|*.*",
// ReSharper restore LocalizableElement
FilterIndex = 1
};
if (saveDialog.ShowDialog() == DialogResult.OK)
{
FileChosen = saveDialog.FileName;
var assocToText = DatagridToString(dataGridAssociates, '|');
using (var outfile = new StreamWriter(saveDialog.FileName))
outfile.Write(assocToText);
}
DoAgain=true;
}
break;

#endregion
}
}
}

-
Not bad! However I'm thinking that I can reduce it further from that if I can figure out how to make my own Methods, but thats a cool solution, thankyou! –  JesterBaze Jan 14 '13 at 21:58

Another tip. You can write code like this which will make it easier to move chucks into functions.

{
int heyGuessWhatICanDeclThisAgainLater=1;
blah blah blah
if (blah != blahblahblah)
{
blah
}
}
{
bool heyGuessWhatICanDeclThisAgainLater= true;//not even the same type
//but confusing if a dif type. But good for single letter loop vars/temps
//like foreach(var v in list) { ...
if (blah == blahblahblah || blu blu blah)
{
blah
}
}


Also you shouldnt create so many variables (or reuse any vars. assign once is a good idea). You should just write

foreach (Associate someAssocVar in sortedMaster.Where(a => a.CanDoDiverts))


Its confusing to see code declare variables one place and used WAYYYY LATER.

-