HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Prompting a user to save and regenerate some entries in a file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fileuserpromptingregeneratesavesomeandentries

Problem

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();
var divertsList = new List();
var mheList = new List();
var loadingList = new List();
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 d.UniqueRandomID).ToList();
var loaderCountRequested = 0;
var mheCountRequested = 0;
var divertCount

Solution

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 GetAssociates(IEnumerable query,  int loadFrom, int maxItems)
{
    var numberRequested = maxItems - loadFrom;
    return query.Take(numberRequested).ToList();
}

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

// ... and your main code
var associates = GetAssociates(canDoLoadingQuery, loaders, loaderCountRequested);
LoadAssociatesInto(listBoxLoaders, associates);
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.

Code Snippets

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
var associates = GetAssociates(canDoLoadingQuery, loaders, loaderCountRequested);
LoadAssociatesInto(listBoxLoaders, associates);
sortedMaster.RemoveAll(associates);

// similiar code for canDoMHEQuery and canDoDivertsQuery

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

Context

StackExchange Code Review Q#20497, answer score: 2

Revisions (0)

No revisions yet.