Just going off the top of my head: This much work should not be happening in a button's click event handler. It should be its own method (or more than one!) ca

Just going off the top of my head: This much work should not be happening in a button's click event handler. It should be its own method (or more than one!) called by the click handler. For that matter, data access should be handled by a DAO, front end code should be calling a service or gateway rather than going to the DB itself. Way too much is happening in GUI code. In a "real" app, the connection string would be configured in some way, not just a local variable. The way the query is written is wide open to SQL injection. Nobody should be writing SQLI vulns in this day and age, ever. In .NET land, if you're still concatenating queries instead of using query parameters, you should go home because you are bad at your job. The list could go on. Some of this can be excused by the fact that it's from an interviewee and not part of a "real" app. But the SQLI is just a huge red flag, to me. One thing they did get right: wrapping the connection object in a "using" block so it is guaranteed to be disposed of correctly.