I'm doing a little project for school where I try to do a spreadsheet program, and I have two classes, I will be simplifying this with pseudocode a little bit so it's not too messy.

class DocumentController {
    Document doc // This is a class with a CRUD on a document (It haves 
        // Sheets and every Sheet haves a Table full of Cells)
    Parser p

    getValueOfCell (sheetName, positionX, positionY) {
         returns value of a cell in a sheet in the position x,y
    }
    
    setCell (String expression, sheetName, positionX, positionY) {
         //Somewhere here we need to use p.evaluate()
    }
 
}

class Parser {
    DocumentController docController;
    evaluate (expression: String) {
        //Somewhere here, I need to use method getCell from Document 
        // for evaluating the  expression (The expressions have 
        // references to other cells so the Parser need to resolve 
        // these references)
        ...
        return value of the expression (float, integer, string, whatever)
    }
}

So apparently my teacher said to me that this is a bad design, because these classes are too coupled and this is a code smell. Can someone explain me why is this so bad? How can I make a better design?

Thank you, sorry if I made some typos or the code is not legible

CodePudding user response：

I think you want something like:

Class Main{
   public void main(){
     DocumentController dc = new DocumentController();
     //you can get ahold of the parser by
     Parser p = dc.getParser();
   }
}



Class Parser{
      DocumentController dc;
      public Parser(DocumentController dc){
         this.dc = dc;
      }
     //your methods
   }



Class DocumentController{
   Parser p;

public DocumentController(){
     this.p = new Parser(this);
   }
   
   public Parser getParser(){
      return this.p;
   }
   //your methods

  }

Although there are probably better ways of doing this instead like passing your object to the method when you need it. Something like

 Class Main{
    public void main(){
     DocumentController dc = new DocumentController();
     Parser p = new Parser();
     p.myParserMethod(dc);
     dc.myDocMethod(p);
    }
 }


 Class Parser{
    public myParserMethod(DocumentController dc){
        //you can use the same documentController object here
    }
}

Class DocumentController{
   public myDocMethod(Parser p){ 
    //you can use your parser object here
   }

}

hope that helps

CodePudding user response：

It looks like you want to format value by some key expression. If yes, then we can create mapping between this key expression and format classes. Then we can use Factory pattern to create desired objects to format your cell value.

Let me show a simple example via C#.

So this is a DocumentController:

public class DocumentController
{
    private DocumentService _documentService;

    public DocumentController()
    {
        _documentService = new DocumentService(); // this dependency can be 
            // resolved by IoC container
    }

    public void GetValueCell(int docId, string sheetName, int positionX, 
        int positionY)
    {
        _documentService.GetValueCell(docId, sheetName, positionX, 
            positionY);
    }

    public void SetCell(int docId, string expression, string sheetName, int 
        positionX, int positionY, object value)
    { 
        _documentService.SetCell(docId, expression, sheetName, positionX, 
            positionY, value);
    }
}

And this is a service which will execute logic related to Document:

public class DocumentService
{
    private DocumentRepository _documentRepository;

    public DocumentService()
    {
        _documentRepository = new DocumentRepository();
    }

    public string GetValueCell(int docId, string sheetName, int positionX, int positionY)
    {
        Document document = _documentRepository.GetById(docId);
        return document.GetCellValue(sheetName, positionX, positionY);
    }

    public void SetCell(int docId, string expression, string sheetName, int 
        positionX, int positionY, object value)
    {
        Document document = _documentRepository.GetById(docId);
        document.SetCellValue(expression, sheetName, positionX, positionY, 
            value);
    }

}

It is unknown how you get Document, but it is possible to use repository pattern for that purpose.

public class DocumentRepository 
{
    public Document GetById(int id) { throw new NotImplementedException(); }
}

and this is a Document class:

public class Document 
{
    private object[][] _cells;

    public Document(int x)
    {
        _cells = new object[x][];
    }

    public string GetCellValue(string sheetName, int positionX, int positionY) 
    {
        return string.Empty;
    }

    public void SetCellValue(string expression, string sheetName, int 
        positionX, int positionY, object value)
    {
        FormatterType formatterType = new 
            FormatterTypeToExpression().FormatterByExpression[expression];
        Formatter formatter = new FormatterFactory().
            FormatterByFormatterType[formatterType];
        object formattedCell = formatter.Format(value);
        _cells[positionX][positionY] = formattedCell;
    }
}

and this is a mapping between FormatterType and your key expression:

public class FormatterTypeToExpression
{
    public Dictionary<string, FormatterType> FormatterByExpression { get; set; } = 
        new Dictionary<string, FormatterType>
    {
        { "string", FormatterType.String}
        // here you write expressions and foramtters
    };
}

This is a formatter type:

public enum FormatterType
{
    String, Number, Decimal, Whatever
}

Then you need something like factory to take a formatter:

public abstract class Formatter 
{
    public abstract object Format(object value);
}

And abstract class which will define behavior of derived formatter classes:

public class FormatterString : Formatter
{
    public override object Format(object value)
    {
        return "I am a formatted string value";
    }
}

An example how FormatterFactory could look like:

public class FormatterFactory
{
    public Dictionary<FormatterType, Formatter> FormatterByFormatterType { get; set; }  
        = new Dictionary<FormatterType, Formatter>
    {
        { FormatterType.String, new FormatterString()}
        // here you write FormatterType and formatters
    };
}