The Lounge is rated Safe For Work. If you're about to post something inappropriate for a shared office environment, then don't post it. No ads, no abuse, and no programming questions. Trolling, (political, climate, religious or whatever) will result in your account being removed.
I once worked at a little web company that had one developer besides me. He had worked there for years, and he wasn't untalented. He was simply self-taught. His front end code did what JQuery did, but it was proprietary! His backend code was PHP but it used his own framework, and worse, he was doing joins and row filters in PHP instead of in the database.
I ended up quitting because I could not code to an undocumented proprietary framework that had been built up for years. As I understand it previous developers were fired.
1) FilterQueue but takes a List? Why not just name it Filter (for both)? The parameter gives you more consistent meaning in the face of changes.
2) For loop but never uses the iterator variable? I know here it's used to lock-in the initial size so re-enqueueing doesn't cause infinite recursion but it just looks bad imo. Could easily be made to use a while loop.
3) Dequeue in both if-else blocks but uses Peek for the test case anyways?
Example fix for #1-#3:
staticvoid Filter(List<Foo> list)
Queue<Foo> queue = new Queue<Foo>(list);
Foo item = null;
while (item = queue.Dequeue())
Still not great as you no doubt know. It unnecessarily involves the Queue and complicates the code for no gain over the original list-only code. If I was to offer something that imo looks "cleaner" it would be:
staticvoid Filter(List<Foo> list) =>
list.RemoveAll(item => IsRemovable(item)); //I agree with Dave on IsRemovable over Remove.
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.
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!
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;
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 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.
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.
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
Last Visit: 29-Sep-20 8:37 Last Update: 29-Sep-20 8:37