Note
Access to this page requires authorization. You can try signing in or changing directories.
Access to this page requires authorization. You can try changing directories.
Question
Tuesday, October 30, 2018 12:43 AM
I am trying to validate the value that gets assigned to the StudentFirstName property and it works fine but I want to see if I am taking the right approach. I see other peoples {get; set;} values and it never has a lot of code in it. Is the way I'm doing bad code practice
public class Students
{
private string _FirstName;
private string _LastName;
public string StudentFirstName
{
get { return _FirstName; }
set {
while(value.Any(char.IsDigit))
{
Console.ForegroundColor = ConsoleColor.Red;
Console.WriteLine("ERROR: First name cannot contain a number. Please enter correct name.");
Console.ForegroundColor = ConsoleColor.White;
value = Console.ReadLine();
}
_FirstName = value;
}
}
}
All replies (6)
Tuesday, October 30, 2018 1:05 AM | 2 votes
In my opinion that is bad practice.
Firstly, that Students class will only work in a console application. So you have limited that code so that it can't be used in a Winforms or any other environment.
Secondly (and more importantly) you are assuming how that setter will be called. If you ask the user, via the console, to input a name and then immediately set it, that will look okay because the user will see more console messages saying what is wrong and asking for a fix. But if you are setting names from, for example, a database table, the user will suddenly see console messages appearing out of nowhere and wonder what the hell is going on. In that sort of case, you probably don't want the user to enter a new name, but instead say that something has gone wrong with the database and give instructions about how to fix it.
In my opinion (and it is just my opinion - you may disagree) a student should never ask for its own name. That should be done outside the class, and the input checked for validity before calling the setter. To make sure the checking is consistent, you could add a method to the Student class called IsValid that takes a string and returns a bool to say if the string is okay or not.
Tuesday, October 30, 2018 1:54 AM
In my opinion (and it is just my opinion - you may disagree) a student should never ask for its own name.
Just want to comment this part.
If this were a program for school administrative works, then it would be normal to store student information in classes for further processing.
Btw, if this is homework, then using console isn't that bad.
Tuesday, October 30, 2018 1:55 AM
add a method to the Student class called IsValid that takes a string and returns a bool to say if the string is okay or not.
So, for example if I had a method like how you mentioned:
public bool IsValid(string FirstName)
{
if(FirstName.Any(char.IsDigit))
{
return false;
}
else
{
return true;
}
}
and then I called it in my code like this:
while(TempStudent.IsValid(TempFirstName) == false)
{
Console.WriteLine("Name is invalid please enter a valid name");
TempFirstName = Console.ReadLine();
}
then I assign the value that is valid to the Students.StudentFirstName like this:
Students.StudentFirstName = TempFirstName;
that would be a better way to do it? (Please point out if there are any faults in my code)
Tuesday, October 30, 2018 2:15 AM | 1 vote
Yes, that is the way I would do it. Someone else might do it differently, but that would be my way.
As to faults in your code, most people would not use "== false" to check a boolean. Instead they would do something like this ;
while(!TempStudent.IsValid(TempFirstName)) // using the 'not' operator ("!").
The way you coded is not wrong, it's just not the most common style.
Edit: And the IsValid method could be static, so you don't need to instantiate a student to use it.
Tuesday, October 30, 2018 10:34 AM
I think Student object it could be container without any logic only. This object is trasfered into application layer. It means there is something like gate into system. Maybe there will be Save method which take Student object as parameter.
You can create StudentValidator class. This class will have small validators (i.e. required field validator, regular expression validator, etc...). You can compose this validator to check if object is valid. To reusable, small validator has Validate method without parameters. It is hard solution but you can change it to create validation attribute which can set validator up to automated validation.
Each validator has ErrorMessage which can be used to set exception message and write to console, ...
Each validator is reusable to validate each input. Your solution is about repeating large code.
Advance of using validator is when more then one entity has i.e. Address entity inside. Address validator is written only once and can be used by Student validator, Parent validator, Teacher validator, ...
class StudentValidator : IValidator
{
public string ErrorMessage {get; private set;}
private readonly Student objectToValidate;
private readonly List<IValidator> validators = new LIst<IValidators>();
public StudentValidator(Student objectToValidate)
{
_objectToValidate = objectToValidate;
validators.Add(new RequireFieldValidator(_objectToValidate.LastName, "Last name cannot be empty."));
}
public bool Validate()
{
ErrorMessage = string.Empty;
foreach(var validator in validators)
{
if (!validator.Validate())
{
ErrorMessage = validator.ErrorMessage;
return false;
}
}
return true;
}
}
interface IValidator
{
string ErrorMessage{get;}
bool Validate();
}
class RequireFieldValidator : IValidator
{
public ErrorMessage {get; private set;}
private readonly string valueToValidate;
public RequireFieldValidator(string valueToValidate, string errorMessage)
{
ErrorMessage = errorMessage;
this.valueToValidate = valueToValidate;
}
public bool Validate()
{ if (valueToValidate == null) return false;
return !string.IsNullOrEmpty(valueToValidate.Trim());
}
}
Tuesday, October 30, 2018 5:54 PM
I have not had a chance to use interfaces but I get the gist of what you are explaining. The example you provided is beyond what I was trying to do but it is very detailed and makes sense, thank you!
(Also you are missing an I in front of your Validator interface declaration)