|
Yep. That's why I said it still wasn't great. There's no point in involving a Queue in the first place. It just complicates things Hopefully you can teach him!
|
|
|
|
|
I had hope to teach him stuff for a while...
Then I got gave up and got irritated...
Now I am like indifferent and answer his questions, but make no particular effort anymore...
|
|
|
|
|
Tell him your intuition tells you his code can be further improved with a few more lines, but you are not quite certain what they are - its just your intuition. Do it with a straight face and see if he comes back with something even crappier.
|
|
|
|
|
and, and also, evil laugh!
|
|
|
|
|
That's evil
|
|
|
|
|
It's my nature. I once had a coworker convinced that 'La Quinta' was Spanish for 'Next to Denny's'. Another time I convinced a different coworker I had turned in a Honda ES Accord I'd just bought because I decided I didn't like the leather seats - they were too hot, and I'd come to my senses and decided to be more frugal. I had a crappy Civic that had been thrashed, but it was just a loaner car while the new one had some work done on it. It was fun to see how long I could string it along, and I had him totally agreeing with me that the cloth seats were better! Unfortunately, a different coworker ruined the La Quinta one - I was very sad ! But I got a real good laugh out of it nonetheless!
|
|
|
|
|
Jon McKee wrote: There's no point in involving a Queue in the first place
In slight defence of the coder in question, selecting the data type that matches your data is actually good practice. eg you *can* store a date as a string;
"2020-07-22"
save it in SQL as a string, retrieve it and show it or parse it to DateTime if you need to add\remove days to it, convert back to string and save again. But we don't and I'm sure I don't need to explain why. Similarly people use decimal for currency when they really shouldn't. In the original code a List was used to process something that isn't best represented as a List, it was data used as a stack or a queue so using Queue makes that explicit. Also I don't think accessing the last item of a List using an index is very efficient.
The biggest failing in the code is obviously not just dequeuing regardless, that bit is silly and not very readable.
|
|
|
|
|
In the general sense I completely agree, but:
F-ES Sitecore wrote: In the original code a List was used to process something that isn't best represented as a List, it was data used as a stack or a queue so using Queue makes that explicit.
We don't really know this. All the function represents is a filter operation. The main purpose of the List outside of this function could require random access which is much more efficient on a List (O(1)) than a Queue (O(n)). Any filter operation is going to be at least O(n) since it requires processing all data elements. Both the List and Queue implementations have the same time complexity here. As written the Queue implementation has higher code complexity and space requirements though.
F-ES Sitecore wrote: Also I don't think accessing the last item of a List using an index is very efficient.
Numerical indexing is O(1) for array-based List access since underneath everything it's just pointer arithmetic. I suppose they could be using a linked List implementation if they want O(1) add/remove complexity which would make random access O(n) but we really don't know. In the linked List case the underlying data structure is the same as the Queue anyways.
|
|
|
|
|
Jon McKee wrote: We don't really know this. All the function represents is a filter operation. The main purpose of the List outside of this function could require random access
I'm talking about the code in this function, I'm not saying that the List that is passed in should be a Queue, I am saying that for this function it is "more proper" to use a Queue than a List as the List is being treated like one. So while there is an additional overhead making a Queue from a List it will be minimal and you have to balance that against what is more important to you...that additional overhead or the code being structured better. As developers we make these decisions all the time, I'm just trying to point out that his idea isn't without merit, some will accept the overhead for "better" code, yet it is being universally panned.
Jon McKee wrote: Numerical indexing is O(1) for array-based List access since underneath everything it's just pointer arithmetic.
Yes, you're quite correct there, I was wrong on that regard.
|
|
|
|
|
Ah! I get where you're coming from now
|
|
|
|
|
I agree with you, Why write multiple lines of code, when you can do it with one in Linq.
Anything that is unrelated to elephants is irrelephant Anonymous
- The problem with quotes on the internet is that you can never tell if they're genuine Winston Churchill, 1944
- Never argue with a fool. Onlookers may not be able to tell the difference. Mark Twain
|
|
|
|
|
It'd be a breaking change, but if you passed the collection as a ref or better returned a new collection then it's one line of code:
static void FilterList(ref List<foo> list)
{
list = list.RemoveAll(f => Remove(f)).ToList();
}
static IEnumerable<Foo> FilterList(List<Foo> list)
{
return list.RemoveAll(f => Remove(f));
}
I hadn't had my coffee yet ... RemoveAll doesn't return a list, it returns the number of removed items...
So, one line of code:
static void FilterList(List<Foo> list)
{
list.RemoveAll(f => Remove(f));
}
If your list is large, then individual removes can be pretty inefficient: List<T> - Is it really as efficient as you probably think?[^] and building a new collection can be a lot quicker.
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony
"Common sense is so rare these days, it should be classified as a super power" - Random T-shirt
AntiTwitter: @DalekDave is now a follower!
modified 22-Jul-20 2:13am.
|
|
|
|
|
mmm.... the issue here is not really performance at all... both from the discussion point of view or the particular code and data set that this execute on..
It's just about basic understanding I'd say.. (but, from past experience, I doubt he'll learn... )
While we are at it, yes, RemoveAll(), why didn't I use that? mmmm.. good point!
|
|
|
|
|
I just had my coffee, and my brain woke up - see the modified comment.
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony
"Common sense is so rare these days, it should be classified as a super power" - Random T-shirt
AntiTwitter: @DalekDave is now a follower!
|
|
|
|
|
|
BTW, since you are now awake... the even better version!
static void FilterList(List<Foo> list)
=> list.RemoveAll(Remove);
|
|
|
|
|
Re my performance comment, I just looked at the source for RemoveAll, and it's well written:
public int RemoveAll(Predicate<T> match) {
if( match == null) {
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.match);
}
Contract.Ensures(Contract.Result<int>() >= 0);
Contract.Ensures(Contract.Result<int>() <= Contract.OldValue(Count));
Contract.EndContractBlock();
int freeIndex = 0;
while( freeIndex < _size && !match(_items[freeIndex])) freeIndex++;
if( freeIndex >= _size) return 0;
int current = freeIndex + 1;
while( current < _size) {
while( current < _size && match(_items[current])) current++;
if( current < _size) {
_items[freeIndex++] = _items[current++];
}
}
Array.Clear(_items, freeIndex, _size - freeIndex);
int result = _size - freeIndex;
_size = freeIndex;
_version++;
return result;
} Because it's got direct access to the underlying array, it moves as little memory as it has to. Nice - and worth using!
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony
"Common sense is so rare these days, it should be classified as a super power" - Random T-shirt
AntiTwitter: @DalekDave is now a follower!
|
|
|
|
|
There was no need of further arguments to convince me that RemoveAll() is most certainly (at least marginally) better!
I think I just... didn't think of it!
|
|
|
|
|
How about:
list.Where(x => !Remove(x));
Personally, I'd rename Remove to ShouldRemove or something, and, for the code above, invert it.
list.Where(x => !ShouldRemove(x));
or:
list.Where(x => ShouldKeep(x));
Optionally, follow up with a ToList() so the result is another list.
I rarely write loops like your original code, but save for the name of the Remove method, which doesn't remove anything, it's not weird or unreadable.
The way your co-worker managed to butcher it demands some respect though
|
|
|
|
|
I've always believed that coding is something that you have a 'knack' for. It's inherent. You either have it or you don't - and there's no point in wasting time on those that don't have it. You are never going to get them there.
The best thing you could do for your team-mate, is to explain to him that he's not very good at coding, (and probably never will be), and he should find himself a different career.
|
|
|
|
|
I posted this response to someone else in the thread, but OP might want to see it too.
--
In slight defence of the coder in question, selecting the data type that matches your data is actually good practice. eg you *can* store a date as a string;
"2020-07-22"
save it in SQL as a string, retrieve it and show it or parse it to DateTime if you need to add\remove days to it, convert back to string and save again. But we don't and I'm sure I don't need to explain why. Similarly people use decimal for currency when they really shouldn't. In the original code a List was used to process something that isn't best represented as a List, it was data used as a stack or a queue so using Queue makes that explicit. Also I don't think accessing the last item of a List using an index is very efficient.
The biggest failing in the code is obviously not just dequeuing regardless, that bit is silly and not very readable.
|
|
|
|
|
But there are going to be people approving his code and defending him.
Reminds me of a similar incident happened a few days back. I was fixing a few bugs on front end. Told this guy (Apparently my senior) I was facing 500 internal server error to which he responded 500 means bad request. You're doing something wrong.
Someone please tell him difference between error code 400 Bad Request and 500 Internal Server Error .
Anyways, I spent the next hour figuring out what was crashing on back-end so he could go and fix it.
The next day, turns out the boss was in the middle of a demo to a potential client when the crash happened. The rest was fun.
|
|
|
|
|
Yea, they did a similarly awful replacement recently.. argl.. oh well... people are nice and pay is good!
|
|
|
|
|
I'm sorry (no, actually I'm not) but the guy should be fired.
|
|
|
|
|
I wonder why they haven't many times already!
|
|
|
|