[seanhunter]

I once was in the position of becoming pr approver[1] for a team of outsourced python programmers who were under some pretty extreme deadline pressure. Anyway one day a PR comes in and I can’t help but notice it was doing a string eval.

Weird. You almost never need to do string eval in python, and whenever there is something where you think you need eval there is a better and safer way to achieve the same result.

Also, I was bending my brain but I couldn’t really figure out what this eval was for until I wrote out some scratch code myself to figure it out.

Turns out this 5 lines or so was constructing a string to do dict lookup and then evalling that. So say you have a dict d = {‘foo’: ‘bar’} and you have a variable i=foo and want to look up d[i], instead of just doing that it was doing something like

   eval(‘d[‘+i+’]’)

Just no.

So I rejected the change and they came back with “but we’ve always done it that way”. I grep the codebase and yes. There were about 200+ uses of eval, all of which were constructing a string to look something up in a dictionary and then evaling the result. Some person who clearly didn’t program in python had found this twisted way to look things up in a dictionary and then this team had dutifully copied and pasted it throughout the codebase.

[1] ie I wasn’t there from the start of the project

[JimDabell]

I saw plenty of similar things from outsourced teams.

For instance: An e-commerce API that used JSON. Not only did the spec. tell them to use integers with pence for prices, but it explicitly called out that it MUST NOT use floating points with pounds. Sure enough, they implemented it as floating point pounds. So we asked them to fix this.

The underlying datatype in the database was pounds in a decimal type. You would think that they would multiply this by 100 and call it a day. What they actually did was: a) render it as a string, b) strip the period character, and c) parse the resulting string as an integer. They didn’t test this properly before deploying, which resulted in us charging the correct price for things that cost £xxx.x5 but undercharging by a factor of ten for things that cost £xxx.x0 and undercharging by a factor of a hundred for things that cost £xxx.00.

[kryptocannon]

> So I rejected the change and they came back with “but we’ve always done it that way”. I grep the codebase and yes. There were about 200+ uses of eval

That's code review's worst! Happened to me many times.

[graemep]

I came across something quite similar. Using eval to convert a string to a dict. The string potentially included user input.

[seanhunter]

I wonder whether my dudes cut and pasted from the same cursed stack overflow snippet as your dudes had.

The strings here included user input too. Worse still, the situation was the company was offering a b2b service and the string didn’t just come from user input by an employee of the company they came from arbitrary customers of the customers of the company.

[graemep]

In my case the data was visible in the URL - they had chosen to not store use session specific data in the DB or cookie or anything sane like that, but to pass it to the page in the URL path by converting a dict to a string/

Git blame shows the same thing done in two different places and the line edited by at least two different people.

[franktankbank]

Yes! India? There is a very big follow the leader culture.

[baobun]

Rhymes with apocryphal monkey-ladder story.