patterncsharpMinor
Getting the max value from a list of sums
Viewed 0 times
thegettingvaluemaxsumslistfrom
Problem
This is my version of the knapsack challenge and I was wondering if there was a way to do it so it doesn't go through the
I also have a class with this in:
```
public class Solver
{
public List> mResults;
public int closestValue = 0;
public List> Solve(int goal, int[] elements)
{
mResults = new List>();
RecursiveSolve(goal, 0, new List(), new List(elements), 0);
return mResults;
}
private void RecursiveSolve(int goal, int currentSum,
List included, List notIncluded, int startIndex)
{
for (int index = startIndex
foreach statement so many times. At the minute it goes through it thousands of times to get he closes value to the target number, which takes long time to do, I was wondering if there was a way to either do it without foreach or make it quick with the foreach.private void btnGenerate_Click(object sender, RoutedEventArgs e)
{
string input;
string bag;
int goal;
int count = 0;
int element;
List finalResult = new List();
do
{
bag = txtBag.Text;
}
while (!int.TryParse(bag, out goal));
input = txtGenerate.Text;
string[] elementsText = input.Split(' ');
List elementsList = new List();
foreach (string elementText in elementsText)
{
if (int.TryParse(elementText, out element))
{
elementsList.Add(element);
}
}
Solver solver = new Solver();
List> results = solver.Solve(goal, elementsList.ToArray());
int closestValue = 0;
foreach (List result in results)
{
count++;
int totalValue = result.Sum(x => Convert.ToInt32(x));
if (totalValue >= closestValue)
{
closestValue = totalValue;
finalResult = result;
txtSum.Text = closestValue.ToString();
}
}
foreach (int value in finalResult)
{
txtCount.Text = count.ToString();
cboResult.ItemsSource = finalResult;
}
}I also have a class with this in:
```
public class Solver
{
public List> mResults;
public int closestValue = 0;
public List> Solve(int goal, int[] elements)
{
mResults = new List>();
RecursiveSolve(goal, 0, new List(), new List(elements), 0);
return mResults;
}
private void RecursiveSolve(int goal, int currentSum,
List included, List notIncluded, int startIndex)
{
for (int index = startIndex
Solution
You've got logic mixed in with your view.
Move as much of the code out of your button handler as you can, and into a separate function (and preferably a separate class).
A button handler should do just that, handle button presses. It should not then perform calculations.
Your spacing could use some work. For example:
Would be a lot more readable like this:
Use
e.g.
Avoid Hungarian notation in C#, and prefer public properties over public fields. Additionally, public collections should be readonly, and you don't need to intiialize an integer as 0. Its default value is 0.
Move as much of the code out of your button handler as you can, and into a separate function (and preferably a separate class).
A button handler should do just that, handle button presses. It should not then perform calculations.
Your spacing could use some work. For example:
int element;
List finalResult = new List();
do
{
bag = txtBag.Text;
}
while (!int.TryParse(bag, out goal));
input = txtGenerate.Text;
string[] elementsText = input.Split(' ');
List elementsList = new List();
foreach (string elementText in elementsText)
{
if (int.TryParse(elementText, out element))
{
elementsList.Add(element);
}
}
Solver solver = new Solver();
List> results = solver.Solve(goal, elementsList.ToArray());
int closestValue = 0;
foreach (List result in results)
{
count++;
int totalValue = result.Sum(x => Convert.ToInt32(x));
if (totalValue >= closestValue)
{
closestValue = totalValue;
finalResult = result;
txtSum.Text = closestValue.ToString();
}
}Would be a lot more readable like this:
int element;
List finalResult = new List();
do
{
bag = txtBag.Text;
}
while (!int.TryParse(bag, out goal));
input = txtGenerate.Text;
string[] elementsText = input.Split(' ');
List elementsList = new List();
foreach (string elementText in elementsText)
{
if (int.TryParse(elementText, out element))
{
elementsList.Add(element);
}
}
Solver solver = new Solver();
List> results = solver.Solve(goal, elementsList.ToArray());
int closestValue = 0;
foreach (List result in results)
{
count++;
int totalValue = result.Sum(x => Convert.ToInt32(x));
if (totalValue >= closestValue)
{
closestValue = totalValue;
finalResult = result;
txtSum.Text = closestValue.ToString();
}
}Use
var when it is obvious from the RHS of a declaration what the type of the variable is, and always use var in foreach loops.e.g.
int element;
var finalResult = new List();
do
{
bag = txtBag.Text;
}
while (!int.TryParse(bag, out goal));
input = txtGenerate.Text;
var elementsText = input.Split(' ');
var elementsList = new List();
foreach (var elementText in elementsText)
{
if (int.TryParse(elementText, out element))
{
elementsList.Add(element);
}
}
var solver = new Solver();
List> results = solver.Solve(goal, elementsList.ToArray());
var closestValue = 0;
foreach (var result in results)
{
count++;
int totalValue = result.Sum(x => Convert.ToInt32(x));
if (totalValue >= closestValue)
{
closestValue = totalValue;
finalResult = result;
txtSum.Text = closestValue.ToString();
}
}Avoid Hungarian notation in C#, and prefer public properties over public fields. Additionally, public collections should be readonly, and you don't need to intiialize an integer as 0. Its default value is 0.
private readonly List> results
public List> Results
{
get
{
return results;
}
}
public int ClosestValue {get; set;}Code Snippets
int element;
List<int> finalResult = new List<int>();
do
{
bag = txtBag.Text;
}
while (!int.TryParse(bag, out goal));
input = txtGenerate.Text;
string[] elementsText = input.Split(' ');
List<int> elementsList = new List<int>();
foreach (string elementText in elementsText)
{
if (int.TryParse(elementText, out element))
{
elementsList.Add(element);
}
}
Solver solver = new Solver();
List<List<int>> results = solver.Solve(goal, elementsList.ToArray());
int closestValue = 0;
foreach (List<int> result in results)
{
count++;
int totalValue = result.Sum(x => Convert.ToInt32(x));
if (totalValue >= closestValue)
{
closestValue = totalValue;
finalResult = result;
txtSum.Text = closestValue.ToString();
}
}int element;
List<int> finalResult = new List<int>();
do
{
bag = txtBag.Text;
}
while (!int.TryParse(bag, out goal));
input = txtGenerate.Text;
string[] elementsText = input.Split(' ');
List<int> elementsList = new List<int>();
foreach (string elementText in elementsText)
{
if (int.TryParse(elementText, out element))
{
elementsList.Add(element);
}
}
Solver solver = new Solver();
List<List<int>> results = solver.Solve(goal, elementsList.ToArray());
int closestValue = 0;
foreach (List<int> result in results)
{
count++;
int totalValue = result.Sum(x => Convert.ToInt32(x));
if (totalValue >= closestValue)
{
closestValue = totalValue;
finalResult = result;
txtSum.Text = closestValue.ToString();
}
}int element;
var finalResult = new List<int>();
do
{
bag = txtBag.Text;
}
while (!int.TryParse(bag, out goal));
input = txtGenerate.Text;
var elementsText = input.Split(' ');
var elementsList = new List<int>();
foreach (var elementText in elementsText)
{
if (int.TryParse(elementText, out element))
{
elementsList.Add(element);
}
}
var solver = new Solver();
List<List<int>> results = solver.Solve(goal, elementsList.ToArray());
var closestValue = 0;
foreach (var result in results)
{
count++;
int totalValue = result.Sum(x => Convert.ToInt32(x));
if (totalValue >= closestValue)
{
closestValue = totalValue;
finalResult = result;
txtSum.Text = closestValue.ToString();
}
}private readonly List<List<int>> results
public List<List<int>> Results
{
get
{
return results;
}
}
public int ClosestValue {get; set;}Context
StackExchange Code Review Q#65180, answer score: 3
Revisions (0)
No revisions yet.