Refactoring for SOLID principles

Question

After reading about SOLID code in a book and in an online article, I wanted to refactor an existing class so that it can be 'SOLID'-compatible.

But I think I got lost, especially with the dependency injection: when I wanted to instantiate an object of the class, I needed to 'inject' the all the dependencies, but the dependencies themselves have dependencies.. this is where I started getting lost.

The Idea is like this: I want to create a class (In my case, a simple Amazon S3 wrapper class) to di simple upload & get URL actions.

How can I correctly use interfaces and dependency injection? what went wrong? How should the class look like?

here is my code:

public interface IConfigurationProvider
{
    string GetConfigurationValue(String configurationKey);
}

public interface ILogger
{
    void WriteLog(String message);
}

public interface IAWSClientProvider
{
    AmazonS3Client GetAmazonS3Client();
}

public interface IAWSBucketManager
{
    string GetDefaultBucketName();
}

public class AWSBucketManager : IAWSBucketManager
{
    ILogger logger;
    IConfigurationProvider configurationProvider;

    public AWSBucketManager(ILogger Logger, IConfigurationProvider ConfigurationProvider)
    {
        logger = Logger;
        configurationProvider = ConfigurationProvider;
    }

    public string GetDefaultBucketName()
    {
        string bucketName = string.Empty;

        try
        {
            bucketName = configurationProvider.GetConfigurationValue("Amazon_S3_ExportAds_BucketName");
        }
        catch (Exception ex)
        {
            logger.WriteLog(String.Format("getBucketName : Unable to get bucket name from configuration.\r\n{0}", ex));
        }

        return bucketName;
    }
}

public class AWSClientProvider : IAWSClientProvider
{
    IConfigurationProvider configurationProvider;
    IAWSBucketManager awsBucketManager;
    ILogger logger;

    private string awsS3BucketName;
    private Dictionary<string, RegionEndpoint> regionEndpoints;

    public AWSClientProvider(IConfigurationProvider ConfigurationProvider, IAWSBucketManager BucketManager, ILogger Logger)
    {
        logger = Logger;
        configurationProvider = ConfigurationProvider;
        awsBucketManager = BucketManager;
    }

    private RegionEndpoint getAWSRegion()
    {
        RegionEndpoint regionEndpoint = null;
        // Init endpoints dictionary
        try
        {
            IEnumerable<RegionEndpoint> regions = RegionEndpoint.EnumerableAllRegions;
            regionEndpoints = regions.ToDictionary(r => r.SystemName, r => r);
        }
        catch (Exception Ex)
        {
            logger.WriteLog(String.Format("getAWSRegion() - Failed to get region list from AWS.\r\n{0}", Ex));
            throw;
        }
        // Get configuration value
        try
        {
            string Config = configurationProvider.GetConfigurationValue("Amazon_S3_Region");
            if (String.IsNullOrEmpty(Config))
            {
                throw new Exception("getAWSRegion() : Amazon_S3_Region must not be null or empty string.");
            }

            regionEndpoint = regionEndpoints[Config];
        }
        catch (Exception Ex)
        {
            logger.WriteLog(String.Format("getAWSRegion() : Unable to get region settings from configuration.\r\n{0}", Ex));
            throw Ex;
        }

        return regionEndpoint;
    }

    private AWSCredentials getAWSCredentials()
    {
        string accessKey, secretKey;
        BasicAWSCredentials awsCredentials;

        try
        {
            accessKey = configurationProvider.GetConfigurationValue("Amazon_S3_AccessKey");
            secretKey = configurationProvider.GetConfigurationValue("Amazon_S3_SecretKey");
        }
        catch (Exception Ex)
        {
            logger.WriteLog(String.Format("getAWSCredentials() - Unable to get access key and secrey key values from configuration.\r\n", Ex.Message));
            throw;
        }

        try
        {
            awsCredentials = new BasicAWSCredentials(accessKey, secretKey);
        }
        catch (Exception Ex)
        {
            logger.WriteLog(String.Format("getAWSCredentials() - Unable to create basic AWS credentials object.\r\n{0}", Ex.Message));
            awsCredentials = null;
            throw;
        }

        return awsCredentials;
    }

    public AmazonS3Client GetAmazonS3Client()
    {
        AmazonS3Client client = null;
        RegionEndpoint region = getAWSRegion();
        AWSCredentials credentials = getAWSCredentials();
        awsS3BucketName = awsBucketManager.GetDefaultBucketName();

        if (credentials != null)
        {
            client = new AmazonS3Client(credentials, region);
        }
        return client;
    }
}

public class AWSS3Actions
{
    IConfigurationProvider configurationProvider;       // decoupling getting configuration
    ILogger logger;                                     // decoupling logger
    IAWSClientProvider awsClientProvider;

    private const int defaultExpirationDays = 14;

    public AWSS3Actions(IConfigurationProvider ConfigurationProvider, ILogger Logger, IAWSClientProvider ClientProvider)
    {
        configurationProvider = ConfigurationProvider;
        logger = Logger;
        awsClientProvider = ClientProvider;
    }

    #region Private Mmethods

    private string getFileUrl(string fileName, int expirationDaysPeriod, string awsS3BucketName)
    {
        GetPreSignedUrlRequest request = new GetPreSignedUrlRequest();
        string URL = "";

        DateTime dtBase = new DateTime();
        dtBase = DateTime.Now;
        dtBase = dtBase.AddDays(expirationDaysPeriod);

        request.BucketName = awsS3BucketName;
        request.Key = fileName;
        request.Expires = dtBase;

        try
        {
            URL = awsClientProvider.GetAmazonS3Client().GetPreSignedURL(request);
        }
        catch (AmazonS3Exception ex)
        {
            // log
            logger.WriteLog(String.Format("getFileUrl() : Could not get presigned URL for the provided request.\r\n{0}", ex));
            throw ex;
        }

        return URL;
    }

