zilzar / google-gdata

Automatically exported from code.google.com/p/google-gdata
0 stars 0 forks source link

Internal Constructors #516

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Hi,
Is there a reasons why some constructors are marked as "internal"?
E.g. GDataGAuthRequest?
I'd like to inherit the class in my own code so I can add some extra logging 
but I can't without modifying the code because the constructor is internal.
Can you please make all constructors public? There should be no reason for the 
framework to limit me in what I can do with it.
Thanks,
Corneliu

Original issue reported on code.google.com by corneliu...@gtempaccount.com on 18 Jun 2011 at 1:43

GoogleCodeExporter commented 9 years ago
Hi Corneliu,

The goal of a good API is to expose externally only those functionalities that 
are required and hide as many implementation details as possible.
Making some constructors internal is one of our design choices.

Claudio

Original comment by ccherub...@google.com on 18 Jun 2011 at 6:38

GoogleCodeExporter commented 9 years ago
Claudio,

If you want to hide implementation details you make classes internal as
well. Making constructors only internal will only get users upset in not
being able to use them.
Also, you should only do that if you provide other extensibility ways.
Otherwise all you achieve is a closed and not-extensible API not fun to work
with.
For example GAuthSubRequestFactory does not have a logging version of it so
if I want to do some logging in the GAuthSubRequest I need to extend it.
Also, the current logging classes all assume I want a file which is not the
scenario for me for example as I need the logging to be consolidated with
other logging. In order to extend I had to do several changes:

   1. Make GAuthSubRequestFactory constructor public - so I can extend the
   class
   2. Make GAuthSubRequest constructor public - so I can extend the class
   3. Make responseStream in the GAuthSubRequest protected so I can "swap"
   it after I read and log it (the streams can be read only once so if I want
   to log I need to read in memory then swap it so the base class can still
   work)
   4. Make the RequestCopy stream in the GAuthSubRequest protected so I can
   read it as well and reset it back to beginning so the request can proceed.

I don't mind changing the source code but maybe some other people do.

Hiding is good when there are build in extensibility points.

My 2 cents,
Corneliu

Original comment by corneliu...@gtempaccount.com on 19 Jun 2011 at 5:53

GoogleCodeExporter commented 9 years ago
Hi Corneliu,

Some of the design choices were made before I joined this team and I may have 
missed some background but nothing is set in stone and a valid suggestion is 
always accepted.

I see your point and I appreciate your contribution, would you like to submit a 
patch to change the constructors' visibility as appropriate?
Thanks

Claudio

Original comment by ccherub...@google.com on 20 Jun 2011 at 9:43

GoogleCodeExporter commented 9 years ago
Hi Claudio,

I will prepare a patch once I do all the changes I want and it's all stable.

Original comment by corneliu...@gtempaccount.com on 20 Jun 2011 at 11:34

GoogleCodeExporter commented 9 years ago
Marking this as obsolete, feel free open a new entry to propose your patch. 

Original comment by ccherub...@google.com on 27 Dec 2011 at 4:18