Feed aggregator
India Grants Tax Officials Sweeping Digital Access Powers
Read more of this story at Slashdot.
CodeSOD: An Argument With QA
Markus does QA, and this means writing automated tests which wrap around the code written by developers. Mostly this is a "black box" situation, where Markus doesn't look at the code, and instead goes by the interface and the requirements. Sometimes, though, he does look at the code, and wishes he hadn't.
Today's snippet comes from a program which is meant to generate PDF files and then, optionally, email them. There are a few methods we're going to look at, because they invested a surprising amount of code into doing this the wrong way.
protected override void Execute() { int sendMail = this.VerifyParameterValue(ParamSendMail); if (sendMail == -1) return; if (sendMail == 1) mail = true; this.TraceOutput(Properties.Resources.textGetCustomerForAccountStatement); IList<CustomerModel> customers = AccountStatement.GetCustomersForAccountStatement(); if (customers.Count == 0) return; StreamWriter streamWriter = null; if (mail) streamWriter = AccountStatement.CreateAccountStatementLogFile(); CreateAccountStatementDocumentEngine engine = new CreateAccountStatementDocumentEngine(); foreach (CustomerModel customer in customers) { this.TraceOutput(Properties.Resources.textCustomerAccountStatementBegin + customer.DisplayName.ToString()); // Generate the PDF, optionally send an email with the document attached engine.Execute(customer, mail); if (mail) { AccountStatement.WriteToLogFile(customer, streamWriter); this.TraceOutput(Properties.Resources.textLogWriting); } } engine.Dispose(); if (streamWriter != null) streamWriter.Close(); }Now, this might sound unfair, but right off the bat I'm going to complain about separation of concerns. This function both generates output and emails it (optionally), while handling all of the stream management. Honestly, I think if the developer were simply forced to go back and make this a set of small, cohesive methods, most of the WTFs would vanish. But there's more to say here.
Specifically, let's look at the first few lines, where we VerifyParameterValue. Note that this function clearly returns -1 when it fails, which is a very C-programmer-forced-to-do-OO idiom. But let's look at that method.
private int VerifyParameterValue(string name) { string stringValue = this.GetParam(name, string.Empty); bool isValid = this.VerifyByParameterFormat(name, stringValue); if (!isValid) return -1; int value = -1; try { value = Convert.ToInt32(stringValue); } catch (Exception e) { this.TraceOutput(string.Concat("\n\n\n", e.Message, "\n\n\n")); return -1; } return value; }We'll come back to the VerifyByParameterFormat but otherwise, this is basically a wrapper around Convert.ToInt32, and could easily be replaced with Int32.TryParse.
Bonus points for spamming the log output with loads of newlines.
Okay, but what is the VerifyByParameterFormat doing?
private bool VerifyByParameterFormat(string name, string value) { string parameter = string.Empty; string message = string.Empty; if (value.Length != 1) { parameter = Properties.Resources.textSendMail; message = string.Format(Properties.Resources.textSendMailNotValid, value); this.TraceOutput(string.Concat("\n\n\n", message, "\n\n\n")); return false; } string numbers = "0123456789"; char[] characters = value.ToCharArray(); for (byte i = 0; i < characters.Length; i++) { if (numbers.IndexOf(characters[i]) < 0) { parameter = Properties.Resources.textSendMail; message = Properties.Resources.textSendMailNotValid; this.TraceOutput(string.Concat("\n\n\n", message, "\n\n\n")); return false; } } return true; }Oh, it just goes character by character to verify whether or not this is made up of only digits. Which, by the way, means the CLI argument needs to be an integer, and only when that integer is 1 do we send emails. It's a boolean, but worse.
Let's assume, however, that passing numbers is required by the specification. Still, Markus has thoughts:
Handling this command line argument might seem obvious enough. I'd probably do something along the lines of "if (arg == "1") { sendMail = true } else if (arg != "0") { tell the user they're an idiot }. Of course, I'm not a professional programmer, so my solution is way too simple as the attached piece of code will show you.
There are better ways to do it, Markus, but as you've shown us, there are definitely worse ways.
.comment { border: none; } [Advertisement] Plan Your .NET 9 Migration with ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!
Goldman Sachs: Why AI Spending Is Not Boosting GDP
Read more of this story at Slashdot.
Utah Passes First US App Store Age Verification Law
Read more of this story at Slashdot.
Amazon Tests AI Dubbing on Prime Video Movies, Series
Read more of this story at Slashdot.
Google is Adding More AI Overviews and a New 'AI Mode' To Search
Read more of this story at Slashdot.
Europe on Alert Over Suspected Sabotage of Undersea Cables
Read more of this story at Slashdot.
Nintendo Says Latest Legal Win Against Piracy 'Significant' For 'Entire Games Industry'
Read more of this story at Slashdot.
Could New Clocks Keep Airplanes Safe From GPS Jamming?
Read more of this story at Slashdot.
Half of World's CO2 Emissions Come From 36 Fossil Fuel Firms, Study Shows
Read more of this story at Slashdot.
Microsoft Warns of Chinese Hackers Spying on Cloud Technology
Read more of this story at Slashdot.
OpenAI Plots Charging $20,000 a Month For PhD-Level Agents
Read more of this story at Slashdot.
Apple Refreshes MacBook Air With M4 Chip, Lower Pricing
Read more of this story at Slashdot.
Google Urges DOJ To Reverse Course on Breaking Up Company
Read more of this story at Slashdot.
Turing Award Winners Sound Alarm on Hasty AI Deployment
Read more of this story at Slashdot.
NASA Uses GPS On the Moon For the First Time
Read more of this story at Slashdot.
World's First 'Synthetic Biological Intelligence' Runs On Living Human Cells
Read more of this story at Slashdot.
China May Be Ready to Use Nuclear Fusion for Power by 2050
Read more of this story at Slashdot.
CodeSOD: Wrap Up Your Date
Today, we look at a simple bit of bad code. The badness is not that they're using Oracle, though that's always bad. But it's how they're writing this PL/SQL stored function:
FUNCTION CONVERT_STRING_TO_DATE --Public (p_date_string IN Varchar2, p_date_format IN Varchar2 DEFAULT c_date_format) Return Date AS BEGIN If p_date_string Is Null Then Return Null; Else Return To_Date(p_date_string, p_date_format); End If; END; -- FUNCTION CONVERT_STRING_DATEThis code is a wrapper around the to_date function. The to_date function takes a string and a format and returns that format as a date.
This wrapper adds two things, and the first is a null check. If the input string is null, just return null. Except that's exactly how to_date behaves anyway.
The second is that it sets the default format to c_date_format. This, actually, isn't a terrible thing. If you check the docs on the function, you'll see that if you don't supply a format, it defaults to whatever is set in your internationalization settings, and Oracle recommends that you don't rely on that.
On the flip side, this code is used as part of queries, not on processing input, which means that they're storing dates as strings, and relying on the application layer to send them properly formatted strings. So while their to_date wrapper isn't a terrible thing, storing dates as strings definitely is a terrible thing.
[Advertisement] Plan Your .NET 9 Migration with ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!
Users Report Emotional Bonds With Startlingly Realistic AI Voice Demo
Read more of this story at Slashdot.