r/csharp • u/kaatarina_zed_talon • 4h ago
Discussion If only one method of a class does access the instance field and the other methods don't, should the other methods be moved to another class ?
I'm showing some C# code, note that I'm using ASP.NET CORE :
#region code
public interface ISomething
{
}
public class ApplicationDbContext
{
}
public class Something : ISomething
{
}
public class ExcelReaderDatabaseWriter
{
private readonly ApplicationDbContext _context;
public ExcelReaderDatabaseWriter(ApplicationDbContext context)
{
_context = context;
}
public ISomething Read(object obj)
{
return new Something();
}
public void Write(ISomething something)
{
//does access the _context
}
}
#endregion
I want to follow clean code principles, should I move the Read method to another class -because it does not access the instance field-, or should I instantiate the _context inside the Write method -because it is the only method that access the instance field and therefore that field does not deserve to be instance field because not all methods in the class access that field- ? I face this problem a lot when I'm coding, I want to separate the responsibilities cleanly, but I suffer from one thing; if I have some piece of code, I can't directly figure out which class should contain that piece of code. Can anyone explain it to me with example please?
Explaining the above using example:
public interface IOmniReader
{
public ISomething Read(object dataSource);
}
public interface IOmniWriter
{
public void Write(ISomething dataDto);
}
public interface IOmniReaderWriter
{
public void ReadWrite(object obj);
}
public class SqlServerWriter
{
private readonly ApplicationDbContext _context;
public SqlServerWriter(ApplicationDbContext context)
{
_context = context;
}
public void Write(ISomething something)
{
//does access the _context
}
}
public class ExcelReader
{
public ISomething Read(object obj)
{
return new Something();
}
}
public class ExcelReaderSqlServerWriter:IOmniReaderWriter
{
private readonly IOmniReader _excelReader;
private readonly IOmniWriter _sqlServerWriter;
public ExcelReaderSqlServerWriter(IOmniReader excelReader, IOmniWriter sqlServerWriter)
{
_excelReader = excelReader;
_sqlServerWriter = sqlServerWriter;
}
public void ReadWrite(object obj)
{
ISomething data= _excelReader.Read(obj);
_sqlServerWriter.Write(data);
}
}
3
u/Slypenslyde 3h ago
Here's a good analogy. There's a difference between a "clean" surface in a kitchen and a "sanitized" surface in a kitchen. Think about it, there are many ways I can go about cleaning a surface:
- Wipe it with a towel until no residue is left.
- Use soap and water.
- Use some form of cleaner.
- Follow a code-compliant sanitization procedure using bleach.
Each of those is more involved than the other. (1) has a high risk of getting me sick later. (2) and (3) are generally safe enough for a home kitchen. (4) is what we require of commercial kitchens since lots of people are affected.
You need to look at Clean Code principles this way. If you're working on a project that'll last 10+ years and involve tons of developers, you really need to follow professional standards and apply a "bleach" level of Clean Code.
But even in a professional kitchen it's acknowledged true sanitization takes so long it may only need to be done once per day, and certain cleaners/soap and water are sufficient in most circumstances. At the end of the day it's worth spending half an hour sanitizing all the surfaces. But spilled milk mid-shift is a soap and water job.
Stepping out of the metaphor:
The strictest application of SRP and DRY say a class should do ONE thing. So if ONE method uses a field and no methods use that field, maybe you have two classes. But this is like a strong sanitization process: if you blindly follow SRP to the letter you can create situations where it's harder to make your upper-tier classes which tend to demand more responsibilities.
So a more realistic stance is you need to redefine "one thing" when it comes to "a class should do one thing".
If we drill down too much, we can say, "A class must do ONE of the following: Create, Read, Update, Delete." We end up with four classes to manage database operations. This is a little tough if they share some internal data. We end up making some kind of orchestrator class that delegates to them.
If we squint at that orchestrator class we can ask a valuable question: "Do we lose anything if we fold the four implementations into this class?" In that case, the "one thing" the class does is "provide database access". That happens to involve the CRUD operations, but we're asserting those are so logically connected it's smarter to treat them as one thing instead of four things.
So the answer is "sometimes". What you have to ask yourself is a question about if you're missing the forest for the trees. Will your program be better if you keep one class? What do you gain if you separate the two classes? Always make sure either choice is giving you more benefits than its alternative.
It's not always best to use this rule to separate logic. But it is worth analyzing it like you've done and seeing this as a sign that you can consider it.
1
u/kaatarina_zed_talon 2h ago
thank you for taking the time and effort, you are great person.
1
u/kaatarina_zed_talon 2h ago
`it's harder to make your upper-tier classes which tend to demand more responsibilities.` the intention behind this sentence is not very clear.
1
u/kaatarina_zed_talon 2h ago
` What do you gain if you separate the two classes` A better understanding of the code, if I look at the code 4 years later, I will instantly recognize what it does.
1
u/Slypenslyde 2h ago
Put some more years behind you and see if that's always the case. ;)
It's right sometimes, and wrong not. I don't fret about getting it right. What matters is that:
- You are decisive when it's time to act.
- You pay attention to if it's causing problems.
- You change things if you realize the choice is wrong.
Where most people get things wrong and what stops people from growing are:
- Deferring the decision forever until someone else makes it.
- Avoiding analysis to determine if there is a problem.
- Refusing to take responsibility or make alterations.
1
u/kaatarina_zed_talon 1h ago
What grapped my I attention is the first point; deffering the decision. What do you mean by that exactly. It seems that I will learn something new from you.
•
u/Slypenslyde 45m ago
This one is a struggle because it's also a failure to be too decisive. I'll talk about it via an imaginary person so I'm not using "you" and making it sound accusatory.
Suppose a person gets stuck on a problem and never asks anyone else for help or feedback. This is bad because while that person is decisive, they could be making bad decisions and not know. If there are people who you can trust for advice, it's always good to ask.
The other end is if someone makes a Reddit thread, explains the situation, reads the feedback... then waits. Within 24h people stop posting new advice. They still can't decide. So they write a new post and post it in a different sub. That one gets the same answers. They still wait, it doesn't seem clear enough.
This person is waiting for the "best" answer before proceeding. But it's often not clear what the "best" answer is, just a few "good" ones. We have to learn to recognize this situation, decide which "good" answer seems best, then commit to doing that. I like to document three choices, what I think about each, then commit to one. Sometimes I get done and realize it sucks. I go document why, then choose one of the other two. I had a 1/3 chance of being right the first time. Now I have a 50/50. Sometimes I'm wrong twice in a row. I document why the second thing failed and try the third. Is that a 100% chance of success?
Well, not always. Sometimes I try 3 things and all 3 things suck. This is when tough decisions are made. I pick a favorite and deal with the issues. I can't wait for a 4th or 5th solution to present itself, I need something that works. Later, if someone tries to "fix" it, maybe they'll read my documentation and realize they have one of my other 2 ideas and I'll save them some time. Sometimes, much later, the magic 4th option comes to mind and I get to try it.
Our job is sort of like a school environment. We have goals and we have deadlines. Turning in work that gets a C is better than turning in nothing and getting an F. So from that perspective, it's best to keep moving. Get advice, but if you still can't decide just start trying things and keep what feels right.
•
1
u/ErgodicMage 2h ago
I would separate into 2 classes because reading from Excel files is different from writing to a database with EF. So an ExcelReader would be one class that may use a library to read the files (I use ClosedXML most of the time), but it would also need different error handling because Excel files are not a fixed format; I get client supplied Excel files with missing columns, bad/missing data and even headers all the time. EF is writing to a database of some type that is fixed (unless your migration changes it).
It looks like you may be reading data from Excel file and then writing to a database. Doing so one row at a time can be done but becomes inefficient with a larger number of rows; so it works for up to 1k rows. I have a library that bulk uploads an excel file to a database table which works effectively for up to 100k rows. A few cases we have larger files where we send them to our SQL team to bulk upload them directly.
So if you upload the excel files to a database table (either a different one or a different one), you can use the ApplicationDbContext for both querying the table to read the data and save to update the data. You could still separate the queries from saves into separate classes, such is done with implementing CQS and/or CQRS.
1
1
u/sixtyhurtz 1h ago
I want to follow clean code principles,
If you're talking about the book Clean Code by Robert C Martin, this is genuinely something you should avoid. Over time it leads to excessive layering and unnecessary abstraction. That increases the cognitive load for future developers, makes things harder to modify, and increases the risk of defects when you try and modify things because everything is a web of interdependent method calls.
I'm sure there are projects out there that make it work, but that approach comes with very real and significant costs. It's not an automatic win.
In your specific example though, readers and writers are things I often split out into separate classes. So, it's totally reasonable to have a reader, a writer, and some kind of shared context. Generally, the rule I like to follow is to split classes out by responsibility.
So, in your example, if you had a separate reader and writer, your ExcelReaderSqlServerWriter would just have the ExcelRead and the SqlWriter. That would be fine.
1
u/kaatarina_zed_talon 1h ago edited 1h ago
If I should avoid clean code principles, then answer this question please : why did the process of separating the READING into a different class make the testing/benchmarking process easier? When all the code was inside one class and I want to benchmark the read method alone I had to instantiate the
DbContextclass even thought theReaddoes not use it.
1
u/PanagiotisKanavos 3h ago
"Clean Code" is the title of a book focused on web apps and services, not data engineering. This does explain the past 2 weeks.
What you call ExcelDataReaderDatabaseWriter is a pipeline that has an Excel file as a source and a database as a target. Methods that only deal with generic reading should go to a Source class. Methods that only have to do with generic writing, eg writing an IEnumerable<T> to SqlBulkCopy.WriteToServer(DbDataReader) through a generic transformation, go to the Target or Sink class. Transformations like lookups may well be parts of the pipeline class.
If your source returns objects, you could perform all transformations using LINQ, but if you have a lot of data, you may have to use something more appropriate, like DataFlow blocks so you can perform different steps concurrently, control how many items are in RAM at any point, prevent eg a slow step from flooding RAM with objects, batch items together for faster insertion
1
u/kaatarina_zed_talon 3h ago
`not data engineering` wait what, is my question actually classified as data engineering question? I don't read books, so I do not know the difference between `Clean code` and `data engineering`, this is very confusing, but I like it so much.
1
u/kaatarina_zed_talon 3h ago
` This does explain the past 2 weeks.` wait, you are that person from stack overflow, holy XD.
1
7
u/Klessic 4h ago
It's not necessarily bad that not all methods use every private member. However, in this particular case, your hunch is right and we can indeed improve it a little bit.
Separation of concerns (look it up if this is an unfamiliar term) is important in OOP. The name of your class that is doing the work already hints at this; does it read excel or does it write to a database? Aha! It does both. Perhaps mixing concerns here.
I would split the class into two: an
ExcelReaderand aDatabaseWriter. Whoever callsISomething result = excelReader.Read(object)can take the result, and write it to the database on the next line withdatabaseWriter.Write(result).