Is there a "correct" way to abstract my code?
I have been developing in C # for about 12 months (from scratch, no previous experience is different from a little PHP script hacking) and I like to think that I have developed my skills to a level that I can write an application and execute its function perfectly.
however, I am still a little confused about the best coding practices, I realize this code is bad:
class Example1
{
public static Alert GenerateAlert()
{
Alert AlertObject = new Alert();
AlertObject.AlertDatetime = DateTime.Now;
AlertObject.AlertHasRecords = false;
return AlertObject;
}
}
If, for example, AlertDatetime
it takes more than a simple string of type DateTime.Now;
, I'll end up typing a massive function. not good!
However, I see no problem with the following two examples (I prefer example 2)
class Example2
{
public static Alert AlertObject = new Alert();
public static Alert GenerateAlert()
{
PopulateAlertDate();
CheckForAlertRecords();
return AlertObject;
}
private static void CheckForAlertRecords()
{
AlertObject.AlertHasRecords = false;
}
private static void PopulateAlertDate()
{
AlertObject.AlertDatetime = DateTime.Now;
}
}
class Example3
{
public static Alert GenerateAlert()
{
Alert AlertObject = new Alert();
AlertObject.AlertDatetime = PopulateAlertDate();
AlertObject.AlertHasRecords = CheckForAlertRecords();
return AlertObject;
}
private static bool CheckForAlertRecords()
{
return false;
}
private static DateTime PopulateAlertDate()
{
return DateTime.Now;
}
}
One example is better than another, and if so, why? or is there a completely different way to do it?
source to share
Your first example is fine.
If a AlertDateTime
more complex function needs to be initialized at a later time , you can always refactor your code to something like Example 3. Until then, respect KISS (keep it simple) and YAGNI .
Note that the interface (public methods and their signature) does not change between examples 1 and 3. This is good. This means that you can navigate between these styles without changing the code your class uses.
Example 2, however, has many problems:
-
The principle of hiding information basically says that you should not publish anything publicly without a good reason. Why would you want to store the newly created Alert in a public "global variable"?
-
Example 2 behaves differently: if you call twice
GenerateAlert
, it will return a reference to the same Alert object both times. (Think about what happens if you call him today and tomorrow.)
As a side note, consider the naming of your methods in example 3. Try to think about each method separately: PopulateAlertDate()
does not fill in the warning date. It returns a date that can be used to populate the alert date. A name might be more appropriate GetDefaultAlertDate()
.
source to share
+1 for Heinzi's great answer.
I'll add that in example 3 you are using the Facade option . You wrap the class with complex reinitialization logic, and also hide the interface of that object and expose new methods instead. If you later have several different ways to create the same object, you should consider the Factory pattern .
Please note, you must first put some code in your original constructor class , unless there is a reason to use a different change at once.
Example 2 resembles the Singleton anti-pattern , which serves a different purpose — keeping one instance of a class. This is usually done for services that you prefer to create once and for all. Even then, you might want to look better at Dependency Containers for more significant unit testing capabilities.
source to share
If there is more logic in these functions than just assigning true or false, then you can use factory and interfaces. Completely abstracted code following solid guidelines would look like this:
public class AlertFactory : IAlertFactory {
IAlertDatePopulator alertDatePopulator;
IAlertRecordsChecker alertRecordsChecker;
public AlertFactory(IAlertDatePopulator alertDatePopulator, IAlertRecordsChecker alertRecordsChecker) {
this.alertDatePopulator= alertDatePopulator;
this.alertRecordsChecker = alertRecordsChecker;
}
public Alert GenerateAlert() {
Alert alertObject = new Alert();
alertObject.AlertDatetime = alertDatePopulator.Populate();
alertObject.AlertHasRecords = alertRecordsChecker.Check();
return alertObject;
}
}
from
interface IAlertFactory { Alert GenerateAlert(); }
interface IAlertDatePopulator { DateTime Populate(); }
interface IAlertRecordsChecker { bool Check(); }
Then you can add concrete implementations for these interfaces, for example:
public class DateTimeNowAlertDatePopulator : IAlertDatePopulator {
public DateTime Populate() { return DateTime.Now; }
}
public class SomeCalculationAlertDatePopulator : IAlertDatePopulator {
public DateTime Populate() { return /* something calculated */; }
}
respectively.
public class AlwaysFalseAlertRecordsChecker : IAlertRecordsChecker {
public bool Check() { return false; }
}
public class SomeCalculationAlertRecordsChecker : IAlertRecordsChecker {
public bool Check() { return /* something calculated */; }
}
Then you can create configured factories:
public class DateNowAndRecordsFalseAlertFactory : AlertFactory {
public DateNowAndRecordsFalseAlertFactory ()
: base (new DateTimeNowAlertDatePopulator(), new AlwaysFalseAlertRecordsChecker()) { }
}
public class DateNowAndCalculatedRecordsAlertFactory : AlertFactory {
public DateNowAndCalculatedRecordsAlertFactory ()
: base (new SomeCalculationAlertDatePopulator(), new AlwaysFalseAlertRecordsChecker()) { }
}
And then just use a factory:
var alertFactory = new DateNowAndRecordsFalseAlertFactory ();
var myAlert1 = alertFactory.GenerateAlert();
var alertFactory2 = new DateNowAndCalculatedRecordsAlertFactory();
var myAlert2 = alertFactory2.GenerateAlert();
etc .. This seems to be a lot of code for simple functionality, but if you expect a lot of extensions with a lot of logic, then this is clean code following the open / close principle (to be open to extensions (just by adding new interface implementations ), but closed for modification (no need to modify existing code anymore)).
Most efficient when used with dependency injection. Then you configured your factory like this:
public class DateNowAndRecordsFalseAlertFactory : AlertFactory {
public DateNowAndRecordsFalseAlertFactory (DateTimeNowAlertDatePopulator alertDatePopulator, AlwaysFalseAlertRecordsChecker alertRecordsChecker)
: base (alertDatePopulator, alertRecordsChecker) { }
}
And just do:
var alertFactory = someDiContainer.Resolve<DateNowAndRecordsFalseAlertFactory>();
source to share
You are trying to instantiate an object and I see no point in having a static method for that (there is an answer already with a factory, do you really need this?)
If you need to create this object, just do
var alert = new Alert();
If you want to tweak some properties after creating an object with default values then this is a shortcut
var anotherAlert = new Alert() { AlertDatetime = DateTime.Now };
Usually you should instantiate an object as much as possible, so if you should always construct it with the current date, the constructor usually does this:
public class Alert
{
// do not add class name to property
public DateTime DateTime {get; set;}
// this don't need initialization if default value is false
public bool HasRecords {get; set;}
public Alert()
{
DateTime = DateTime.Now;
}
}
source to share