Improved exception messages.

Avoid magic numbers.
Improve test coverage.

Author: drieseng

src/Renci.SshNet/SshMessageFactory.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.Globalization;
+using System.Linq;
 using Renci.SshNet.Common;
 using Renci.SshNet.Messages;
 using Renci.SshNet.Messages.Authentication;
@@ -14,9 +15,19 @@ internal class SshMessageFactory
         private readonly MessageMetadata[] _enabledMessagesByNumber;
         private readonly bool[] _activatedMessagesById;
 
-        private static readonly MessageMetadata[] AllMessages;
+        internal static readonly MessageMetadata[] AllMessages;
         private static readonly IDictionary<string, MessageMetadata> MessagesByName;
 
+        /// <summary>
+        /// Defines the highest message number that is currently supported.
+        /// </summary>
+        internal const byte HighestMessageNumber = 100;
+
+        /// <summary>
+        /// Defines the total number of supported messages.
+        /// </summary>
+        internal const int TotalMessageCount = 31;
+
         static SshMessageFactory()
         {
             AllMessages = new MessageMetadata[]
@@ -54,15 +65,15 @@ static SshMessageFactory()
                 new MessageMetadata<KeyExchangeDhGroupExchangeReply> (30, "SSH_MSG_KEX_DH_GEX_REPLY", 33)
             };
 
-            MessagesByName = new Dictionary<string, MessageMetadata>(31);
+            MessagesByName = new Dictionary<string, MessageMetadata>(AllMessages.Length);
             foreach (var messageMetadata in AllMessages)
                 MessagesByName.Add(messageMetadata.Name, messageMetadata);
         }
 
         public SshMessageFactory()
         {
-            _activatedMessagesById = new bool[31];
-            _enabledMessagesByNumber = new MessageMetadata[101];
+            _activatedMessagesById = new bool[TotalMessageCount];
+            _enabledMessagesByNumber = new MessageMetadata[HighestMessageNumber + 1];
         }
 
         /// <summary>
@@ -76,14 +87,24 @@ public void Reset()
 
         public Message Create(byte messageNumber)
         {
-            var messageMetadata = _enabledMessagesByNumber[messageNumber];
+            if (messageNumber > HighestMessageNumber)
+            {
+                throw CreateMessageTypeNotSupportedException(messageNumber);
+            }
 
-            if (messageMetadata == null)
+            var enabledMessageMetadata = _enabledMessagesByNumber[messageNumber];
+            if (enabledMessageMetadata == null)
             {
-                throw new SshException(string.Format(CultureInfo.CurrentCulture, "Message type {0} is not valid.", messageNumber));
+                var definedMessageMetadata = AllMessages.FirstOrDefault(p => p.Number == messageNumber);
+                if (definedMessageMetadata == null)
+                {
+                    throw CreateMessageTypeNotSupportedException(messageNumber);
+                }
+
+                throw new SshException(string.Format(CultureInfo.InvariantCulture, "Message type {0} is not valid in the current context.", messageNumber));
             }
 
-            return messageMetadata.Create();
+            return enabledMessageMetadata.Create();
         }
 
         public void DisableNonKeyExchangeMessages()
@@ -106,58 +127,89 @@ public void EnableActivatedMessages()
                 if (!_activatedMessagesById[messageMetadata.Id])
                     continue;
 
