- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
"Every choice made here was a bad choice." - I think I spotted at least one line in that code that was a good choice <<grin>>
Admin
found another one: 'allMachine' should be 'allMachines' i'm not trying to be picky - it just shows such a deep ignorance.
Admin
Oh, and let's not miss the fact that
machineIdNotExistsistrueif the MachineID DOES exist.Admin
If only there was a way to generate a unique automatically incrementing number in the database that could be used as a primary key for things...
Admin
"Easy Reader Version: Honestly, it's the data grid control in use in the year 2022 that really irks me- this seems like it's a WebForms application in use at this late date."
You know, I think that WebForms did have the right spirit of things, even if the implementation was just awful. When you're developing these intranet CRUD systems, it's very, very convenient if you have a good library of reusable components that all look and behave the same way everywhere. Grids in particular, but also edit forms, date pickers etc.
20 years later we've come a full circle and we have something like them fully on the client side - React, Vue, and others. They're more universal and flexible, but it's the way that it should have been done from the start. Add websockets and a live "session" on the server side (is that being done already? I think I saw that somewhere...), and you have a pretty comfortable and secure framework both for users and developers.
Admin
I have always been here
I will always look out from behind these eyes
It's only a lifetime
It's only a lifetime
It's only a lifetime
Admin
I want to know how many machines each customer could reasonably be expected to have. No wait, this is checking it against all machines in the database, not just those with a matching customer ID.
I would not be surprised if someone determined 3 digits wouldn't be enough, and told the developer to use 6 instead, hence the "999" padding.
Admin
There's also the quite minor issue that the variable "machineIdNotExists" is completely unnecessary . . . the loop could simply be a while(true) with a if (machineExists.Count == 0) break;. No else required.
Admin
Hey, they just did! Give them some credit. ;-)
Admin
Including hiring this developer.
Admin
Wow. So many WTFs in this code. Besides what is already mentioned in the code comments:
Admin
The performance is gone tank as soon as they hit the customer with 900 machines.
Admin
I can't. I just can't...
Admin
There are valid business reasons for non-sequential IDs and different ones for forcing a range of 999100-999999. While judging how this is accomplished can be done looking only at the code judging why can't be done without the original instructions.
I'm not saying it is impossible the resulting MachineID is ridiculous, just that that might not be the developer's fault.
Admin
I agree that the boolean isn’t necessary, but a while (true) loop is not the solution. I would consider a while (true) loop here an anti pattern as well.
Admin
This code also suffers from TOCTOU bug. If two threads run in parallel and happen to select the same random ID, it's quite likely one will overwrite the other one's record in the database; or, depending on how MachineManager.InsertMachine() is implemented, it might just fail the second one.
Unless of course the web application enforces single-threadedness with some other external locking or concurrency control, which would also be a WTF.
Admin
What if the Session variable near the top is null? Oops.
Admin
Stored procedures have never protected from SQL Injection. Parameterization instead of concatenation does. Sure, parameterization is more closely tied to the use of stored procedures. But, as this code demonstrates, the actual use of a stored procedure is quite insufficient.
I really wish the group of developers that really want to put all of the business logic in the database engine would simply try to argue their case instead of pretending that "doing it their way" is somehow "for SQL Injection protection". It reminds me of all of the misguided "for the children" demands out there in the world.
Admin
Function name: AddBlankRowToDatabase
After finding an unused id value in a specific table (not a generic or specified one), it then creates an entry and populates it with at least a machine title and a bunch of other boring properties.
Admin
Obviously in that work environment programmers were not allowed to do ANY direct database access or maintenance. All DB access was done by Stored Procedures (and maybe a few Views) created by the DBA gods. It probably also took weeks if not months to get them to make a business change. This whole thing is a travesty. Add a column MachineNum to the Customer table. Initialize it to 999000. When a new machine arrives bump the MachineNum, replace, commit (or surround in a transaction) and use that MachineNum for you new machine table. Perhaps 10 minutes work including initialization. BUT the gods don't allow it so mangle up a poor workaround. I was sooooooooooooooooooooo lucky that my final 7 year job had me doing all IT business systems work for a midsize (300+ employees) manufacturer. It was heaven on earth and I was the god. Everyone loved what I could do quickly and perfectly. I guess when the .... hits the fan I could always go back. So much fun.
Admin
Well, yes, but the obvious recourse there would be to construct a SQL query that (regrettably) pulls down the nought to a thousand machine ids. People obsess about dragging a whole dataset across the interface, but I can't see how it would hurt in this case.
At that point, you just muck around in C#. No stupid randomisation required. A First() in Linq would do. (FirstOrDefault jic you've run over the limit of 1000).
I can't see a plausible business purpose in ensuring that IDs are not sequential, although I've worked with many PHBs who might think that -- but even that is easy. You have a set in Linq of available empty slots. Use Random() for what it's intended to do -- pick a random empty slot.
The code as presented is .... the work of a maniac.
Admin
Paraphrasing Charles Babbage: I am not able to rightly apprehend the kind of confusion of ideas that could provoke such a construct.
Admin
I'm one of those developers, and I do. Easier to test, easier to reuse, easier to hotfix, and while parameters are what really provide injection protection, stored procedures make it natural to minimize use of dynamic SQL where injection has a chance to get a foothold. Our API layer is mostly limited to simple sanity checks like "required parameter value missing/invalid".
Admin
The public site for my main work project uses WebForms, but we started developing it in 2008, so yeah. The past few years have mostly been about replacing the much worse internal-maintenance desktop app with an internal web site (using at least somewhat newer components).
Maybe in another couple years, we'll have enough time available to start working on a newer-components version of the public site. At least we should be able to reuse much of the back end, but the front end is relatively complex. Would be a nice thing to work on, but oh boy would it take a while...
Admin
egads...
Admin
I'm trying to think if there's a fast way to generate a random integer without replacement, given that you have to release the set of unused numbers from memory each time. If you can can maintain the set of unused numbers in set/dictionary/map form, you can get everything done in O(1). However, if you have to rebuild that set every time, you're looking at O(n) to get a new one. If it doesn't need to be random, obviously O(1) via a counter is the best. Does SQL/SPs let you maintain some set/state?
Admin
Yes, databases have a concept of set/dictionary/map... they call it a "table". With any sensible database engine, a small frequently accessed table will always be in memory. They even have a very efficient way of streaming the changes back to disk in batches without destroying data integrity.
Admin
Really? Every time I move logic out of a data layer, it's easier to test. By "test", I don't mean "invoke this thing in production and see what it does".
I don't see how a store procedure is "easier to use" than a REST method hosted on a web endpoint. Both are simply code that exists in a single place and has a well-defined mechanism to communicate with it.
I also don't see "easier to hotfix". ALTER PROC isn't any easier that the many methods available on other technology stacks. There's plenty of hot-reload capabilities out there that allow you to change server code by saving a file if you are trying to keep things simple. If you have anything resembling a process, then fixing production code is usually as easy as checking in a fix and pushing a button to deploy.
Admin
You're correct, of course, but I was merely pointing out that the developer couldn't get even a tiny aspect of the code right.
Admin
I love the way they can't even do basic variable handling. Using strings to add 999000 to an integer - that says it all for me.
Admin
In fact it is much easier to test, especially when testing manually/not having a CI/CD pipeline. For example you can wrap your sproc in a transaction that you rollback, along with some SELECTs to validate the outputs. This allows even hotfixing in production, with a preliminary checks before 'committing' the change.
And in CI/CD pipeline is not a big friction when you're using automatic migration scripts approach, like Flywheel or DbUp.
And regardless of testing, stored procedures has the advantage that they can serve as a 'queries inventory', so is more easier to manage the performance, for example by carefully crafting the queries and indexes so that multiple queries can take advantage of reusing a smaller number of indexes. Also, makes it easier to manipulate schema under the hood, should a normalization change of tables being needed, when sprocs signature doesn't change.
Admin
Not all code runs on a web server. Consider machine control app that needs some flexibility after deployed. Much easier to adjust a stored procedure than modify code, compile and deploy a new version of the app binary to the customer. The customer may even do some changes themselves. SQL is a scripting engine after all :-)
Admin
That's a straw-man argument. Self-updating executables and/or stand-alone updaters are a things that every application maintainer has access to. The fact that many people struggle to roll out updated binaries is a testament to their incompetence, not a legitimate problem to be overcome.
Beside, if you need to "deploy a new version of the binary to the customer", then one of the two things below are true:
Your argument really comes down to: if you're still stuck in the 90's and are not motivated enough to figure out how to support a deployed application, then maybe stored procedures will solve your problems for you. Well, a portion of them, since I certainly hope you aren't defining your UI via stored procedure.
Admin
My argument really comes down to a simple fact that the world is a bit bigger and more diverse than what you suggest it is. There are environments where restarting a running app is measurable cost, because one must stop the manufacturing process controlled by the app. We found SQL a convenient scripting language that provides more flexibility than any kind of "configuration settings" alternatives, nearly the same as actual app code changes - in places, not everywhere, of course. There is no silver bullet, no solution that fits all use cases, and calling it "being not motivated enough to figure out" brings no benefit.
Admin
It's kind of a small thing compared to other issues, but use of Count to check for presence/absence instead of Any() always grates on me.