Deprecated: Assigning the return value of new by reference is deprecated in /var/www/serverdude.dk/public_html/wp-settings.php on line 512

Deprecated: Assigning the return value of new by reference is deprecated in /var/www/serverdude.dk/public_html/wp-settings.php on line 527

Deprecated: Assigning the return value of new by reference is deprecated in /var/www/serverdude.dk/public_html/wp-settings.php on line 534

Deprecated: Assigning the return value of new by reference is deprecated in /var/www/serverdude.dk/public_html/wp-settings.php on line 570

Strict Standards: Declaration of Walker_Page::start_lvl() should be compatible with Walker::start_lvl(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1199

Strict Standards: Declaration of Walker_Page::end_lvl() should be compatible with Walker::end_lvl(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1199

Strict Standards: Declaration of Walker_Page::start_el() should be compatible with Walker::start_el(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1199

Strict Standards: Declaration of Walker_Page::end_el() should be compatible with Walker::end_el(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1199

Strict Standards: Declaration of Walker_PageDropdown::start_el() should be compatible with Walker::start_el(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1244

Strict Standards: Declaration of Walker_Category::start_lvl() should be compatible with Walker::start_lvl(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1391

Strict Standards: Declaration of Walker_Category::end_lvl() should be compatible with Walker::end_lvl(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1391

Strict Standards: Declaration of Walker_Category::start_el() should be compatible with Walker::start_el(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1391

Strict Standards: Declaration of Walker_Category::end_el() should be compatible with Walker::end_el(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1391

Strict Standards: Declaration of Walker_CategoryDropdown::start_el() should be compatible with Walker::start_el(&$output) in /var/www/serverdude.dk/public_html/wp-includes/classes.php on line 1442

Strict Standards: Redefining already defined constructor for class wpdb in /var/www/serverdude.dk/public_html/wp-includes/wp-db.php on line 306

Deprecated: Assigning the return value of new by reference is deprecated in /var/www/serverdude.dk/public_html/wp-includes/cache.php on line 103

Strict Standards: Redefining already defined constructor for class WP_Object_Cache in /var/www/serverdude.dk/public_html/wp-includes/cache.php on line 431

Deprecated: Assigning the return value of new by reference is deprecated in /var/www/serverdude.dk/public_html/wp-includes/query.php on line 61

Deprecated: Assigning the return value of new by reference is deprecated in /var/www/serverdude.dk/public_html/wp-includes/theme.php on line 1109

Strict Standards: Declaration of Walker_Comment::start_lvl() should be compatible with Walker::start_lvl(&$output) in /var/www/serverdude.dk/public_html/wp-includes/comment-template.php on line 1266

Strict Standards: Declaration of Walker_Comment::end_lvl() should be compatible with Walker::end_lvl(&$output) in /var/www/serverdude.dk/public_html/wp-includes/comment-template.php on line 1266

Strict Standards: Declaration of Walker_Comment::start_el() should be compatible with Walker::start_el(&$output) in /var/www/serverdude.dk/public_html/wp-includes/comment-template.php on line 1266

Strict Standards: Declaration of Walker_Comment::end_el() should be compatible with Walker::end_el(&$output) in /var/www/serverdude.dk/public_html/wp-includes/comment-template.php on line 1266

Strict Standards: Redefining already defined constructor for class WP_Dependencies in /var/www/serverdude.dk/public_html/wp-includes/class.wp-dependencies.php on line 31

Strict Standards: Redefining already defined constructor for class WP_Http in /var/www/serverdude.dk/public_html/wp-includes/http.php on line 61
juni « 2014 « Serverdude

Archive for juni, 2014

Bad Teaching

søndag, juni 29th, 2014

Someone is wrong on the internet!

Someone is wrong on the internet
(Visit http://xkcd.com/ for a lot more of similar stuff)

Making too many mistakes while trying to teach a concept is worse than not teaching at all.

Don’t get me wrong - I admire the people making an effort to teach and
especially when it is about how to program. But just as enthusiastic I am about those who can, I am frustrated and angry with those who really can’t. Unfortunately there are plenty of the those who can’t who try - that is most likely due to the Dunning-Kruger effect

For some odd reason I stumbled upon one of these bad teaching resources, http://javapostsforlearning.blogspot.in/2014/06/method-overriding-in-java.html.

I became so furious with the content. A teacher should know better. A teacher should do better.

In an effort to teach inheritance of Object Oriented Programming, specifically for Java, the author takes a simple example and by doing so violates principles of design and good practices.

Concrete vs. Abstract

One of the benefits of inheritance is the ability to use the interface or in case of a missing interface then the super class to abstract the
hierarchy.

In the code, MethodOverridingMain lines 9-12 the declared objects (left hand side) should be Employee.


package org.arpit.javapostsforlearning;

public class MethodOverridingMain {

    public static void main(String[] args) {
        Employee d1 = new Developer(1, "Arpit", 20000);
        Employee d2 = new Developer(2, "John", 15000);
        Employee m1 = new Manager(1, "Amit", 30000);
        Employee m2 = new Manager(2, "Ashwin", 50000);

        System.out.println("Name of Employee:" + d1.getEmployeeName() + "---"
                + "Salary:" + d1.getSalary());
        System.out.println("Name of Employee:" + d2.getEmployeeName() + "---"
                + "Salary:" + d2.getSalary());
        System.out.println("Name of Employee:" + m1.getEmployeeName() + "---"
                + "Salary:" + m1.getSalary());
        System.out.println("Name of Employee:" + m2.getEmployeeName() + "---"
                + "Salary:" + m2.getSalary());
    }
}

This is particular useful as the subclasses don’t provide additional methods. Now every employee can be thought of as an Employee.

Violation of Liskov Substitution Principle

When working with hierarchies - which is a natural part of inheritance - then it is important to adhere to best practices such as Liskov Substitution Principle (LSP), which states that if a program module is using a Base class, then the reference to the Base class can be replaced with a Derived class without affecting the functionality of the program module.

Why is this important? It allows any developers using your source code as a library to reduce the cognitive load to only be concerned with the base class, which is another reason why you should program to an interface and not a concrete implementation (Interface Segregation Principle).

The violation is in the getSalary method of Manager and Developer. For the base class employee what you set is what you get, not so for the others.

Let us say that we have a policy of dividing the surplus every month with equal shares to every employee. The code to set the new salary for the employees would look something like this:


    public static void divideSurplus(double surplus, List<Employee> employees) {
        if (employees != null && employees.size() > 0) {
            double share = surplus / employees.size();
            for (Employee employee : employees) {
                employee.setSalary(employee.getSalary() + share);
            }
        }
    }

Yes, this is ugly mutating code but let us not be concerned with this yet.

If every employee were created as Employee this would work, that is, if version 1 of the library only had Employee, then this would have been the implementation to do the work.

When employees are created as Developer and Manager as well as Employee the code doesn’t break, but the business logic does. You end up paying more than you have made. This is an extremely ugly side effect of not adhering to LSP.


import java.util.ArrayList;
import java.util.List;

import org.arpit.javapostsforlearning.Developer;
import org.arpit.javapostsforlearning.Employee;
import org.arpit.javapostsforlearning.Manager;

public class SurplusDivision {

    public static void divideSurplus(double surplus, List<Employee> employees) {
        if (employees != null && employees.size() > 0) {
            double share = surplus / employees.size();
            for (Employee employee : employees) {
                employee.setSalary(employee.getSalary() + share);
            }
        }
    }

    // cannot be used if salaries are 0
    public static void divideSurplus2(double surplus, List<Employee> employees) {
        if (employees != null && employees.size() > 0) {
            double share = surplus / totalSalaries(employees);
            for (Employee employee : employees) {
                employee.setSalary(employee.getSalary()*(1 + share));
            }
        }
    }

    public static double totalSalaries(List<Employee> employees) {
        double total = 0;
        for (Employee employee : employees) {
            total += employee.getSalary();
        }
        return total;
    }

    public static double claculateSalary(Employee employee) {
        return employee.getSalary();
    }

    public static void main(String[] args) {
        double revenue = 90000.0;
        List<Employee> employees = new ArrayList<>();
        employees.add(new Employee(1, "name1", 10000.0));
        employees.add(new Employee(2, "name2", 20000.0));
        employees.add(new Employee(3, "name3", 30000.0));
        double surplus = revenue - totalSalaries(employees); 

        divideSurplus(surplus, employees);
        System.out.println(totalSalaries(employees)); // prints 90000.0

        employees = new ArrayList<>();
        employees.add(new Employee (1, "name1", 10000.0));
        employees.add(new Developer(2, "name2", 20000.0));
        employees.add(new Manager  (3, "name3", 30000.0));
        surplus = revenue - totalSalaries(employees); 

        divideSurplus(surplus, employees);
        System.out.println(totalSalaries(employees)); // prints 101600.0

        divideSurplus(0, employees);
        System.out.println(totalSalaries(employees)); // prints 115226.66666666666

        // surplus 2
        employees = new ArrayList<>();
        employees.add(new Employee(1, "name1", 10000.0));
        employees.add(new Employee(2, "name2", 20000.0));
        employees.add(new Employee(3, "name3", 30000.0));
        surplus = revenue - totalSalaries(employees); 

        divideSurplus2(surplus, employees);
        System.out.println(totalSalaries(employees)); // prints 90000.0

        employees = new ArrayList<>();
        employees.add(new Employee (1, "name1", 10000.0));
        employees.add(new Developer(2, "name2", 20000.0));
        employees.add(new Manager  (3, "name3", 30000.0));
        surplus = revenue - totalSalaries(employees);

        divideSurplus2(surplus, employees);
        System.out.println(totalSalaries(employees)); // prints 102441.17647058822

        divideSurplus2(0, employees);
        System.out.println(totalSalaries(employees)); // prints 117079.41176470587

    }

}

The main method should print 90000.0 in every case, but it doesn’t.

Not only has the hierarchy broken the business case - it has also made it quite impossible to create correct.

Double the money

This is a widespread mistake - having a decimal point in the string representation of the number does not make it a viable currency indicator. Please go and read What Every Computer Scientist Should Know About Floating-Point Arithmetic

It is incredible that people over and over again seem to think that infinitely many elements can be stored in a finite machine.

Public Fields are Bad

Well, there are no public fields in the code shown, they are class protected fields. While that is technically true, the fact is that getters and setters galore is basically no better. Having this bean structure or promiscuous objects makes it easy to implement in imperative style, and extremely hard to preserve the codes maintainability because you have violated the core tenet of Object Oriented Programming: Encapsulation. Read Why getter and setter methods are evil for more on this.

How often is it required to set the Employee name, Id, or salary?

Hardcoded Values

The BONUSPERCENT constant for both Manager and Developer are hardcoded, that is if one manager is allowed a different bonus percentage then it is not possible. If every developer needs a different bonus, then the code needs to be recompiled.

Unclear or Misleading names

BONUSPERCENT is in the decimal representation, that is 0.2 = 20%.

Bad Design

While I do understand the notion that we seemingly have a hierarchy because a Developer is an Employee and a Manager is an Employee, there really is no reason for this. They are only different - in the provided example - through title, which apparently isn’t a part of the object, and how their salaries are calculated. If an existing employee becomes a manager or developer, we cannot shift their roles, but must create a new instance of the matching object - something there isn’t support for in the code provided.

So if the Employee had a Role associated, then something else could calculate the salary to be paid based upon the role. Naturally this wouldn’t help with explaining code inheritance, and it probably wouldn’t help with the surplus division.