comment on factory method coding

[-]

i have a superclass 'A', two subclasses 'A1' and 'A2' and a factory
class 'AFactory'.

there is a method inside 'AFactory' called public static A createA()
which returns an instance of 'A', 'A1' or 'A2' depending on the values
of x and y.

public static A createA(String string) {
// parses string and assigns x and y
...
...
x = ...
y = ...


try {
return new A1(x, y);
} catch (IllegalArgumentException ex) {
}

try {
return new A2(x, y);
} catch (IllegalArgumentException ex) {
}

return new A(x, y);
}

1) is the above try and catch code considered non-ugly for a factory method?

2) i reckon that this method should be placed inside AFactory class
rather than A since the A class should not be aware of how many
subclasses it has.

 

[Vincent Cantin]

i have a superclass 'A', two subclasses 'A1' and 'A2' and a factory class
'AFactory'.

there is a method inside 'AFactory' called public static A createA() which
returns an instance of 'A', 'A1' or 'A2' depending on the values of x and
y.

public static A createA(String string) {
// parses string and assigns x and y
...
...
x = ...
y = ...


try {
return new A1(x, y);
} catch (IllegalArgumentException ex) {
}

try {
return new A2(x, y);
} catch (IllegalArgumentException ex) {
}

return new A(x, y);
}

1) is the above try and catch code considered non-ugly for a factory
method?

This is very ugly, and here are some reasons why :
- Here you suppose that A1 and A2 are build with some constructors of the
same signature, it is not always the case.
- Instead of trying to build some correctly, you try everything that doesn't
cast a RuntimeException .. how can A1 or A2 knows if they are the classes
that the user want to create ?? What if there is no exception with "new
A1(x, y)" AND "with A2(x, y)" ? ... it would mean that the type of the
returned object depends of the implementation of the factory ... so ... why
do you put the selection condition in the constructor of the class ?! Non
sense. You should put the condition inside the createA(), not in the
constructor. A constructor should construct, not check if you really want to
construct.

Vincent Cantin

 

[-]

This is very ugly, and here are some reasons why :
- Here you suppose that A1 and A2 are build with some constructors of the
same signature, it is not always the case.
- Instead of trying to build some correctly, you try everything that doesn't
cast a RuntimeException .. how can A1 or A2 knows if they are the classes
that the user want to create ?? What if there is no exception with "new
A1(x, y)" AND "with A2(x, y)" ? ... it would mean that the type of the
returned object depends of the implementation of the factory ... so ... why
do you put the selection condition in the constructor of the class ?! Non
sense. You should put the condition inside the createA(), not in the
constructor. A constructor should construct, not check if you really want to
construct.

in my case, they have the same signature.

secondly, x is a variable that can either be, for instance a letter, a
number or a punctuation.

if x is a number, A1 is returned. if x is a letter, A2 is returned. if
x is not a number or a letter, A is returned.

the reason why i do not put the condition inside the createA() is
because the constructor validate the arguments and will return an
IllegalArgumentException respectively. hence the try catch codes.

try {
return new A1(x, y);
} catch (IllegalArgumentException ex) {
}

try {
return new A2(x, y);
} catch (IllegalArgumentException ex) {
}

return new A(x, y);

one advantage i can think of is that it does not require me to recheck
the arguments and simply throw it to the constructor to check it one time.

based on the above rationale, is it now acceptable or should i still
need to revamp it?

 

[Vincent Cantin]

in my case, they have the same signature.

secondly, x is a variable that can either be, for instance a letter, a
number or a punctuation.

if x is a number, A1 is returned. if x is a letter, A2 is returned. if x
is not a number or a letter, A is returned.

So you are in a specific special case. Your previous question didn't precise
this, thus my answer was about the general way to design programs.

the reason why i do not put the condition inside the createA() is because
the constructor validate the arguments and will return an
IllegalArgumentException respectively. hence the try catch codes.

My thoughs are still the same that what I previously say about your special
usage of the runtime exceptions.

try {
return new A1(x, y);
} catch (IllegalArgumentException ex) {
}

try {
return new A2(x, y);
} catch (IllegalArgumentException ex) {
}

return new A(x, y);

one advantage i can think of is that it does not require me to recheck the
arguments and simply throw it to the constructor to check it one time.

If your classes are only built from your factory, then you don't have to
check the arguments in the constructor.

based on the above rationale, is it now acceptable or should i still need
to revamp it?

If you prefer to implement special cases instead of easy to
use-and-understand-and-integrate design patterns, then yes it is now
acceptable.


Regards, Vincent