-2

I have an example about design patterns that demonstrates the adapter pattern. But I see casting to derived class here. That is not very good. If I meet such code in my practice, what is the best way to refactor it? Or what principles should I follow to avoid such situation?

public class SqlServerLogSaverAdapter : ILogSaver
{
    private readonly SqlServerLogSaver _sqlServerLogSaver = new SqlServerLogSaver();
    public void Save(LogEntry logEntry)
    {
        var simpleEntry = logEntry as SimpleLogEntry;
        if (simpleEntry != null)
        {
            _sqlServerLogSaver.Save(simpleEntry.EntryDateTime,
                simpleEntry.Severity.ToString(),
                simpleEntry.Message);
            return;
        }
        var exceptionEntry = (ExceptionLogEntry)logEntry;
        _sqlServerLogSaver.SaveException(exceptionEntry.EntryDateTime, 
                exceptionEntry.Message,
                exceptionEntry.Exception);
    }
}

ps. May be visitor pattern solves it but I'm not sure.

  • As I see single negative score I've the question "why"? (To take into account it in future) – vitm Jun 19 '17 at 11:48
  • I would expect the Save method to work correctly for all types of LogEntry objects. Why can't it? – Tanner Swett Jun 19 '17 at 12:01
  • I didn't comprehend your question completely. So far Jules answered in correct way I suppose. – vitm Jun 19 '17 at 12:10
  • 2
    As this is a question on how to refactor working, but ugly, code, you should post it to Code Review, rather than here. – David Arno Jun 19 '17 at 12:15
  • 2
    If you post this to Code Review @VitalyMosin, try to use a title that describes what the code does. Otherwise, the site would fill up with titles identical to this one. – RubberDuck Jun 20 '17 at 10:05
3

The best practice is to follow SOLID principles. Specifically, code should depend on interfaces, not implementation details.

The problem here is that your LogSaver depends on a particular concrete implementation of LogEntry. The solution is have LogSaver depend on a common interface instead. In this example I name that interface ISaveableLogEntry.

public interface ISaveableLogEntry
{
    DateTime EntryDateTime { get; set; }
    String   TextToSave { get; }
}

public class SimpleLogEntry : ISaveableLogEntry
{
    public DateTime EntryDateTime { get; set; }
    public String Severity { get; set; }
    public String Message { get; set; }
    public String TextToSave 
    {
        get { return Severity + " " + Message; }
    }
}

public class ExceptionLogEntry : ISaveableLogEntry
{
    public DateTime EntryDateTime {get; set; }
    public Exception Exception { get; set; }
    public String TextToSave 
    {
        get { return this.Exception.Message + " " + this.Exception.StackTrace; }
    }
}

public class SqlServerLogSaverAdapter : ILogSaver
{
    private readonly SqlServerLogSaver _sqlServerLogSaver = new SqlServerLogSaver();

    public void Save(LogEntry logEntry)
    {
        ISaveableLogEntry e = logEntry as ISaveableLogEntry;
        if (e == null) return;
        _sqlServerLogSaver.Save(e.EntryDateTime, e.TextToSave);
    }
}

With the above code you never need to cast from general to specific. You only need to cast from specific to general (to get the ISaveableLogEntry interface). If the cast fails, the entry can't be saved (which makes sense if the object is not a saveable log entry). Otherwise, there is only one code path for saving it, since each SaveableLogEntry knows what needs to be saved.

As a David pointed out, if you're willing to modify the prototype of SqlServerLogSaverAdapter.Save() then you can even avoid all casting by changing it to

public void Save(ISaveableLogEntry logEntry)

...and filtering the list before you call it. Easy to do with LINQ:

foreach (ISaveableLogEntry logEntry in _logEntryList.OfType<ISaveableLogEntry>())
{
    logAdapter.Save(logEntry);
}
  • The use of a LogEntry parameter, which you then cast to ISaveableLogEntry is odd. Why not just make the parameter, ISaveableLogEntry? Also, TextToSave isn't a good choice of name IMO, as it's making assumptions over how it'll be used. Something like LogDescription might be better. Aside from that, this is a very good answer that shows the power of polymorphism when used well. – David Arno Jun 20 '17 at 7:54
  • Good point about the cast, I edited my answer. As for TextToSave, I wanted to make a relatively simple example. If I were to code this I'd probably expose loggable text as a string[] so that the logger could decide how to concatenate them, and give it a totally different name, like LoggableContent or something. – John Wu Jun 20 '17 at 8:16
  • 1
    Thank you, but what if it's not enough to have single Save realization in _sqlServerLogSaver. if I need to save record to different db-tables? – vitm Jun 20 '17 at 8:46
  • You write another implementation of Save, in a class that implements ILogSaver, that also accepts an ISaveableLogEntry as an argument, then register the class in your composition root. Where is the issue? – John Wu Jun 20 '17 at 10:56
  • No doubt I could add new realization of ILogSaver (f.ex. OracleLogSaverAdapter ) to save to other storage. But if i need different realization (simple logs save to one table and exception to another one) of _sqlServerLogSaver.Save? – vitm Jun 20 '17 at 11:22
0

In my opinion, the interface definition of ILogSaver is wrong: when you have to handle SimpleLogEntry different from ExceptionLogEntry, you need two distinct functions in the interface instead of the single Save function.

-1

Simply put, any code that works only with one type should be part of that type, rather than a client. Clients shouldn't care what particular subtype of an abstract base or interface they receive. So, for your specific example, I would refactor it like this:

public class SqlServerLogSaverAdapter : ILogSaver
{
    private readonly SqlServerLogSaver _sqlServerLogSaver =
    new SqlServerLogSaver();
    public void Save(LogEntry logEntry)
    {
        logEntry.saveTo(_sqlServerLogSaver);
    }
}

// ... in LogEntry
    abstract void saveTo(SqlServerLogSaver saver);

// ... in SimpleLogEntry

    void saveTo(SqlServerLogSaver  _sqlServerLogSaver)
    {
            _sqlServerLogSaver.Save(EntryDateTime,
                Severity.ToString(),
                Message);
    }


    //  in ExceptionLogEntry...

    void saveTo(SqlServerLogSaver  _sqlServerLogSaver)
    {
        _sqlServerLogSaver.SaveException(EntryDateTime, Message,
                                         Exception);
    }
}
  • 3
    "Simply put, any code that works only with one type should be part of that type...". I completely disagree. Your code now couples log entries to a persistence mechanism. This is in direct violation of the single responsibility principle. – David Arno Jun 19 '17 at 12:19
-2

Since the method will fail if anything else that an ExceptionLogEntry object is passed the methods parameter should be of type ExceptionLogEntry so that the caller is aware of this dependency and no cast would be needed.

  • The code also works if the type is SimpleLogEntry. – Jules Jun 19 '17 at 9:56
  • @Jules I'd expect a ClassCastException at varexceptionEntry = (ExceptionLogEntry)logEntry; – Timothy Truckle Jun 19 '17 at 9:59
  • There's an early return inside the if statement above that prevents it when the type is SimpleLogEntry. – Jules Jun 19 '17 at 10:11
  • 1
    "Since the method will fail if anything else that an ExceptionLogEntry object is passed..." This statement is incorrect. It works for SimpleLogEntry too. – David Arno Jun 19 '17 at 12:16

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.