Attachment 5131 Details for Bug 37241 – Code Review.htm - feedback on initial implementation

Code Review.htm - feedback on initial implementation

Code Review.htm (text/html), 6.86 KB, created by Jared Burns

on 2003-06-09 15:22:05 EDT

(hide)

Description:

Filename:

MIME Type:

Creator: Jared Burns

Created: 2003-06-09 15:22:05 EDT

Size: 6.86 KB

patch

obsolete

><html>
><head>
><title>Untitled Document</title>
><meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
></head>
>
><body bgcolor="#FFFFFF" text="#000000">
><h1>Launch Variables Code Review</h1>
><h2>General Comments</h2>
><ul>
>  <li> <code>package.html</code> documentation is missing for <code>org.eclipse.debug.core.variables</code>. 
>    Actually, all API packages require this brief description of the package. 
>    Any extra information you put in this file explaining how variables work is 
>    useful.</li>
>  <li>Somewhere the javadoc needs to explain that simple variables and context 
>    variables can be defined by extension points, with examples (as our other 
>    extension points do).</li>
>  <li>You shold re-use the static &quot;serializeDocument(..)&quot; method in 
>    the LaunchManager, rather than writing your own. This way, all our docs are 
>    written the same way (and we should likely expose this method better in a 
>    utility class)</li>
>  <li>In general we should create APIs that are as small as possible (i.e. less 
>    to maintain and doc).</li>
>  <li>the schema tag is missing for the &quot;simpleLaunchVariables&quot; extension 
>    point, and the generated doc is not released to the &quot;doc&quot; folder.</li>
>  <li>The new extension points need to be added to the ISV doc for the platform.</li>
>  <li>NLS'ing is required for the integration/milestone builds (else we fail CHKPII 
>    tests). </li>
>  <li>I started to update javadoc, but then gave up since there is lots missing 
>    (missing class comments, missing @since tags, missing param tags, missing 
>    return comments...)</li>
>  <li>This is a perfect opportunity to write tests - core (non-UI) function is 
>    the easiest to write tests for... - create a variable, set/get its value, 
>    define an extension point with an initializer, expand a variable....</li>
></ul>
><h3>ISimpleVariableRegistry</h3>
><ul>
>  <li> The <code>addVariable()</code> and <code>addVariables()</code> methods 
>    are redundant. I think we should go with the plural form only (since that 
>    is what happenned with other debug APIs - i.e. we added plural APIs where 
>    they were singular). </li>
>  <li>Similarly, I suggest to have a plural remove API - i.e. <code>removeVariables</code>(...)</li>
>  <li>The <code>clear()</code> method is not required (and can be removed - since 
>    I can simple remove all variables with the plural API). </li>
>  <li>The storeVariables() method should not be API. The implementation should 
>    persist variables whenever a variables is added/remove/changed to avoid losing 
>    data in a crash.</li>
>  <li>The implementation should store variables with user preferences such that 
>    the user can export/import variables from workspace to workspace.</li>
>  <li>addVariables(...) should throw an exception if there is an attempt to add 
>    a variable with a duplciate name</li>
></ul>
><h3>IContextLaunchVariableRegistry</h3>
><ul>
>  <li> missing class comment</li>
>  <li>javadoc for both methods missing @return flags</li>
>  <li>what's a &quot;tag&quot;? we should be consistent with our name vs. tag 
>    terminology - or describe the difference if there is one.</li>
>  <li>the implementation of ContextVariableLaunchRegistry should not be API - 
>    it should be in an internal package.</li>
></ul>
><h3>ISimpleLaunchVariable</h3>
><ul>
>  <li>javadoc should describe how to define a simple launch variable with an extension 
>    point, or with creation API.</li>
>  <li>Actually, I don't think clients should implement this interface - there 
>    is no need to. There should (instead), be a &quot;createVariable&quot; API 
>    on the variable registry - passing in a name and optional value. All other 
>    simple variables are defined by extension points, so we can instantiate them 
>    internally. The SimpleLaunchVariable implementation should be internal too.</li>
></ul>
><h3>IContextLaunchVariable</h3>
><ul>
>  <li>javadoc should explain how to define a context variable (i.e. via extension 
>    point), with an example</li>
>  <li>the implementation for a context launch variable should not be public - 
>    all context variables are defined by extension points?</li>
></ul>
><h3>IVariableInitializer</h3>
><ul>
>  <li>missing class comment</li>
>  <li>needs to describe what a variable initializer is, and how it relates to 
>    simple variables - i.e. via extension point</li>
></ul>
><h3>IVariableExpander</h3>
><ul>
>  <li> missig class comment</li>
>  <li>how do I define a variable expander?</li>
>  <li>how do I know if the variable expands to resources or a string?</li>
></ul>
><h3>ExpandVariableContext</h3>
><ul>
>  <li> I think this should be called a &quot;LaunchVariableContext&quot; - i.e. 
>    a context in which a launch variable is resolved/expanded</li>
>  <li>As well, we could define this more generically, as ILaunchVariableContext, 
>    which has no methods (or we could call it an Object). Since a client defines 
>    a context variable, they also define the context. We should not restrict contexts 
>    to resources/projects. The client that calls the utility to expand the variable 
>    provides the context - so they can provide anything and cast on the inside.</li>
></ul>
><h2>Variable Expanders in Debug UI</h2>
><ul>
>  <li>It seems that all the expanders could be internal implementations. They 
>    are defined via extension points. As long as the variables names (tags?) are 
>    known, then there is no need to expose our implementations. The constant definitions 
>    just need to explain what the variable provides.</li>
>  <li>Similarly, the components do not need to be API - only the Abstract class 
>    needs to be API. The other components are internal implementations.</li>
></ul>
><h3>ILaunchVariableComponentManager</h3>
><ul>
>  <li>There needs to be a public accessor to get at this manager. Better yet (since 
>    the interface is only one method), you could put the method in DebugUITools.</li>
></ul>
><h2>Issues</h2>
><ul>
>  <li>Should the implementation store variables with user preferences such that 
>    the user can export/import variables from workspace to workspace? (this would 
>    be nice, but I could see a problem with over-writing existing variables). 
>    Since variables are defined by extension points, perhaps this does not make 
>    sense.</li>
>  <li>Should there be variable notifications - i.e. add/change/remove notification, 
>    just like all our other services - breakpoints, expressions, etc.</li>
>  <li>Can we combine the variable registries (smaller number of classes)? Just 
>    add two methods to the simple registry: 
>    <ul>
>      <li>getContextVariable(tag)</li>
>      <li>getContextVariables()</li>
>    </ul>
>  </li>
>  <li>Better yet - the VariableUtil class should also be combined with the variable 
>    registries into an IVariableManager (with an internal implementation). This 
>    provides one place to retrieve/register/resolve variables. As well, this is 
>    where change notification would be centralized.</li>
></ul>
></body>
></html>

