3
\$\begingroup\$

I wrote code to creating structure with files on hard disc. I used here Composite pattern. Please make code review to this.

using System.Collections.Generic;
using System.Text;

namespace Common.FileSystem
{
    public abstract class FileSystemItem
    {
        public FileSystemItem(string rootPath, string name)
        {
            if (rootPath == null)
                throw new ArgumentNullException("rootPath");

            if (name == null)
                throw new ArgumentNullException("name");

            RootPath = rootPath;
            Name = name;
        }

        public string Name { get; private set; }

        public string RootPath { get; private set; }

        public string Path
        {
            get { return System.IO.Path.Combine(RootPath, Name); }
        }

        public abstract void Create();

        public abstract Directory AddDirectory(string name);

        public abstract void AddFile(string name, IFileBuilder fileBuilder);

        public abstract void Remove(FileSystemItem item);

        public abstract string Print();

        private string _rootPath;
    }

    public class Directory : FileSystemItem
    {
        public Directory(string rootPath, string name) : base(rootPath, name) { }

        public override void Create()
        {
            System.IO.Directory.CreateDirectory(Path);
            foreach (var item in _items)
                item.Create();
        }

        public override Directory AddDirectory(string name)
        {
            var directory = new Directory(Path, name);
            _items.Add(directory);

            return directory;
        }

        public override void AddFile(string name, IFileBuilder fileBuilder)
        {
            var file = new File(Path, name, fileBuilder);
            _items.Add(file);
        }

        public override void Remove(FileSystemItem item)
        {
            _items.Remove(item);
        }

        public override string Print()
        {
            var stringBuilder = new StringBuilder();
            stringBuilder.AppendLine(Path);
            foreach (var item in _items)
                stringBuilder.Append(item.Print());

            return stringBuilder.ToString();
        }

        private List<FileSystemItem> _items = new List<FileSystemItem>();
    }

    public class File : FileSystemItem
    {
        public File(string rootPath, string name, IFileBuilder fileBuilder) : base(rootPath, name)
        {
            if (fileBuilder == null)
                throw new ArgumentNullException("fileBuilder");

            _fileBuilder = fileBuilder;
        }

        public override void Create()
        {
            var fileContent = _fileBuilder.Build();

            System.IO.File.WriteAllText(Path, fileContent);
        }

        public override Directory AddDirectory(string name)
        {
            throw new InvalidOperationException("Operation is unavailable for this object");
        }

        public override void AddFile(string name, IFileBuilder fileBuilder)
        {
            throw new InvalidOperationException("Operation is unavailable for this object");
        }

        public override void Remove(FileSystemItem item)
        {
            throw new InvalidOperationException("Operation is unavailable for this object");
        }

        public override string Print()
        {
            return Path + Environment.NewLine;
        }

        private IFileBuilder _fileBuilder;
    }

    public interface IFileBuilder
    {
        string Build();
    }
}
\$\endgroup\$
5
\$\begingroup\$

There is no way to get the items contained in a Directory once they have been added.


Your base class defines:

public abstract Directory AddDirectory(string name);
public abstract void AddFile(string name, IFileBuilder fileBuilder);
public abstract void Remove(FileSystemItem item);

These methods only make sense for a Directory. The fact that one of your two subclasses has to throw an InvalidOperationException for each of these abstract methods is an indication that they should not be part of the base class. Even worse, someone who has a reference to a FileSystemItem has no way of knowing if calling one of these methods is safe without explicitly checking the actual type of the instance.

Note: Iterating over the contents of a directory (mentioned above) is another operation that only makes sense for a Directory. It would be best to let FileSystemItem define the core properties, but not try to force the aspects that don't work for all of the subclasses.

This will likely mean you will need to keep a directories contents in two separate collections. You can still expose a function that allows iterating over all of the items, but someone using that call would not have access the Directory specific methods.


FileSystemItem defines _rootPath as a member variable, but it is not used.


I prefer to have member variables at the top of the class. This way, someone reading the class for the first time, will know what variables are available to methods before seeing a method trying to use one.

I prefer to always use curly brackets for if statements and loops. It makes things less error prone when you have to edit the code later.

\$\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.