|
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!
static void FilterList(List<Foo> list)
{
for (int i = list.Count - 1; i >= 0; i--)
{
if (Remove(list[i]))
list.RemoveAt(i);
}
}
And he shared with us his improved version! 
static void FilterQueue(List<Foo> list)
{
var q = new Queue<Foo>(list);
for (int N = q.Count; N > 0; N--)
{
if (Remove(q.Peek()))
{
q.Dequeue();
}
else
{
q.Enqueue(q.Dequeue());
}
}
list.Clear();
list.AddRange(q);
}
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!
|
|
|
|
|
improved ? ... 
|
|
|
|
|
Yeah, I am glad we are both confused! 
|
|
|
|
|
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.
Voices raised.
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.
|
|
|
|
|
maybe they billed the hours to write this so called parsing function....rather than use msft one..
Caveat Emptor.
"Progress doesn't come from early risers – progress is made by lazy men looking for easier ways to do things." Lazarus Long
|
|
|
|
|
abmv wrote: maybe they billed the hours to write this so called parsing function
Oh, they definitely did. 
|
|
|
|
|
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. 
Real programmers use butterflies
|
|
|
|
|
Great story of how things really work out there at a lot of places.
honey the codewitch wrote: e was doing joins and row filters in PHP instead of in the database
So terrible! I've seen this kind of thing via numerous languages. So much code to do what could be done so much simpler in DB. 
These people are often seen as geniuses at those places too, because no one knows better.
All you can do is move on. RUN!!! 
|
|
|
|
|
Like I said, he wasn't untalented - I actually went to highschool with him, and we were both coding. He ended up getting named on a patent for some of the code he wrote later.
It's just he was self taught and so he didn't understand the processes behind team coding or really business coding, and there were gaps in his understanding of technology as well.
I say that as someone who was self taught as well, and had to learn all that the hard way.
Real programmers use butterflies
|
|
|
|
|
He is putting you on. That's the only explanation. His routine would not survive code review in any shop I've worked at, no offense.
Real programmers use butterflies
|
|
|
|
|
I am not the one who should be offended! 
He tested my patience many times... And I became a better person for it!
But, clearly, it is not intentional just... he is not very good (I think)
Other than that I sometimes think that his ineptitude is what gave me the opportunity to get a well paid contractor position... Because the company might now be reluctant to hire permanent worker now!
Well, that's how I put a positive spin to it!
|
|
|
|
|
honey the codewitch wrote: He is putting you on
I didn't think that expression was still in use, I hadn't heard it for about forty years. 
|
|
|
|
|
/hipster anachronism
Real programmers use butterflies
|
|
|
|
|
|
That's the "improved" version? Improved how? I just don't see it.
About the only thing I would improve in the original code is changing the method called by the if expression to:
if (IsRemovable(list[i]))
Of course, I'm making assumptions about what Remove does.
|
|
|
|
|
Yes, IsRemovable would be a good idea!
It was just a test / prototype thought!
|
|
|
|
|
Hmmmm...
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:
static void Filter(List<Foo> list)
{
Queue<Foo> queue = new Queue<Foo>(list);
list.Clear();
Foo item = null;
while (item = queue.Dequeue())
{
if (!Remove(item))
list.Add(item);
}
}
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:
static void Filter(List<Foo> list) =>
list.RemoveAll(item => IsRemovable(item));
But that's just a personal preference.
EDIT: A word to clarify.
modified 22-Jul-20 1:57am.
|
|
|
|
|
Nicer code!
But then, anyway, why use that code anyway!^_^
|
|
|
|
|
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.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