<html>
<head>
<title>Untitled Document</title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
</head>

<body bgcolor="#FFFFFF" text="#000000">
<h1>Launch Variables Code Review</h1>
<h2>General Comments</h2>
<ul>
  <li> <code>package.html</code> documentation is missing for <code>org.eclipse.debug.core.variables</code>. 
    Actually, all API packages require this brief description of the package. 
    Any extra information you put in this file explaining how variables work is 
    useful.</li>
  <li>Somewhere the javadoc needs to explain that simple variables and context 
    variables can be defined by extension points, with examples (as our other 
    extension points do).</li>
  <li>You shold re-use the static &quot;serializeDocument(..)&quot; method in 
    the LaunchManager, rather than writing your own. This way, all our docs are 
    written the same way (and we should likely expose this method better in a 
    utility class)</li>
  <li>In general we should create APIs that are as small as possible (i.e. less 
    to maintain and doc).</li>
  <li>the schema tag is missing for the &quot;simpleLaunchVariables&quot; extension 
    point, and the generated doc is not released to the &quot;doc&quot; folder.</li>
  <li>The new extension points need to be added to the ISV doc for the platform.</li>
  <li>NLS'ing is required for the integration/milestone builds (else we fail CHKPII 
    tests). </li>
  <li>I started to update javadoc, but then gave up since there is lots missing 
    (missing class comments, missing @since tags, missing param tags, missing 
    return comments...)</li>
  <li>This is a perfect opportunity to write tests - core (non-UI) function is 
    the easiest to write tests for... - create a variable, set/get its value, 
    define an extension point with an initializer, expand a variable....</li>