-                var enabledMessage = _enabledMessagesByNumber[messageMetadata.Number];
-                if (enabledMessage != null && enabledMessage != messageMetadata)
+                var enabledMessageMetadata = _enabledMessagesByNumber[messageMetadata.Number];
+                if (enabledMessageMetadata != null && enabledMessageMetadata != messageMetadata)
                 {
-                    throw new Exception("Message X is already enabled for Y");
+                    throw CreateMessageTypeAlreadyEnabledForOtherMessageException(messageMetadata.Number,
+                        messageMetadata.Name,
+                        enabledMessageMetadata.Name);
                 }
                 _enabledMessagesByNumber[messageMetadata.Number] = messageMetadata;
             }
         }
 
         public void EnableAndActivateMessage(string messageName)
         {
+            if (messageName == null)
+                throw new ArgumentNullException("messageName");
+
             lock (this)
             {
                 MessageMetadata messageMetadata;
 
                 if (!MessagesByName.TryGetValue(messageName, out messageMetadata))
                 {
-                    throw new Exception("TODO");
+                    throw CreateMessageNotSupportedException(messageName);
                 }
 
-                var enabledMessage = _enabledMessagesByNumber[messageMetadata.Number];
-                if (enabledMessage != null && enabledMessage != messageMetadata)
+                var enabledMessageMetadata = _enabledMessagesByNumber[messageMetadata.Number];
+                if (enabledMessageMetadata != null && enabledMessageMetadata != messageMetadata)
                 {
-                    throw new Exception("Message X is already enabled for Y");
+                    throw CreateMessageTypeAlreadyEnabledForOtherMessageException(messageMetadata.Number,
+                        messageMetadata.Name,
+                        enabledMessageMetadata.Name);
                 }
-                _enabledMessagesByNumber[messageMetadata.Number] = messageMetadata;
 
+                _enabledMessagesByNumber[messageMetadata.Number] = messageMetadata;
                 _activatedMessagesById[messageMetadata.Id] = true;
             }
         }
 
         public void DisableAndDeactivateMessage(string messageName)
         {
+            if (messageName == null)
+                throw new ArgumentNullException("messageName");
+
             lock (this)
             {
                 MessageMetadata messageMetadata;
 
                 if (!MessagesByName.TryGetValue(messageName, out messageMetadata))
                 {
-                    throw new Exception("TODO");
+                    throw CreateMessageNotSupportedException(messageName);
                 }
 
-                _activatedMessagesById[messageMetadata.Id] = false;
+                var enabledMessageMetadata = _enabledMessagesByNumber[messageMetadata.Number];
+                if (enabledMessageMetadata != null && enabledMessageMetadata != messageMetadata)
+                {
+                    throw CreateMessageTypeAlreadyEnabledForOtherMessageException(messageMetadata.Number,
+                        messageMetadata.Name,
+                        enabledMessageMetadata.Name);
+                }
 
-                var enabledMetadata = _enabledMessagesByNumber[messageMetadata.Number];
-                if (enabledMetadata != null && enabledMetadata != messageMetadata)
-                    throw new Exception();
+                _activatedMessagesById[messageMetadata.Id] = false;
                 _enabledMessagesByNumber[messageMetadata.Number] = null;
             }
         }
 
-        private abstract class MessageMetadata
+        private SshException CreateMessageTypeNotSupportedException(byte messageNumber)
+        {
+            throw new SshException(string.Format(CultureInfo.InvariantCulture, "Message type {0} is not supported.", messageNumber));
+        }
+
+        private SshException CreateMessageNotSupportedException(string messageName)
+        {
+            throw new SshException(string.Format(CultureInfo.InvariantCulture, "Message '{0}' is not supported.", messageName));
+        }
+
+        private SshException CreateMessageTypeAlreadyEnabledForOtherMessageException(byte messageNumber, string messageName, string currentEnabledForMessageName)
+        {
+            throw new SshException(string.Format(CultureInfo.InvariantCulture,
+                "Cannot enable message '{0}'. Message type {1} is already enabled for '{2}'.",
+                messageName, messageNumber, currentEnabledForMessageName));
+        }
+
+        internal abstract class MessageMetadata
         {
             protected MessageMetadata(byte id, string name, byte number)
             {
@@ -175,7 +227,7 @@ protected MessageMetadata(byte id, string name, byte number)
             public abstract Message Create();
         }
 
-        private class MessageMetadata<T> : MessageMetadata where T : Message, new()
+        internal class MessageMetadata<T> : MessageMetadata where T : Message, new()
         {
             public MessageMetadata(byte id, string name, byte number)
                 : base(id, name, number)