1
\$\begingroup\$

I am writing a time and attendance system. This is my idea of a simple elegant solution for the employee class. To me it makes the client very simple an encapsulates everything it needs, however I an new to Object Orientated Design and would appreciate a review. I plan to create the other classes like this but fear there may be a better way.

My employee class...

 class Employee
    {
        public static Employee CreateNew(string forename, string surname)
        {
            Employee employee = new Employee(forename, surname);
            AddToDataBase(employee);
            return employee;
        }

        public string Forename { get; set; }
        public string Surname { get; set; }


        private Employee(string forename, string surname)
        {
            this.Forename = forename;
            this.Surname = surname;
        }

        private static void AddToDataBase(Employee employee)
        {
          //code to add employee to database  

        }
    }

usage...

Employee.CreateNew("Bob", "Smith");
\$\endgroup\$

closed as off-topic by Vogel612, Graipher, Mast, Mathias Ettinger, alecxe Aug 16 '17 at 16:44

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I'd recommend reading this kalzumeus.com/2010/06/17/… though for this case it likely isn't a major concern \$\endgroup\$ – DavidTheWin Aug 15 '17 at 11:47
  • \$\begingroup\$ I wouldn't save right away. Usually we fill up the business object, check for business rule and later save to the database. \$\endgroup\$ – the_lotus Aug 15 '17 at 12:13
  • \$\begingroup\$ Your example might be pseudo code but you really should name your properties as Forename, Surname. \$\endgroup\$ – Eonasdan Aug 15 '17 at 12:27
  • \$\begingroup\$ i usually do, amended now \$\endgroup\$ – user3755946 Aug 15 '17 at 13:41
  • 1
    \$\begingroup\$ At first glance, something seems bad: How do you read an Employee back from the DB? Using CreateNew will result in a duplicate in the DB, so the only option is to call the private constructor though reflection, breaking encapsulation (unless there is more code to handle that not shown here). \$\endgroup\$ – Alejandro Aug 15 '17 at 17:09
7
\$\begingroup\$

It is common practice to use static methods-creators instead of constructors. But this approach is used in cases where you need to convert some objects to the object of desired type. It is the main reason for such methods – conversion from one representation to another.

But you use your CreateNew method for side effects and it is not good in my opinion. If I call CreateNew I'll expect just to get a new instance of the Employee. I don't expect that this method will do something else.

I would create method AddNewEmployeeAndAddToDB but not in the Employee class. Adding to database is not a part of employee functionality.

\$\endgroup\$
3
\$\begingroup\$

In addition to Maxim's answer, to avoid giving the employee class too much responsibilities, by adding the AddToDataBase method there, you could take a look at the repository pattern.

Repository pattern explained.

This pattern is created to handle database management.

\$\endgroup\$
  • \$\begingroup\$ never even heard of that and I have been looking for some. 'best followed practices. Thanks \$\endgroup\$ – user3755946 Aug 16 '17 at 13:56
1
\$\begingroup\$

If you assume that everybody has exactly one "forename" and one "surname", your code should probably reference where that decision was made (and, ideally, explain how to work around that where it's not valid).

It may well be useful to include additional unique information (payroll number, National Insurance number, or similar) to help distinguish between pairs of employees who have identical names.

\$\endgroup\$
  • \$\begingroup\$ In that version the database auto generated an employee id. \$\endgroup\$ – user3755946 Aug 17 '17 at 7:10

Not the answer you're looking for? Browse other questions tagged or ask your own question.