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 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!
At work I have a team mate.... (nothing ground breaking so far), of which I have a certain opinion I shall keep to myself...
Anyway his latest antic is that he found this code confusing!
staticvoid FilterList(List<Foo> list)
for (int i = list.Count - 1; i >= 0; i--)
And he shared with us his improved version!
staticvoid FilterQueue(List<Foo> list)
var q = new Queue<Foo>(list);
for (int N = q.Count; N >0; N--)
needless to say, I am not impressed...
But more than I am .. speechless... where did he come up with that from?!
And before you suggest it, the change has nothing to do with possible performance enhancement (beside it's slower for small list like the one I am using in that piece of code and business data), he find his alternate code simpler to read and understand somehow!
Wow, mind blown...
Anyway, I should not get so upset and dismissive.. maybe he is learning, this time?
I doubt it and he is kind beyond hope at this stage.. but one never know!
But someone in your company or group will surely defend the code. I mean who is to say?
To each his own. And all that blather. No one can just tell anyone no now, it seems.
Years ago, I noticed a contractor wrote his own parsing routines for XML.
I said, "why didn't you use the built-in .NET XML library? Now you've created an entirely proprietary API that everyone must learn to parse the XML." Contractor: "Oh, I was going to use Microsoft's but mine is like 100 times faster." Me: "What? So you've written better code than the people at Microsoft? Wow, they don't knwo what they're missing." Contractor: "Well it is faster. There's is slow."
I examined the code closer and learned that it was straight-up parsing -- not converting to the XML DOM. That meant that you had to parse the entire XML string every time you wanted update a value in the XML. Whereas with the XML DOM you could just set the value.
I tried to explain, but he didn't know what the XML DOM was.
My dev manager stepped out of his office into our area. "what is all the noise?"
I explained that contractor created a proprietary API that was now stuck inside of company code. Dev Manager: "Listen, keep it down. I'm sure [Contractor] has done the right thing. It's not a big deal."
The company would be tied to that proprietary code for many years.
Oh well, as long as we didn't make noise arguing. That's what is important.
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.