0
\$\begingroup\$

Which approach is better and why?

First approach (inner class for wrapping):

public class OOP {
    private SomeService someService;

    private final class OrganizationPrimaryKeyString implements Serializable {
        private static final long serialVersionUID = -256615521081939636L;
        private final String pkString

        public OrganizationPrimaryKeyString(final OrganizationEnum organizationEnum) {
            final PK pk = someService.getValue(organizationEnum).getPk();

            if (pk == null) {
                throw new IllegalArgumentException("Can't be null");
            }

            pkString = pk.getLongValueAsString();
        }

        @Override
        public String toString() {
            return pkString;
        }
    }

    public List<Information> getInformation(final OrganizationEnum organizationEnum) {
        final OrganizationPrimaryKeyString organization = new OrganizationPrimaryKeyString(organizationEnum);

        try {
            final SessionContext ctx = createAndSetupLocalSessionContext(organization);

            (...)

            return (...);
        } catch (final FactoryException e) {
            throw new SystemException(e.getMessage(), e);
        } finally {
            closeLocalSessionContext();
        }
    }

    private SessionContext createAndSetupLocalSessionContext(final OrganizationPrimaryKeyString organization) {
        (...)
        sessionContext.setAttribute("organization", salesOrganization.toString());

        return sessionContext;
    }
}

Second approach (class method):

public class OOP {
    private SomeService someService;

    public List<Information> getInformation(final OrganizationEnum organizationEnum) {
        try {
            final SessionContext ctx = createAndSetupLocalSessionContext(organizationEnum);

            (...)

            return (...);
        } catch (final FactoryException e) {
        throw new SystemException(e.getMessage(), e);
        } finally {
            closeLocalSessionContext();
        }
    }

    private SessionContext createAndSetupLocalSessionContext(final OrganizationEnum organizationEnum) {
        (...)
        sessionContext.setAttribute("organization", getPkString(organizationEnum));

        return sessionContext;
    }

    private String getPkString(final OrganizationEnum organizationEnum) {
        final PK pk = someService.getValue(organizationEnum).getPk();

        if (pk == null) {
            throw new IllegalArgumentException("Can't be null");
        }

        return pk.getLongValueAsString();
    }
}
\$\endgroup\$
2
\$\begingroup\$

I think the first approach is the best; it uses more design-patterns:

  1. You throw an IllegalArgumentExcaption but pk is not an argument of the method.

    From JavaDoc:

    Thrown to indicate that a method has been passed an illegal or inappropriate argument.

  2. You throw an IllegalArgumentExcaption because of a null. It should be a NullPointerException - it's more meaningful. Objects.requreNonNull() does it for you automatically. A NullPointerException would be automatically thrown in 2 lines later, so no need for a check except you would like to add a custom message to the NullPointerException. That would be a good idea.
  3. The Class who has the .getPk()-method can have a null as a PrimaryKey; it looks like a design-flaw? Anyway, if this instance who has the .getPk()-method can have an illegal state, then an IllegalStateException should be thrown.
  4. The toString() implementation is good.
  5. OrganizationPrimaryKeyString is a good read-only object. Read-only objects are good for memory because you do not need to have two instances of the same organisation. But if you call getInformation() one million times for ibm, then every call to getInformation() returns one million equal ibm-objects that are not the same but require its own memory in the heap! A HashMap may help caching, so every call for ibm can return one of the same object.
  6. The naming is good and meaningful. getInformation() should be named getInformations() and OrganizationPrimaryKeyString should only be named OrganizationPrimaryKey.
  7. OrganizationPrimaryKey should be static because YDKJ: A subclass that is not static automatically has the wrapped-class as constructor-argument who internally is cached as a field of OrganizationPrimaryKey. Trust me, every Constructor of OrganizationPrimaryKey caches the OOP-Instance.
\$\endgroup\$
  • \$\begingroup\$ 7th require to not use the someService-class-variable in the OrganizationPrimaryKey \$\endgroup\$ – Grim Nov 28 '14 at 12:32
  • \$\begingroup\$ 1. & 2. Good point 3. This is method from 3rd part library 4. thx 5. There will be maximum 1000 invocation per day. but this is good option 6. ok \$\endgroup\$ – KirkoR Nov 28 '14 at 12:57
  • \$\begingroup\$ I disagree with the point you make concerning 2. Some methods are very well off with null as argument. I'd never expect a NPE for passing in something as an argument. Rather than that I'd prefer recieving IAE, since.. "a method has been passed an illegal or inappropriate argument" \$\endgroup\$ – Vogel612 Nov 28 '14 at 13:13
  • 1
    \$\begingroup\$ @Vogel612 indeed, this point is often in critic! I tend to rely on Objects.requireNonNull() because the JavaDoc sais This method is designed primarily for doing parameter validation in joyously expecting to use Lombok's @NonNull in the future. Btw.: Even Lombok tend to NPE by default. \$\endgroup\$ – Grim Nov 28 '14 at 13:21
  • 1
    \$\begingroup\$ @Vogel612 ... and so does Guava. Unlike Peter, they even give precedence to NPE over ISE. \$\endgroup\$ – maaartinus Nov 28 '14 at 17:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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