Can this switch and while loop be simplified in C#?
while (p3.Alive || p2.Alive)
{
Random rnd = new Random();
int victim = rnd.Next(1, 3);
switch (victim)
{
case 1:
p1.Attack(p2, 2);
break;
case 2:
p1.Attack(p3, 2);
break;
}
Thread.Sleep(2000);
}
p2 and p3 and objects of class Person. They are about to be killed off on random by the killer, p1. But when I look at it, I feel like there could be a better solution because what if I had 1000 objects of class Person ready to be killed? I just can't seem to get the p2 and p3 to be easily programmed variables.
The while loop is the same. What if I had 1000 objects. Or even just 10. How can I write this so that the condition is "while anyone but the killer is alive" and an "if" or "switch" which attacks anyone chosen by the Random rnd?
The question is badly written in the topic. I don't know how I could summarize all this into a short question. I'll edit it if anyone has a good suggestion. Thank you.
c#
add a comment |
while (p3.Alive || p2.Alive)
{
Random rnd = new Random();
int victim = rnd.Next(1, 3);
switch (victim)
{
case 1:
p1.Attack(p2, 2);
break;
case 2:
p1.Attack(p3, 2);
break;
}
Thread.Sleep(2000);
}
p2 and p3 and objects of class Person. They are about to be killed off on random by the killer, p1. But when I look at it, I feel like there could be a better solution because what if I had 1000 objects of class Person ready to be killed? I just can't seem to get the p2 and p3 to be easily programmed variables.
The while loop is the same. What if I had 1000 objects. Or even just 10. How can I write this so that the condition is "while anyone but the killer is alive" and an "if" or "switch" which attacks anyone chosen by the Random rnd?
The question is badly written in the topic. I don't know how I could summarize all this into a short question. I'll edit it if anyone has a good suggestion. Thank you.
c#
1
Why not have a list of players?
– stuartd
Nov 16 '18 at 10:51
2
Don't create a newRandom
object at each iteration, that could make the outcomes predictable because they all start at 'the first number from a semi-random list'. You are probably being saved here by theSleep(2000)
, but still it's never a good idea and never needed.
– Peter B
Nov 16 '18 at 10:51
1
Also this code makes it possible to attack a player that is not alive anymore. Waste of "attack resources" and a big logic flaw.
– Peter B
Nov 16 '18 at 10:54
@PeterB Hey, I did as you said and tested it and it works better now, thank you. How can I prevent him from attacking someone if already dead then? What would you do?
– Furnus
Nov 16 '18 at 12:25
add a comment |
while (p3.Alive || p2.Alive)
{
Random rnd = new Random();
int victim = rnd.Next(1, 3);
switch (victim)
{
case 1:
p1.Attack(p2, 2);
break;
case 2:
p1.Attack(p3, 2);
break;
}
Thread.Sleep(2000);
}
p2 and p3 and objects of class Person. They are about to be killed off on random by the killer, p1. But when I look at it, I feel like there could be a better solution because what if I had 1000 objects of class Person ready to be killed? I just can't seem to get the p2 and p3 to be easily programmed variables.
The while loop is the same. What if I had 1000 objects. Or even just 10. How can I write this so that the condition is "while anyone but the killer is alive" and an "if" or "switch" which attacks anyone chosen by the Random rnd?
The question is badly written in the topic. I don't know how I could summarize all this into a short question. I'll edit it if anyone has a good suggestion. Thank you.
c#
while (p3.Alive || p2.Alive)
{
Random rnd = new Random();
int victim = rnd.Next(1, 3);
switch (victim)
{
case 1:
p1.Attack(p2, 2);
break;
case 2:
p1.Attack(p3, 2);
break;
}
Thread.Sleep(2000);
}
p2 and p3 and objects of class Person. They are about to be killed off on random by the killer, p1. But when I look at it, I feel like there could be a better solution because what if I had 1000 objects of class Person ready to be killed? I just can't seem to get the p2 and p3 to be easily programmed variables.
The while loop is the same. What if I had 1000 objects. Or even just 10. How can I write this so that the condition is "while anyone but the killer is alive" and an "if" or "switch" which attacks anyone chosen by the Random rnd?
The question is badly written in the topic. I don't know how I could summarize all this into a short question. I'll edit it if anyone has a good suggestion. Thank you.
c#
c#
edited Nov 16 '18 at 10:55
stuartd
51.4k1199128
51.4k1199128
asked Nov 16 '18 at 10:49
FurnusFurnus
93
93
1
Why not have a list of players?
– stuartd
Nov 16 '18 at 10:51
2
Don't create a newRandom
object at each iteration, that could make the outcomes predictable because they all start at 'the first number from a semi-random list'. You are probably being saved here by theSleep(2000)
, but still it's never a good idea and never needed.
– Peter B
Nov 16 '18 at 10:51
1
Also this code makes it possible to attack a player that is not alive anymore. Waste of "attack resources" and a big logic flaw.
– Peter B
Nov 16 '18 at 10:54
@PeterB Hey, I did as you said and tested it and it works better now, thank you. How can I prevent him from attacking someone if already dead then? What would you do?
– Furnus
Nov 16 '18 at 12:25
add a comment |
1
Why not have a list of players?
– stuartd
Nov 16 '18 at 10:51
2
Don't create a newRandom
object at each iteration, that could make the outcomes predictable because they all start at 'the first number from a semi-random list'. You are probably being saved here by theSleep(2000)
, but still it's never a good idea and never needed.
– Peter B
Nov 16 '18 at 10:51
1
Also this code makes it possible to attack a player that is not alive anymore. Waste of "attack resources" and a big logic flaw.
– Peter B
Nov 16 '18 at 10:54
@PeterB Hey, I did as you said and tested it and it works better now, thank you. How can I prevent him from attacking someone if already dead then? What would you do?
– Furnus
Nov 16 '18 at 12:25
1
1
Why not have a list of players?
– stuartd
Nov 16 '18 at 10:51
Why not have a list of players?
– stuartd
Nov 16 '18 at 10:51
2
2
Don't create a new
Random
object at each iteration, that could make the outcomes predictable because they all start at 'the first number from a semi-random list'. You are probably being saved here by the Sleep(2000)
, but still it's never a good idea and never needed.– Peter B
Nov 16 '18 at 10:51
Don't create a new
Random
object at each iteration, that could make the outcomes predictable because they all start at 'the first number from a semi-random list'. You are probably being saved here by the Sleep(2000)
, but still it's never a good idea and never needed.– Peter B
Nov 16 '18 at 10:51
1
1
Also this code makes it possible to attack a player that is not alive anymore. Waste of "attack resources" and a big logic flaw.
– Peter B
Nov 16 '18 at 10:54
Also this code makes it possible to attack a player that is not alive anymore. Waste of "attack resources" and a big logic flaw.
– Peter B
Nov 16 '18 at 10:54
@PeterB Hey, I did as you said and tested it and it works better now, thank you. How can I prevent him from attacking someone if already dead then? What would you do?
– Furnus
Nov 16 '18 at 12:25
@PeterB Hey, I did as you said and tested it and it works better now, thank you. How can I prevent him from attacking someone if already dead then? What would you do?
– Furnus
Nov 16 '18 at 12:25
add a comment |
2 Answers
2
active
oldest
votes
This seems like it is essentially a coin toss with 2 outcomes; 2 outcomes can be modelled with the "conditional" operator:
var target = rnd.Next(0,2) == 0 ? p2 : p3;
pi.Attack(target);
For larger groups, having the possible targets in a list or array may be useful; then you can essentially do:
var target = list[rnd.Next(list.Count)];
Or with C# 8, "switch expressions" may be useful:
var target = rnd.Next(4) switch (
case 0: p1,
case 1: p2,
case 2: p3,
case 3: p4,
case _: default
);
The ternary operator is costly in term of processing.
– Hassaan
Nov 16 '18 at 10:59
4
@Hassaan it is a single conditional jump (typically brtrue or brfalse - or their "short" forms) - certainly not "expensive" by most definitions; it may cause a CPU branch prediction fail, and there are cases where that might be relevant, but frankly, that is going to be absolutely nothing compared to the cost of generating the next random number, so: I'm going to ignore it completely. In the scenario presented, a conditional is absolutely free
– Marc Gravell♦
Nov 16 '18 at 11:03
add a comment |
Fixes for all issues:
- Only make one Random object
- Use a List to avoid repeated code
- Don't attack opponents that are no longer Alive
using System.Collections.Generic; // provides the List type
Random rnd = new Random();
var opponents = new List<Player> { p2, p3 }; // add more as needed
while (true)
{
opponents.RemoveAll(x => !x.Alive); // only keep live enemies (efficient: does not create new List)
if (opponents.Count == 0) // if nobody left --> exit loop
break;
int victim = rnd.Next(0, opponents.Count);
p1.Attack(opponents[victim]);
Thread.Sleep(2000);
}
1
creating an array every time, and using theWhere
predicate each time - are both unnecessarily expensive here; I'd be more inclined to populate a list at the start, and basically dovar target = opponents[victim]; p1.Attack(target); if (!target.IsAlive) { opponents.Remove(target); }
– Marc Gravell♦
Nov 16 '18 at 11:07
Thanks, probably a better approach, unless opponents could also die for other reasons (e.g. old age) outside the control of this loop ;-)
– Peter B
Nov 16 '18 at 11:08
2
indeed; in that case,List<T>
has a predicate-based remove all method, so - before picking a target:opponents.RemoveAll(x => !x.IsAlive);
- the predicate will be hoisted into a static by the C# compiler, so that it doesn't cost an allocation per call
– Marc Gravell♦
Nov 16 '18 at 11:10
1
Followed through. Just a thought: maybe SO should introduce PRs and Code Reviews for answers (and questions!)
– Peter B
Nov 16 '18 at 12:08
@PeterB This looks good, but I would still here have to add the 1000 people to the array. - I also don't understand what we're doing with the Where and x equal to or less than x.Alive and all that. What are we asking? - And what is opponents.length in the if statement?
– Furnus
Nov 16 '18 at 12:36
|
show 4 more comments
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "1"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: true,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: 10,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53336331%2fcan-this-switch-and-while-loop-be-simplified-in-c%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
This seems like it is essentially a coin toss with 2 outcomes; 2 outcomes can be modelled with the "conditional" operator:
var target = rnd.Next(0,2) == 0 ? p2 : p3;
pi.Attack(target);
For larger groups, having the possible targets in a list or array may be useful; then you can essentially do:
var target = list[rnd.Next(list.Count)];
Or with C# 8, "switch expressions" may be useful:
var target = rnd.Next(4) switch (
case 0: p1,
case 1: p2,
case 2: p3,
case 3: p4,
case _: default
);
The ternary operator is costly in term of processing.
– Hassaan
Nov 16 '18 at 10:59
4
@Hassaan it is a single conditional jump (typically brtrue or brfalse - or their "short" forms) - certainly not "expensive" by most definitions; it may cause a CPU branch prediction fail, and there are cases where that might be relevant, but frankly, that is going to be absolutely nothing compared to the cost of generating the next random number, so: I'm going to ignore it completely. In the scenario presented, a conditional is absolutely free
– Marc Gravell♦
Nov 16 '18 at 11:03
add a comment |
This seems like it is essentially a coin toss with 2 outcomes; 2 outcomes can be modelled with the "conditional" operator:
var target = rnd.Next(0,2) == 0 ? p2 : p3;
pi.Attack(target);
For larger groups, having the possible targets in a list or array may be useful; then you can essentially do:
var target = list[rnd.Next(list.Count)];
Or with C# 8, "switch expressions" may be useful:
var target = rnd.Next(4) switch (
case 0: p1,
case 1: p2,
case 2: p3,
case 3: p4,
case _: default
);
The ternary operator is costly in term of processing.
– Hassaan
Nov 16 '18 at 10:59
4
@Hassaan it is a single conditional jump (typically brtrue or brfalse - or their "short" forms) - certainly not "expensive" by most definitions; it may cause a CPU branch prediction fail, and there are cases where that might be relevant, but frankly, that is going to be absolutely nothing compared to the cost of generating the next random number, so: I'm going to ignore it completely. In the scenario presented, a conditional is absolutely free
– Marc Gravell♦
Nov 16 '18 at 11:03
add a comment |
This seems like it is essentially a coin toss with 2 outcomes; 2 outcomes can be modelled with the "conditional" operator:
var target = rnd.Next(0,2) == 0 ? p2 : p3;
pi.Attack(target);
For larger groups, having the possible targets in a list or array may be useful; then you can essentially do:
var target = list[rnd.Next(list.Count)];
Or with C# 8, "switch expressions" may be useful:
var target = rnd.Next(4) switch (
case 0: p1,
case 1: p2,
case 2: p3,
case 3: p4,
case _: default
);
This seems like it is essentially a coin toss with 2 outcomes; 2 outcomes can be modelled with the "conditional" operator:
var target = rnd.Next(0,2) == 0 ? p2 : p3;
pi.Attack(target);
For larger groups, having the possible targets in a list or array may be useful; then you can essentially do:
var target = list[rnd.Next(list.Count)];
Or with C# 8, "switch expressions" may be useful:
var target = rnd.Next(4) switch (
case 0: p1,
case 1: p2,
case 2: p3,
case 3: p4,
case _: default
);
edited Nov 16 '18 at 11:01
answered Nov 16 '18 at 10:56
Marc Gravell♦Marc Gravell
793k19821602565
793k19821602565
The ternary operator is costly in term of processing.
– Hassaan
Nov 16 '18 at 10:59
4
@Hassaan it is a single conditional jump (typically brtrue or brfalse - or their "short" forms) - certainly not "expensive" by most definitions; it may cause a CPU branch prediction fail, and there are cases where that might be relevant, but frankly, that is going to be absolutely nothing compared to the cost of generating the next random number, so: I'm going to ignore it completely. In the scenario presented, a conditional is absolutely free
– Marc Gravell♦
Nov 16 '18 at 11:03
add a comment |
The ternary operator is costly in term of processing.
– Hassaan
Nov 16 '18 at 10:59
4
@Hassaan it is a single conditional jump (typically brtrue or brfalse - or their "short" forms) - certainly not "expensive" by most definitions; it may cause a CPU branch prediction fail, and there are cases where that might be relevant, but frankly, that is going to be absolutely nothing compared to the cost of generating the next random number, so: I'm going to ignore it completely. In the scenario presented, a conditional is absolutely free
– Marc Gravell♦
Nov 16 '18 at 11:03
The ternary operator is costly in term of processing.
– Hassaan
Nov 16 '18 at 10:59
The ternary operator is costly in term of processing.
– Hassaan
Nov 16 '18 at 10:59
4
4
@Hassaan it is a single conditional jump (typically brtrue or brfalse - or their "short" forms) - certainly not "expensive" by most definitions; it may cause a CPU branch prediction fail, and there are cases where that might be relevant, but frankly, that is going to be absolutely nothing compared to the cost of generating the next random number, so: I'm going to ignore it completely. In the scenario presented, a conditional is absolutely free
– Marc Gravell♦
Nov 16 '18 at 11:03
@Hassaan it is a single conditional jump (typically brtrue or brfalse - or their "short" forms) - certainly not "expensive" by most definitions; it may cause a CPU branch prediction fail, and there are cases where that might be relevant, but frankly, that is going to be absolutely nothing compared to the cost of generating the next random number, so: I'm going to ignore it completely. In the scenario presented, a conditional is absolutely free
– Marc Gravell♦
Nov 16 '18 at 11:03
add a comment |
Fixes for all issues:
- Only make one Random object
- Use a List to avoid repeated code
- Don't attack opponents that are no longer Alive
using System.Collections.Generic; // provides the List type
Random rnd = new Random();
var opponents = new List<Player> { p2, p3 }; // add more as needed
while (true)
{
opponents.RemoveAll(x => !x.Alive); // only keep live enemies (efficient: does not create new List)
if (opponents.Count == 0) // if nobody left --> exit loop
break;
int victim = rnd.Next(0, opponents.Count);
p1.Attack(opponents[victim]);
Thread.Sleep(2000);
}
1
creating an array every time, and using theWhere
predicate each time - are both unnecessarily expensive here; I'd be more inclined to populate a list at the start, and basically dovar target = opponents[victim]; p1.Attack(target); if (!target.IsAlive) { opponents.Remove(target); }
– Marc Gravell♦
Nov 16 '18 at 11:07
Thanks, probably a better approach, unless opponents could also die for other reasons (e.g. old age) outside the control of this loop ;-)
– Peter B
Nov 16 '18 at 11:08
2
indeed; in that case,List<T>
has a predicate-based remove all method, so - before picking a target:opponents.RemoveAll(x => !x.IsAlive);
- the predicate will be hoisted into a static by the C# compiler, so that it doesn't cost an allocation per call
– Marc Gravell♦
Nov 16 '18 at 11:10
1
Followed through. Just a thought: maybe SO should introduce PRs and Code Reviews for answers (and questions!)
– Peter B
Nov 16 '18 at 12:08
@PeterB This looks good, but I would still here have to add the 1000 people to the array. - I also don't understand what we're doing with the Where and x equal to or less than x.Alive and all that. What are we asking? - And what is opponents.length in the if statement?
– Furnus
Nov 16 '18 at 12:36
|
show 4 more comments
Fixes for all issues:
- Only make one Random object
- Use a List to avoid repeated code
- Don't attack opponents that are no longer Alive
using System.Collections.Generic; // provides the List type
Random rnd = new Random();
var opponents = new List<Player> { p2, p3 }; // add more as needed
while (true)
{
opponents.RemoveAll(x => !x.Alive); // only keep live enemies (efficient: does not create new List)
if (opponents.Count == 0) // if nobody left --> exit loop
break;
int victim = rnd.Next(0, opponents.Count);
p1.Attack(opponents[victim]);
Thread.Sleep(2000);
}
1
creating an array every time, and using theWhere
predicate each time - are both unnecessarily expensive here; I'd be more inclined to populate a list at the start, and basically dovar target = opponents[victim]; p1.Attack(target); if (!target.IsAlive) { opponents.Remove(target); }
– Marc Gravell♦
Nov 16 '18 at 11:07
Thanks, probably a better approach, unless opponents could also die for other reasons (e.g. old age) outside the control of this loop ;-)
– Peter B
Nov 16 '18 at 11:08
2
indeed; in that case,List<T>
has a predicate-based remove all method, so - before picking a target:opponents.RemoveAll(x => !x.IsAlive);
- the predicate will be hoisted into a static by the C# compiler, so that it doesn't cost an allocation per call
– Marc Gravell♦
Nov 16 '18 at 11:10
1
Followed through. Just a thought: maybe SO should introduce PRs and Code Reviews for answers (and questions!)
– Peter B
Nov 16 '18 at 12:08
@PeterB This looks good, but I would still here have to add the 1000 people to the array. - I also don't understand what we're doing with the Where and x equal to or less than x.Alive and all that. What are we asking? - And what is opponents.length in the if statement?
– Furnus
Nov 16 '18 at 12:36
|
show 4 more comments
Fixes for all issues:
- Only make one Random object
- Use a List to avoid repeated code
- Don't attack opponents that are no longer Alive
using System.Collections.Generic; // provides the List type
Random rnd = new Random();
var opponents = new List<Player> { p2, p3 }; // add more as needed
while (true)
{
opponents.RemoveAll(x => !x.Alive); // only keep live enemies (efficient: does not create new List)
if (opponents.Count == 0) // if nobody left --> exit loop
break;
int victim = rnd.Next(0, opponents.Count);
p1.Attack(opponents[victim]);
Thread.Sleep(2000);
}
Fixes for all issues:
- Only make one Random object
- Use a List to avoid repeated code
- Don't attack opponents that are no longer Alive
using System.Collections.Generic; // provides the List type
Random rnd = new Random();
var opponents = new List<Player> { p2, p3 }; // add more as needed
while (true)
{
opponents.RemoveAll(x => !x.Alive); // only keep live enemies (efficient: does not create new List)
if (opponents.Count == 0) // if nobody left --> exit loop
break;
int victim = rnd.Next(0, opponents.Count);
p1.Attack(opponents[victim]);
Thread.Sleep(2000);
}
edited Nov 16 '18 at 12:10
answered Nov 16 '18 at 11:03
Peter BPeter B
13.6k52046
13.6k52046
1
creating an array every time, and using theWhere
predicate each time - are both unnecessarily expensive here; I'd be more inclined to populate a list at the start, and basically dovar target = opponents[victim]; p1.Attack(target); if (!target.IsAlive) { opponents.Remove(target); }
– Marc Gravell♦
Nov 16 '18 at 11:07
Thanks, probably a better approach, unless opponents could also die for other reasons (e.g. old age) outside the control of this loop ;-)
– Peter B
Nov 16 '18 at 11:08
2
indeed; in that case,List<T>
has a predicate-based remove all method, so - before picking a target:opponents.RemoveAll(x => !x.IsAlive);
- the predicate will be hoisted into a static by the C# compiler, so that it doesn't cost an allocation per call
– Marc Gravell♦
Nov 16 '18 at 11:10
1
Followed through. Just a thought: maybe SO should introduce PRs and Code Reviews for answers (and questions!)
– Peter B
Nov 16 '18 at 12:08
@PeterB This looks good, but I would still here have to add the 1000 people to the array. - I also don't understand what we're doing with the Where and x equal to or less than x.Alive and all that. What are we asking? - And what is opponents.length in the if statement?
– Furnus
Nov 16 '18 at 12:36
|
show 4 more comments
1
creating an array every time, and using theWhere
predicate each time - are both unnecessarily expensive here; I'd be more inclined to populate a list at the start, and basically dovar target = opponents[victim]; p1.Attack(target); if (!target.IsAlive) { opponents.Remove(target); }
– Marc Gravell♦
Nov 16 '18 at 11:07
Thanks, probably a better approach, unless opponents could also die for other reasons (e.g. old age) outside the control of this loop ;-)
– Peter B
Nov 16 '18 at 11:08
2
indeed; in that case,List<T>
has a predicate-based remove all method, so - before picking a target:opponents.RemoveAll(x => !x.IsAlive);
- the predicate will be hoisted into a static by the C# compiler, so that it doesn't cost an allocation per call
– Marc Gravell♦
Nov 16 '18 at 11:10
1
Followed through. Just a thought: maybe SO should introduce PRs and Code Reviews for answers (and questions!)
– Peter B
Nov 16 '18 at 12:08
@PeterB This looks good, but I would still here have to add the 1000 people to the array. - I also don't understand what we're doing with the Where and x equal to or less than x.Alive and all that. What are we asking? - And what is opponents.length in the if statement?
– Furnus
Nov 16 '18 at 12:36
1
1
creating an array every time, and using the
Where
predicate each time - are both unnecessarily expensive here; I'd be more inclined to populate a list at the start, and basically do var target = opponents[victim]; p1.Attack(target); if (!target.IsAlive) { opponents.Remove(target); }
– Marc Gravell♦
Nov 16 '18 at 11:07
creating an array every time, and using the
Where
predicate each time - are both unnecessarily expensive here; I'd be more inclined to populate a list at the start, and basically do var target = opponents[victim]; p1.Attack(target); if (!target.IsAlive) { opponents.Remove(target); }
– Marc Gravell♦
Nov 16 '18 at 11:07
Thanks, probably a better approach, unless opponents could also die for other reasons (e.g. old age) outside the control of this loop ;-)
– Peter B
Nov 16 '18 at 11:08
Thanks, probably a better approach, unless opponents could also die for other reasons (e.g. old age) outside the control of this loop ;-)
– Peter B
Nov 16 '18 at 11:08
2
2
indeed; in that case,
List<T>
has a predicate-based remove all method, so - before picking a target: opponents.RemoveAll(x => !x.IsAlive);
- the predicate will be hoisted into a static by the C# compiler, so that it doesn't cost an allocation per call– Marc Gravell♦
Nov 16 '18 at 11:10
indeed; in that case,
List<T>
has a predicate-based remove all method, so - before picking a target: opponents.RemoveAll(x => !x.IsAlive);
- the predicate will be hoisted into a static by the C# compiler, so that it doesn't cost an allocation per call– Marc Gravell♦
Nov 16 '18 at 11:10
1
1
Followed through. Just a thought: maybe SO should introduce PRs and Code Reviews for answers (and questions!)
– Peter B
Nov 16 '18 at 12:08
Followed through. Just a thought: maybe SO should introduce PRs and Code Reviews for answers (and questions!)
– Peter B
Nov 16 '18 at 12:08
@PeterB This looks good, but I would still here have to add the 1000 people to the array. - I also don't understand what we're doing with the Where and x equal to or less than x.Alive and all that. What are we asking? - And what is opponents.length in the if statement?
– Furnus
Nov 16 '18 at 12:36
@PeterB This looks good, but I would still here have to add the 1000 people to the array. - I also don't understand what we're doing with the Where and x equal to or less than x.Alive and all that. What are we asking? - And what is opponents.length in the if statement?
– Furnus
Nov 16 '18 at 12:36
|
show 4 more comments
Thanks for contributing an answer to Stack Overflow!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53336331%2fcan-this-switch-and-while-loop-be-simplified-in-c%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
1
Why not have a list of players?
– stuartd
Nov 16 '18 at 10:51
2
Don't create a new
Random
object at each iteration, that could make the outcomes predictable because they all start at 'the first number from a semi-random list'. You are probably being saved here by theSleep(2000)
, but still it's never a good idea and never needed.– Peter B
Nov 16 '18 at 10:51
1
Also this code makes it possible to attack a player that is not alive anymore. Waste of "attack resources" and a big logic flaw.
– Peter B
Nov 16 '18 at 10:54
@PeterB Hey, I did as you said and tested it and it works better now, thank you. How can I prevent him from attacking someone if already dead then? What would you do?
– Furnus
Nov 16 '18 at 12:25