    private int getDefaultURLExpiration()
    {
        int expirationDays = 0;
        try
        {
            // set the time span in days
            int.TryParse(configurationProvider.GetConfigurationValue("getDefaultURLExpiration() : Amazon_S3_ExportAds_ExpirationDaysOfURL"), out expirationDays);     // get from configuration util
        }
        catch
        {
            // in case of exception, set the min 14 days time space exiration
            expirationDays = defaultExpirationDays;
        }
        return expirationDays;
    }

    private void validateUpload(string fileName, Stream fileStream)
    {
        if (fileName == null || fileName.Equals(string.Empty) || fileStream.Length < 1)
        {
            throw new Exception("fileName : File name must not be an empty string.");
        }
        if (fileStream == null)
        {
            throw new Exception("fileStream : Input memory stream (file stream) must not be null.");
        }
    }

    #endregion

    #region Public methods

    public bool IsFileExists(string fileName, string awsS3BucketName)
    {
        bool fileExists = false;
        try
        {
            S3FileInfo fileInfo = new S3FileInfo(awsClientProvider.GetAmazonS3Client(), awsS3BucketName, fileName);
            fileExists = fileInfo.Exists;
        }
        catch (AmazonS3Exception Ex)
        {
            // log
            logger.WriteLog(String.Format("isFileExists() : Could not determine if file (key) exists in S3 Bucket.\r\n", Ex.Message));
            throw;
        }
        return fileExists;
    }

    public bool UploadObject(string fileName, Stream fileStream, string awsS3BucketName)
    {
        bool uploadResult = true;
        // Validate input parameters
        validateUpload(fileName, fileStream);

        if (awsClientProvider.GetAmazonS3Client() != null)
        {
            try
            {
                PutObjectRequest request = new PutObjectRequest
                {
                    BucketName = awsS3BucketName,
                    Key = fileName,
                    InputStream = fileStream
                };

                PutObjectResponse response = awsClientProvider.GetAmazonS3Client().PutObject(request);
                if (response != null)
                {
                    if (response.HttpStatusCode != System.Net.HttpStatusCode.OK)
                    {
                        var meta = response.ResponseMetadata.Metadata.Keys.
                            Select(k => k.ToString() + " : " + response.ResponseMetadata.Metadata[k]).
                            ToList().Aggregate((current, next) => current + "\r\n" + next);

                        // log error
                        logger.WriteLog(String.Format("Status Code: {0}\r\nETag : {1}\r\nResponse metadata : {1}",
                            (int)response.HttpStatusCode, response.ETag, meta));

                        // set the return value
                        uploadResult = false;
                    }
                }
            }
            catch (AmazonS3Exception ex)
            {
                uploadResult = false;

                if (ex.ErrorCode != null && (ex.ErrorCode.Equals("InvalidAccessKeyId") || ex.ErrorCode.Equals("InvalidSecurity")))
                {
                    // LOG
                    logger.WriteLog(String.Format("UploadObject() : invalied credentials"));
                    throw ex;
                }
                else
                {
                    // LOG
                    logger.WriteLog(String.Format("UploadObject() : Error occurred. Message:'{0}' when writing an object", ex.Message));
                    throw ex;
                }
            }
        }
        else
        {
            throw new Exception("UploadObject() : Could not start object upload because Amazon client is null.");
        }

        return uploadResult;
    }

    public bool UploadObject(string subFolderInBucket, string FileName, Stream fileStream, string awsS3BucketName)
    {
        return UploadObject(subFolderInBucket + @"/" + FileName, fileStream, awsS3BucketName);
    }

    public string GetURL(string fileName, string bucket)
    {
        string url = string.Empty;
        try
        {
            if (IsFileExists(fileName, bucket))
            {
                url = getFileUrl(fileName, getDefaultURLExpiration(), bucket);
            }
        }
        catch (Exception Ex)
        {
            // log
            logger.WriteLog(String.Format("getURL : Failed in isFileExists() method. \r\n{0}", Ex.Message));
        }
        return url;
    }

    #endregion
}

Answer 1

With your current class structure, the Composition Root might look like this:

var logger = new FileLogger("c:\\temp\\log.txt");
var configurationProvider = new ConfigurationProvider();

var actions = new AWSS3Actions(
    configurationProvider,
    logger,
    new AWSClientProvider(
        configurationProvider,
        new AWSBucketManagerlogger(
            logger,
            configurationProvider),
        logger));

The above example shows a hand-wired object graph (a.k.a. Pure DI). My advice is to start off by applying Pure DI and switch to a DI library (such as Simple Injector, Autofac or StructureMap) when building up object graphs by hand becomes cumbersome and maintenance heavy.

From perspective of Dependency Injection, what you're doing seems sane, although your code smells. Here are some references to look at:

• Logging and the SOLID principles
• How to design a logger abstraction

Side note: In general it's better to load configuration values up front (at application start-up), instead of reading it at runtime. Reading those values at runtime, causes these values to be read in delayed fashion, which prevents the application from failing fast, and it spreads the use of the configuration abstraction throughout the application. If possible, inject those primitive configuration values directly into the constructor of the type that requires that value.