I have a class that stores the data related to all employees in a particular department.
In this class, I am having a list of employees and other fields.
Also, I am having a property which returns a bool
, indicating whether an employee exists or not. I am a bit confused regarding the naming of this property.
class Employee_Details
{
#region Private Fields
private List<Employee> employees;
#endregion
#region Fields
// Gets a value indicating whether an employee exists
public bool DoesEmployeeExists // what should be the name of this property?
{
get
{
return this.employees.Any();
}
}
#endregion
}
Is the property name DoesEmployeeExists
correct, or should I use any other name?
3 Answers 3
I would use simply HasEmployees
.
In case of Boolean, I always try to use hasSomething
or isSomething
. In Java, many libraries use this strategy (it's just a convention). And I like it, because it's so easy to read when you see:
if(annoyingEmployee.isRemovable() && !annoyingEmployee.hasBirthday()) {
manager.fireEmployee(annoyingEmployee);
}
-
7\$\begingroup\$ Sometimes also other prefixes can be used for booleans: e.g.
if (employee.canFixBug(bug)) { manager.giveExtraHoursTo(employee); }
\$\endgroup\$Bakuriu– Bakuriu2013年09月08日 07:29:45 +00:00Commented Sep 8, 2013 at 7:29
class Employee_Details
This class is badly named. First, .Net naming conventions say you shouldn't use underscore for this, the class should be called EmployeeDetails
. Second, the name seems to imply it contains details about a single employee. Better names would be EmployeesDetails
or something like EmployeeList
.
#region Private Fields
I don't see any reason to use #region
here, especially since it's for a single field.
#region Fields
If you're going to have regions, at least don't lie in their descriptions. This #region
contains properties, not fields.
// Gets a value indicating whether an employee exists
Consider using XML documentation comments. That way, the IDE (and other tools) can understand them.
public bool DoesEmployeeExists
I think a better name is indicated by the implementation: AnyEmployees
. But probably even better name would be a negated one: something like NoEmployees
or Empty
.
Also consider whether this property actually makes sense. I think a better option would be exposing the private list as something like IEnumerable<Employee>
or IReadOnlyList<Employee>
, which you probably have to do anyway. That way, any user of your class can run any query they want on the list, and you won't have to create separate properties or methods for each kind of query.
-
3\$\begingroup\$ I'm not convinced about
AnyEmployees
,NoEmployees
orEmpty
. I don't see any reason in using a negative name: if you want to check if any employees exist you'd have to useif(!NoEmployees)
which isn't very fluent.Empty
is ambigious because it might just as well refer to any other property in the class.HasEmployees
follows conventions (Has-a
,Is-a
etc) and makes its purpose clear. Agree on everything else. \$\endgroup\$Jeroen Vannevel– Jeroen Vannevel2013年09月07日 17:12:57 +00:00Commented Sep 7, 2013 at 17:12 -
\$\begingroup\$ @ svick, this is not my actual code. I was just confused with the property name that's why I have given random name to other entities. For all the regions and all, I was just trying to make the code readable :). I do need to have properties like this in which I check for empty collection, for WPF bindings. \$\endgroup\$MehaJain– MehaJain2013年09月07日 19:17:08 +00:00Commented Sep 7, 2013 at 19:17
-
3\$\begingroup\$ @MehaJain Well, the rules of this site say that you should post your actual code. That way, you might get comments about thing that you didn't expect to be problematic too. \$\endgroup\$svick– svick2013年09月07日 22:17:17 +00:00Commented Sep 7, 2013 at 22:17
First, @svick is absolutely right about the class name, regions and xml comments.
But to address your basic question, I would have the class implement either ICollection
or IList
depending upon how you use the rest of the class. If neither works for you, consider Count
... but I wuld really go with IList, as that would allow them to get the answer without you doing much at all other than providing some wrappers around your private variable.
-
2\$\begingroup\$ Most of the time, have a property of the appropriate collection type makes more sense over implementing a collection interface. Basically, composition over inheritance. \$\endgroup\$svick– svick2013年09月07日 22:19:12 +00:00Commented Sep 7, 2013 at 22:19
-
\$\begingroup\$ You're right, I was assuming that the OP had a good reason to make the list private instead of public, and if he does, he would probably want the
IReadOnlyList
instead. \$\endgroup\$jmoreno– jmoreno2013年09月08日 02:25:10 +00:00Commented Sep 8, 2013 at 2:25
DoesEmployeeExists
is grammatically incorrect. At least useDoesEmployeeExist
. But +1 toHas*
/Is*
, guitar_freak beat me to it. \$\endgroup\$