HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Small function and level of abstraction - clean code - refactor further

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
levelfunctionabstractionfurthercodesmallandrefactorclean

Problem

In the book Clean Code from Robert C. Martin are given many recommendations. Two of them are

  • to keep functions small and



  • within a function to only have one level of abstraction.



I already cleaned the following method up a bit by extracting a few methods (RemoveFontColor(), RemoveTextHighlight()). But i am not sure if the few-liners (font-style, font-weight, etc.) should be extracted into a method as well. The advise from the author is to not mix different-levels of abstraction within a function because this could lead to other details creeping in. This would mean that even the two liners will be extracted into their own methods.

The partly refactored code example

private string Fix(FixPreferences preferences,string value)
{
    if (preferences.IsRemoveBold) {
        value = value.Replace("font-weight:bold", "font-weight:normal");
        value = value.Replace("font-weight: bold", "font-weight:normal");
    }
    if (preferences.IsRemoveItalics) {
        value = value.Replace("font-style:italic;", "font-style:normal;");
        value = value.Replace("font-style: italic;", "font-style:normal;");
    }    
    if (preferences.IsRemoveFontColor) {
        value = RemoveFontColor(value);
    }
    if (preferences.IsRemoveTextHighlight) {
        value = RemoveTextHighlight(value);
    }  
    if (preferences.IsRemoveStrikethrough){
       value = 
        value.Replace("text-decoration: line-through;", "text-decoration:none;");
       value = 
        value.Replace("text-decoration:line-through;", "text-decoration:none;");
    }
    if (preferences.IsRemoveUnderline)
    {           
      value = 
       value.Replace("text-decoration: underline;", "text-decoration:none;");
      value = 
       value.Replace("text-decoration:underline;", "text-decoration:none;");
    }
    return value;
}


Update

I changed to extract even the few lines - but it still looks not pretty.

```
private string Fix(ChangeLogFixOperationPreferences preferences,str

Solution

First of all, each condition and formatting will be moved to your own class. Let's create a common interface:

interface FormattingRemover
{
    bool Match(string value);
    string RemoveFormat(string value);
}


Now we can use it to abstract the formatting:

public class RemoveBold : FormattingRemover
{
    public bool Match(string value) {
        // your condition here
        return true;
    }

    public string RemoveFormat(string value) {
        return RemoveBold(value);
    }
}


In your class you'll need a list with all implementantions of FormattingRemover, iterate over it and when the value matches, you can remove its formatting.

private removers = AllRemovers();

private string Fix(ChangeLogFixOperationPreferences preferences,string value)
{
    foreach (var remover in removers)
    {
        if (remover.Match(value))
        {
            value = remover.RemoveFormat(value);
        }
    }

    return value;
}


For now on, you just need to add new implementations to provide it with AllRemovers. A good idea is inject it with some DI container, like Ninject.

Code Snippets

interface FormattingRemover
{
    bool Match(string value);
    string RemoveFormat(string value);
}
public class RemoveBold : FormattingRemover
{
    public bool Match(string value) {
        // your condition here
        return true;
    }

    public string RemoveFormat(string value) {
        return RemoveBold(value);
    }
}
private removers = AllRemovers();

private string Fix(ChangeLogFixOperationPreferences preferences,string value)
{
    foreach (var remover in removers)
    {
        if (remover.Match(value))
        {
            value = remover.RemoveFormat(value);
        }
    }

    return value;
}

Context

StackExchange Code Review Q#29444, answer score: 5

Revisions (0)

No revisions yet.