reviewing old code
-
They're both horrors. :(( You don't ever need to return Stuff. Not to mention this "ShuffleNodeList" function is called each time loadStuff is called (I'm guessing it should only be called after everything has been loaded). I wonder... is this code that is using the Umbraco API? Some of that looks familiar. :)
Yes it's the Umbraco api and it's not the complete code, it returns the list to be used in an other function that generates html in order to make the page look different every time you visit it. Having the ShuffleNodeList was at the end at first... I though I had a good reason to put it in between at first, but looking at it doesn't make sense anymore. :~
Giraffes are not real.
-
WHYYYYY??? If you're declaring Stuff in Page_Load and never doing anything with it, just nuke it all! Or did you not show the code that actually uses Stuff to populate the page or whatever?
-
Behold! I found something like this:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
someMoreStuff:
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (Stuff.Count < 100 && Stuff.Count > 0)
{
goto someMoreStuff;
}
return Stuff;
}It worked... but I changed it so that it never happened:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = new List();
Stuff = loadStuff(Stuff);
}private List loadStuff(List Stuff)
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (blablabla)
{
loadStuff(Stuff);
}
return Stuff;
}Edit: So I changed it again...
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Giraffes are not real.
I wonder, why u had to use recursion/goto at all:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = loadStuff();
}private List loadStuff()
{
var Stuff = new List();
do
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
}
while (blablabla);
return Stuff;
} -
Behold! I found something like this:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
someMoreStuff:
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (Stuff.Count < 100 && Stuff.Count > 0)
{
goto someMoreStuff;
}
return Stuff;
}It worked... but I changed it so that it never happened:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = new List();
Stuff = loadStuff(Stuff);
}private List loadStuff(List Stuff)
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (blablabla)
{
loadStuff(Stuff);
}
return Stuff;
}Edit: So I changed it again...
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Giraffes are not real.
On the no need for recursion and getting rid of the goto, can I add only doing a single shuffle:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}/// Create a list of nodes.
/// Takes the child nodes from the current node and loads at
/// least 100 items into the list. If there are currently 30 child nodes
/// this will return 120 items in the list.
private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Nice, self contained and easy to follow.
Panic, Chaos, Destruction. My work here is done. Drink. Get drunk. Fall over - P O'H OK, I will win to day or my name isn't Ethel Crudacre! - DD Ethel Crudacre I cannot live by bread alone. Bacon and ketchup are needed as well. - Trollslayer Have a bit more patience with newbies. Of course some of them act dumb - they're often *students*, for heaven's sake - Terry Pratchett
-
Behold! I found something like this:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
someMoreStuff:
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (Stuff.Count < 100 && Stuff.Count > 0)
{
goto someMoreStuff;
}
return Stuff;
}It worked... but I changed it so that it never happened:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = new List();
Stuff = loadStuff(Stuff);
}private List loadStuff(List Stuff)
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (blablabla)
{
loadStuff(Stuff);
}
return Stuff;
}Edit: So I changed it again...
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Giraffes are not real.
Chuckle! I think a lot of us do the same, when the occasion arises. I certainly do. Of course, there's no stigma involved in the practice, as long as you remember to test your replacement code thoroughly. Right? Right?
(As it happens, I also do it to my old, already-published fiction...which causes my readers a bit of concern when they discover it! Well, at least with fiction the "debugging" is less onerous.)
-
Behold! I found something like this:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
someMoreStuff:
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (Stuff.Count < 100 && Stuff.Count > 0)
{
goto someMoreStuff;
}
return Stuff;
}It worked... but I changed it so that it never happened:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = new List();
Stuff = loadStuff(Stuff);
}private List loadStuff(List Stuff)
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (blablabla)
{
loadStuff(Stuff);
}
return Stuff;
}Edit: So I changed it again...
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Giraffes are not real.
Gah! You needed that goto for the "Goto Hell" Achievement! I remember when goto and gosub/return were the ONLY way to get around in your program flow, how times have changed!
-
Yes it's used for something of course. I'm not 'that' stupid. :doh: Edit: and I'm not actually calling my variables "Stuff" either in case you're wondering. ;P
Giraffes are not real.
0bx wrote:
and I'm not actually calling my variables "Stuff" either
No? I'm disappointed. I found that rather original. ;P
-
Behold! I found something like this:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
someMoreStuff:
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (Stuff.Count < 100 && Stuff.Count > 0)
{
goto someMoreStuff;
}
return Stuff;
}It worked... but I changed it so that it never happened:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = new List();
Stuff = loadStuff(Stuff);
}private List loadStuff(List Stuff)
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (blablabla)
{
loadStuff(Stuff);
}
return Stuff;
}Edit: So I changed it again...
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Giraffes are not real.
Yep, both are horrors.
-
On the no need for recursion and getting rid of the goto, can I add only doing a single shuffle:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}/// Create a list of nodes.
/// Takes the child nodes from the current node and loads at
/// least 100 items into the list. If there are currently 30 child nodes
/// this will return 120 items in the list.
private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Nice, self contained and easy to follow.
Panic, Chaos, Destruction. My work here is done. Drink. Get drunk. Fall over - P O'H OK, I will win to day or my name isn't Ethel Crudacre! - DD Ethel Crudacre I cannot live by bread alone. Bacon and ketchup are needed as well. - Trollslayer Have a bit more patience with newbies. Of course some of them act dumb - they're often *students*, for heaven's sake - Terry Pratchett
-
Behold! I found something like this:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
someMoreStuff:
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (Stuff.Count < 100 && Stuff.Count > 0)
{
goto someMoreStuff;
}
return Stuff;
}It worked... but I changed it so that it never happened:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = new List();
Stuff = loadStuff(Stuff);
}private List loadStuff(List Stuff)
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (blablabla)
{
loadStuff(Stuff);
}
return Stuff;
}Edit: So I changed it again...
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Giraffes are not real.
0bx wrote:
foreach (Node thing in Node.GetCurrent().Children) { Stuff.Add(thing); }
Others have commented on the outer looping constructs. Regarding the inner foreach loop, I'd just get rid of it:
Stuff.AddRange(Node.GetCurrent().Children);
Cheers.
-
Yes it's the Umbraco api and it's not the complete code, it returns the list to be used in an other function that generates html in order to make the page look different every time you visit it. Having the ShuffleNodeList was at the end at first... I though I had a good reason to put it in between at first, but looking at it doesn't make sense anymore. :~
Giraffes are not real.
0bx wrote:
it returns the list to be used in an other function
But the calling function already has the list, as it was passed in as a parameter, so it does not need to be returned since it already exists at the location it would be returned to. I suppose returning it could make for shorter syntax, but not in the case you have shown.
-
Behold! I found something like this:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
someMoreStuff:
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (Stuff.Count < 100 && Stuff.Count > 0)
{
goto someMoreStuff;
}
return Stuff;
}It worked... but I changed it so that it never happened:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = new List();
Stuff = loadStuff(Stuff);
}private List loadStuff(List Stuff)
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (blablabla)
{
loadStuff(Stuff);
}
return Stuff;
}Edit: So I changed it again...
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Giraffes are not real.
What if Stuff.Count never exceeds 100 items? Both versions seem like infinite loops. Recursion would be superior here because it will at least blow the call stack in a reasonable time frame vs. waiting for the application container (IIS?) to kill the non-responsive thread. I usually include a safety valve for that kind of code: int maxReps = 100; blalbalba && (maxReps-- > 0) I always use safety valves when writing code that "should" converge to an answer.
-
Behold! I found something like this:
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
someMoreStuff:
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (Stuff.Count < 100 && Stuff.Count > 0)
{
goto someMoreStuff;
}
return Stuff;
}It worked... but I changed it so that it never happened:
protected void Page_Load(object sender, EventArgs e)
{
var Stuff = new List();
Stuff = loadStuff(Stuff);
}private List loadStuff(List Stuff)
{
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
Stuff = ShuffleNodeList(Stuff);
if (blablabla)
{
loadStuff(Stuff);
}
return Stuff;
}Edit: So I changed it again...
protected void Page_Load(object sender, EventArgs e)
{
List Stuff = loadStuff();
}private List loadStuff()
{
List Stuff = new List();
do {
foreach (Node thing in Node.GetCurrent().Children)
{
Stuff.Add(thing);
}
} while (Stuff.Count < 100 && Stuff.Count > 0))return ShuffleNodeList(Stuff);}
Giraffes are not real.