</ul>
<h3>ISimpleVariableRegistry</h3>
<ul>
  <li> The <code>addVariable()</code> and <code>addVariables()</code> methods 
    are redundant. I think we should go with the plural form only (since that 
    is what happenned with other debug APIs - i.e. we added plural APIs where 
    they were singular). </li>
  <li>Similarly, I suggest to have a plural remove API - i.e. <code>removeVariables</code>(...)</li>
  <li>The <code>clear()</code> method is not required (and can be removed - since 
    I can simple remove all variables with the plural API). </li>
  <li>The storeVariables() method should not be API. The implementation should 
    persist variables whenever a variables is added/remove/changed to avoid losing 
    data in a crash.</li>
  <li>The implementation should store variables with user preferences such that 
    the user can export/import variables from workspace to workspace.</li>
  <li>addVariables(...) should throw an exception if there is an attempt to add 
    a variable with a duplciate name</li>
</ul>
<h3>IContextLaunchVariableRegistry</h3>
<ul>
  <li> missing class comment</li>
  <li>javadoc for both methods missing @return flags</li>
  <li>what's a &quot;tag&quot;? we should be consistent with our name vs. tag 
    terminology - or describe the difference if there is one.</li>
  <li>the implementation of ContextVariableLaunchRegistry should not be API - 
    it should be in an internal package.</li>
</ul>
<h3>ISimpleLaunchVariable</h3>
<ul>
  <li>javadoc should describe how to define a simple launch variable with an extension 
    point, or with creation API.</li>
  <li>Actually, I don't think clients should implement this interface - there 
    is no need to. There should (instead), be a &quot;createVariable&quot; API 
    on the variable registry - passing in a name and optional value. All other 
    simple variables are defined by extension points, so we can instantiate them 
    internally. The SimpleLaunchVariable implementation should be internal too.</li>
</ul>
<h3>IContextLaunchVariable</h3>
<ul>
  <li>javadoc should explain how to define a context variable (i.e. via extension 
    point), with an example</li>
  <li>the implementation for a context launch variable should not be public - 
    all context variables are defined by extension points?</li>
</ul>
<h3>IVariableInitializer</h3>
<ul>
  <li>missing class comment</li>
  <li>needs to describe what a variable initializer is, and how it relates to 
    simple variables - i.e. via extension point</li>
</ul>
<h3>IVariableExpander</h3>
<ul>
  <li> missig class comment</li>
  <li>how do I define a variable expander?</li>
  <li>how do I know if the variable expands to resources or a string?</li>
</ul>
<h3>ExpandVariableContext</h3>
<ul>
  <li> I think this should be called a &quot;LaunchVariableContext&quot; - i.e. 
    a context in which a launch variable is resolved/expanded</li>
  <li>As well, we could define this more generically, as ILaunchVariableContext, 
    which has no methods (or we could call it an Object). Since a client defines 
    a context variable, they also define the context. We should not restrict contexts 
    to resources/projects. The client that calls the utility to expand the variable 
    provides the context - so they can provide anything and cast on the inside.</li>
</ul>
<h2>Variable Expanders in Debug UI</h2>
<ul>
  <li>It seems that all the expanders could be internal implementations. They 
    are defined via extension points. As long as the variables names (tags?) are 
    known, then there is no need to expose our implementations. The constant definitions 
    just need to explain what the variable provides.</li>
  <li>Similarly, the components do not need to be API - only the Abstract class 
    needs to be API. The other components are internal implementations.</li>
</ul>
<h3>ILaunchVariableComponentManager</h3>
<ul>
  <li>There needs to be a public accessor to get at this manager. Better yet (since 
    the interface is only one method), you could put the method in DebugUITools.</li>
</ul>
<h2>Issues</h2>
<ul>
  <li>Should the implementation store variables with user preferences such that 
    the user can export/import variables from workspace to workspace? (this would 
    be nice, but I could see a problem with over-writing existing variables). 
    Since variables are defined by extension points, perhaps this does not make 
    sense.</li>
  <li>Should there be variable notifications - i.e. add/change/remove notification, 
    just like all our other services - breakpoints, expressions, etc.</li>
  <li>Can we combine the variable registries (smaller number of classes)? Just 
    add two methods to the simple registry: 
    <ul>
      <li>getContextVariable(tag)</li>
      <li>getContextVariables()</li>
    </ul>
  </li>
  <li>Better yet - the VariableUtil class should also be combined with the variable 
    registries into an IVariableManager (with an internal implementation). This 
    provides one place to retrieve/register/resolve variables. As well, this is 
    where change notification would be centralized.</li>
</ul>
</body>
</html>

Actions: View

Attachments on bug 37241: 5131