3
\$\begingroup\$

I am unsure if the list could store a large number of messages.

There are 50,000 queue messages which I am receiving and assigning to the messages list:

    var msgEnumerator = msgQueue.GetMessageEnumerator2();
    var messages = new List<System.Messaging.Message>();
    while(msgEnumerator.MoveNext(new TimeSpan(0, 0, 1))) {
    var msg = msgQueue.ReceiveById(msgEnumerator.Current.Id, new TimeSpan(0, 0, 1));
    messages.Add(msg);
     }

    foreach( var k in messages){
    MailMessage mailM = (k.Body as SerializeableMailMessage).GetMailMessage();

    try {
    SmtpClient sp = new SmtpClient(smtpip, 25);
    sp.EnableSsl = false;
    sp.Send(mailM);
    }

    catch (Exception ex){
    logger.ErrorException("General error sending email.", ex);
    }
   }

Is this the correct way, or is there an alternative? Let me hear your suggestions.

\$\endgroup\$
4
\$\begingroup\$
  1. You create a lot of TimeSpan objects which is unnecessary. Just create one timeout variable. Especially since it's used in multiple places. This makes changing the timeout later easier. Otherwise you are bound to forget one place.
  2. Prefer TimeSpan.FromXYZ() methods. They makes it more obvious for what period you create a TimeSpan.
  3. Consider moving the mail sending code into its own function which just accepts a single message and sends it.
  4. It might make sense to iterate over the message queue directly instead of buffering them all up:

    var timeout = TimeSpan.FromSeconds(1);
    var queueIter = msgQueue.GetMessageEnumerator2();
    while (queueIter.MoveNext(timeout))
    {
        using (var message = msgQueue.ReceiveById(queueIter.Current.Id, timeout))
        {
            SendMessage(message);
        }
    }
    
  5. Instead of directly sending the message consider making an event available on the class like MessageReceived. Then other code can subscribe to it and do stuff (like sending it as mail or logging it or saving it somewhere, etc).
  6. As mentioned in the comment you should wrap all objects you create which are IDisposable into using statements to make sure any allocated resources are cleaned up properly.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.