• Guest (unregistered)

    And then Krista probably got yelled at for changing code without fixing a bug or implementing a new feature, which costs time and creates merge conflicts...

  • mitch (unregistered)

    As the whole body consists of "Exported files attached", using ASCII is totally fine. "IsBodyHtml = true" looks more dubious.

  • (nodebb)

    Just to expand on this, sometimes it's good to define overloads which take varying number of arguments, specifically when the method is called frequently and performance is critical, and the allocation of the parameters array can be avoided. Not true in this case because sending emails is a network operation and worrying about array allocation is silly.

  • (nodebb) in reply to Mr. TA

    Came here to post this. Yes, overly relying onparams can creep up on you. It looks like laziness in this case, but it's also possible that the "senior" had heard of the allocation problem and avoided params everywhere without thinking why.

  • Me (unregistered) in reply to Applied Mediocrity

    Also quite possible they simply didn't want to use some "new-fangled" language feature or even wasn't allowed by a senior manager.

    In a previous life the software we used was an horrible and obscure language, that was (and still is) years behind .NET and other languages in terms of language features it supports. Nevertheless, it did support a few features that weren't used in the software when I was there (like static variables!) and every now and again a new version of the dev tool would emerge that would support some amazing new feature like "exceptions" (gosh!).

    But actually using any of these new features when I developed new software was a serious struggle at first, at a time when a certain senior technical CEO liked to keep a close eye on things. Among the reasons I was given why I could use a new thing like proper exception handling (as opposed to functions that returned -1 on error) were: a) "we've never used that before, so can't start using it now", b) "other developers won't understand it" c) "it might not work" (as in, that language feature may not actually work in the dev tool).

  • Brian (unregistered)

    Sounds like this "senior" dev has a lot of experience in CPP - copy/paste programming, that is. This is the bane of my current workplace, as well. The general rule for adding new code is to find something similar that already works and copy it. No need to waste time thinking about about whether it's actually the right solution in this case, or maybe if some refactoring and abstraction is in order. Nope, gotta keep that sprint burndown looking good.

  • Foo AKA Fooo (unregistered)

    *due

  • dpm (unregistered) in reply to Applied Mediocrity

    If he really wanted to avoid "params", he could have simply

    private void SendEmail(ExportData exportData, String subject, String fileName1, String fileName2 = null, String fileName3 = null, String fileName4 = null, String fileName6 = null)

    and so on.

  • (nodebb) in reply to dpm

    Optional positioned or named parameters? That's sacrilege! According to C# team prior to version 4

  • (nodebb) in reply to Mr. TA

    Have you ever tried to call MS Office Interop from C# prior to version 4? That's what pressured them into doing it. Their own company had a major technology that was easy to use from every except C#.

    I've actually written VB.Net dlls that existed simply as a pass through to call MS Office.

  • MiserableOldGit (unregistered)

    Doing it properly would have you lined up and shot for premature optimisation.

    I've worked with developers (including "senior" ones and "head" ones) who code this way, they are consistently busy copy-pasta-ing new flags and file attachment fields and whatnot into their sprawling, but brittle, code spaghetti. Guess what, they look busy and responsive to business needs. I, on the other hand, look lazy because I'm able to say "Another file? just attach it, the system will take it, let me know if you have a problem" or "A new flag? go into the config screen and add it to the list, 1st line can help you if ... oh, nevermind, what is it? ... there, done it!".

    I have an ex-boss who basically gold-plated his retirement by splatting out software around the planet that needed him to go and run a couple of commands once a year. I used to get sent to the places he didn't want to go to, basically anywhere without a coastline. I used to patch the problems so I wouldn't need to revisit unless there was something drastic to change. This is why good programmers don't get rich, but bad ones do.

  • (nodebb)

    paramarray is for lazy people who can't be bother to define the array manually. Which is a direct result of the axioms of weak-typed languages to reduce time spent programming in favor of time spent debugging.

  • kdgregory (unregistered)

    Sending emails is positively a walk in the park compared to retrieving them and extracting something useful.

  • (nodebb) in reply to MiserableOldGit

    Guess what, they look busy and responsive to business needs

    Where I work, the developers are split into two camps.

    Camp one is the "old guard" and has no work definition or testing processes to speak of. They are really busy, work 10 tickets a day, and deploy three things every day. Unfortunately, 95% of their work is fixing bugs they've recently introduced.

    Camp two is the people that have been hired because the business is tired of the bugs. Camp two can't write an agile story to save their lives, so they get stuck in testing forever and never release anything.

    Camp one is using Camp two's inability to produce to show that their way is the right way and the bugs are simply because "IT is hard" and not really their fault. Unfortunately, leadership sees their argument as very convincing.

  • Wrath of Bubba (unregistered)

    I had an interview where I had to email notifications. What I proposed was to expose an API, and handle notifications from a send mail platform. My only responsibility was to expose data to the mail team which would be handled by marketing.

  • (nodebb)

    MailMessage.CC.Add lets you pass a list of comma separated addresses. He just needs to change the ; to ,

  • MiserableOldGit (unregistered) in reply to Jaime

    oh yuk, hard to say which is worse, but I suppose camp 2 could be "led" to sanity. There's probably no easy way out, in that kind of situation, even where you've managed to hire a good team, camp one have years of favours to call in all over the organisation ... in a phrase we used a lot in a place a few years ago "they know where all the bodies are buried". Once they see their entrenched position threatened your life becomes hell on earth, and the project is a loser unless management really are genuine about fixing the problem, they usually get scared.

  • Chris (unregistered)

    Am I the only who thinks it's weird that they have a "ExportData" object, which seems to just hold all the information they need to grab and stuff into a MailMessage object, along with potentially a bunch of file attachments? From what we're given here, which is not the whole picture, it looks like they could cut the middle man out perhaps.

  • David Mårtensson (unregistered) in reply to Chris

    As in sending in the mail object directly?

    That is not a good practise, what if you decide to change from creating a mail object and use an external mailer that uses an http call to send the data over.

    Then your stuck pushing mail objects around your code but never use them for what they are intended for, and your out of luck if you need to add some param.

    The mail object is an internal implementation detail of the send function/method and should not be exposed.

    Even if you continue to send emails you might switch component for it to one that does not use MailMessage.

    I actually have seen this in one product that started out using MailMessage but later switched to a more feature rich component but the code was built around MailMessage so it created those and then converted those into the new Mail object class.

    This became a real headache later when switching to .net Core as everything had to be rebuilt, not just ported.

  • Quirkafleeg (unregistered)

    TRWTF is a company that hired so many developers who missed the school/college/university lesson "What is an array, and what can it be used for?"

  • MiserableOldGit (unregistered)

    I understand some people are a bit resistant to using paramarray, one of those things junior developers "discover" one day and then dreate some hideous monstrosity the next time they code by overusing it.

    Still, refusing to use it in favour of this is a WTF.

    Personally I'd have gone with some sort of collection or library, then you are not constraining yourself to just passing (what I assume is) some sort of path.

    And the fileexists test smells of a race condition where the email goes out with or without the attachment, depending on whether it got in the right folder quick enough!

  • (nodebb)

    And the fileexists test smells of a race condition

    I agree. I recently saw a case where the component a developer was using to create an attachment hadn't released a file lock on it by the time the email component went to read it.

    Personally, I make every attempt possible to pass attachment data as streams or byte arrays.

  • WTFGuy (unregistered)

    @Chris & @David Mårtensson

    Perhaps SendEmail() ought to be a method on the ExportData object. Along with potentially other SendViaWhatever() methods. But if so then the ExportData object would need fields for all the necessary content info. Such as .Subject and .AttachmentStreams() or .AttachmentFileNames().

    Actually, the ExportData object itself is really misnamed; it contains (as far as we can see) a list of export recipients, but not a single byte of export data; nor even metadata such as of filename(s) of the data to be exported. So an ExportData object might better be an ExportRecipients or an ExportRequest or an ExportRouting depending on what other fields it may already contain.

    Not WTF-worthy, but less than great naming choices. As always we need to remember the name was chosen back at v0.5 and we're looking at v 27.12 in an work environment apparently never designed for continuous refactoring and improvement. A lot of mucky polluted water has passed under the bridge in the mean time.

  • hartmut holzgraefe (google)

    IMHO every auto generated mail should have at least one custom X-.....: header that clearly identifies by what process it was generated, so that it can be filtered by a rule matching that specific header, and not having to rely on Subject:, From:, or worst of all: some magic cookie string in the mail body.

  • (nodebb)

    Yes, I've spent too much time figuring out the generator of an email from spelling errors and formatting inconsistencies.

  • (nodebb) in reply to Jaime

    Yes, I've spent too much time figuring out the generator of an email from spelling errors and formatting inconsistencies.

    Grepping the codebase for text strings (either directly or through a properties file)? I feel you. This is a case to be very glad not to have self modifying code.

  • Igor (unregistered) in reply to mitch

    This due to many email clients using courier new as a font for the message when the html flag is set to false.

  • jochem (unregistered)

    I'm missing the 'using' block. Else, In the case an exception is thrown the dispose method will not be called.

Leave a comment on “Carbon Copy”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article