Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up the utility classes and methods #2383

Closed
AaronGreenhouse opened this issue Jul 14, 2020 · 21 comments · Fixed by #2487
Closed

Clean up the utility classes and methods #2383

AaronGreenhouse opened this issue Jul 14, 2020 · 21 comments · Fixed by #2487
Assignees
Milestone

Comments

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented Jul 14, 2020

I searched through for classes in the workspace that contain Util in the name. I came up with this list after filtering out some classes that were obviously related to supported Xtext or some other framework:

  • org.osate.aadl2.instance.util.InstanceUtil
  • org.osate.aadl2.parsesupport.ParseUtil
  • org.osate.aadl2.util.Aadl2InstanceUtil
  • org.osate.aadl2.util.Aadl2Util
  • org.osate.aadl2.modelsupport.resources.OsateResourceUtil
  • org.osate.aadl2.modelsupport.scoping.Aadl2GlobalScopeUtil
  • org.osate.aadl2.modelsupport.util.AadlUtil
  • org.osate.aadl2.modelsupport.util.ResolvePrototypeUtil
  • org.osate.analysis.flows.FlowLatencyUtil
  • org.osate.analysis.flows.reporting.utils.ReportUtils
  • org.osate.annexsupport.AnnexParseUtil
  • org.osate.annexsupport.AnnexUtil
  • org.osate.importer.Utils
  • org.osate.importer.simulink.Utils
  • org.osate.importer.simulink.generator.Utils
  • org.osate.pluginsupport.ExecuteJavaUtil
  • org.osate.pluginsupport.PluginSupportUtil
  • org.osate.result.util.ResultUtil
  • org.osate.ge.internal.ui.util.UiUtil
  • org.osate.ui.UiUtil
  • org.osate.ui.utils.FileUtils
  • org.osate.utils.Aadl2Utils
  • org.osate.utils.Aadl2Visitors
  • org.osate.utils.FileUtils
  • org.osate.utils.FloatUtil
  • org.osate.utils.FloatRange
  • org.osate.utils.IntegerRange
  • org.osate.utils.PropertyUtils
  • org.osate.utils.UnitConversion
  • org.osate.verify.util.VerifyJavaUtil
  • org.osate.verify.util.VerifyUtilExtension
  • org.osate.xtext.aadl2.properties.util.InstanceModelUtil
  • org.osate.xtext.aadl2.properties.util.PropertyUtils

These classes need to be inspected, consolidated, and @deprecated.

Some work has already been one on OsateResourceUtil. I think this was previously deprecated and if so, should be able to be removed immediately.

Most of the property-related stuff I think is being replaced by issue #2113.

Some of this following

  • org.osate.annexsupport.AnnexParseUtil
  • org.osate.annexsupport.AnnexUtil
  • org.osate.pluginsupport.ExecuteJavaUtil
  • org.osate.pluginsupport.PluginSupportUtil

is related to supporting stand-alone "headless" Eclipse execution, that should have it's own issue.

@AaronGreenhouse AaronGreenhouse self-assigned this Jul 14, 2020
@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jul 14, 2020

org.osate.analysis.flows.FlowLatencyUtil should be made non public by making it an internal package.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jul 14, 2020

Looking at package org.osate.utils. The whole thing seems questionable, although there are some users of it.

  • Unused: FloatRange and IntegerRange, PropertyNotFound (a Runtime exception)
  • UnitConversion has only one method convertInMs(). That method is only called by PropertyUtils.getFloatValue(NamedElement, String, String), which itself doesn't seem to be used by anyone. It's a questionable use of convertInMs because it is not guaranteed to be dealing with time units.
  • FloatUtil is only used by UnitConversion
  • FileUtils is only used by AadlBaExamplesWizard (the copy files method). But this whole class seems unnecessary because it doesn't use the Eclipse Resource API, rather it used java.io. AadlBaExamplesWizard should be changed to use the correct eclipse API.
  • PropertyUtils is only used in the org.osate.ba project. Seems obsolete, especially given Joe's work on a property getter code generator.

Oh, I see. I this project/project is part of the ba subtree. Still, it can be cleaned up, and it should not be public the whole world. It's very misleading considering the package name is org.osate.utils.

  • Aadl2Utils and Aadl2Visitors see more heavy usage within the ba subtree, and may be harder to get rid of. But we still find a way of shielding them from public use.
  • Should be made .internal.

@AaronGreenhouse
Copy link
Contributor Author

The class org.osate.aadl2.modelsupport.resources.OsateResourceUtil now only has the methods toIFile(URI) and toResourceURI(IResource). These methods are used a lot and are unfortunately necessary for dealing mapping between EMF and Eclipse file resources.

@AaronGreenhouse
Copy link
Contributor Author

Actually, the whole plugin org.osate.aadl2.modelsupport seems to want to be part of the API.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jul 14, 2020

Class org.osate.ui.utils.FileUtils is a copy of org.osate.ui.FileUtils!

It seems to be there because org.osate.ui.wizards.AadlExamplesWizard seems to be a. copy and modify of AadlBaAbstractyWizard?

In any case, FileUtils should be a replaced with appropriate Eclipse resource apis.

@AaronGreenhouse
Copy link
Contributor Author

Class org.osate.ge.internal.ui.util.UiUtil is not the same as class org.osate.ui.UiUtil. The GE version is at least marked as internal so we can leave that one alone.

@AaronGreenhouse
Copy link
Contributor Author

Class org.osate.ui.UiUtil is a reasonable concept. Seems like it should be part of the 3rd party UI, but needs some cleanup.

  • The methods BestDecPoint(), OneDecPoint(), and TwoDecPoint() seem out of place. They are used by the latency analysis. (TwoDecPoint() is not used at all.)
  • Not sure about the usefulness of executeCommand() and openEditorAndExcecute(). The first one isn't used, and the second one is only used by AbstractAaxlEditorMarkerResolution which isn't used anywhere. Seems obsolete.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jul 15, 2020

org.osate.aadl2.instance.util.InstanceUtil looks important: deals with prototype resolution. According to Lutz it was only meant to be used during instantiation. Apparently Peter added a few more methods to it.

  • getComponentClassifier() is used by org.osate.analysis.resource.management.handlers.Binpack which is probably wrong.
  • isNoMode() is called from Aadl2Util printable SOM methods. Also used from EndToEndFlowInstance.isActive() which is weird because it doesn't resemble the other isActive() methods. In any case, this method should be moved somewhere else.
  • There are several methods that aren't used at all.
  • This class should be put into an .internal. package

How does InstanceUtil relate to org.osate.aadl2.modelsupport.util.ResolvePrototypeUtil which also deal with prototypes?

  • This is used by the graphical editor and the properties linking/scoping classes.

@AaronGreenhouse
Copy link
Contributor Author

Note: Utility classes should be final and contain a single private constructor.

@AaronGreenhouse
Copy link
Contributor Author

Class org.osate.aadl2.parsesupport.ParseUtil also seems important. Looks like it supports parsing in annexes and sublanguages? Need more info on this.

@AaronGreenhouse
Copy link
Contributor Author

Most of the methods in org.osate.aadl2.util.Aadl2InstanceUtil are not used. But some of them are used in the instantiation process and in other places.

@AaronGreenhouse
Copy link
Contributor Author

Most of org.osate.aadl2.util.Aadl2Util seems legit.

Question:

  • Should we always be using isNull()? When do we have to worry about proxy nodes?

@AaronGreenhouse
Copy link
Contributor Author

Class org.osate.aadl2.modelsupport.scoping.Aadl2GlobalScopeUtil seems important; supports annex and sub-languages. Seems strange it is in a different plugin than ParseUtil.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jul 15, 2020

Aadl2InstanceUtil.getResourcePath() and Aadl2InstanceUtil.makeSureFoldersExist() are out of place. Should be in OsateResourceUtil?

AadlUtil does appear to get a lot of use. But it should probably also be split up into different subjects.

  • Seems to be declarative model related.

@AaronGreenhouse
Copy link
Contributor Author

org.osate.analysis.flows.reporting.utils.ReportUtils only contains getReportPath(). This method seems generally useful for commands that generate reports. Should probably me merged into what ever replaces AbstractAaxlHandler.

The method is currently used directly from the GenericExport class, but this class should not directly produce the path name it. It should receive it as a parameter from the command handler.

@AaronGreenhouse
Copy link
Contributor Author

I don't understand the classes

  • org.osate.verify.util.VerifyJavaUtil
  • org.osate.verify.util.VerifyUtilExtension

@AaronGreenhouse
Copy link
Contributor Author

We should definitely start using the Eclipse .internal. convention to prevent abuse of classes (such as InstanceUtil).

See (https://wiki.eclipse.org/Naming_Conventions)

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jul 20, 2020

All these classes (if we keep them) should be made final and have a private no-arg constructor.

org.osate.aadl2.instance.util.InstanceUtil

Deals with prototype resolution. According to Lutz it was only meant to be used during instantiation. Apparently Peter added a few more methods to it.

  • getComponentClassifier() is used by org.osate.analysis.resource.management.handlers.Binpack which is probably wrong.
  • isNoMode() is called from Aadl2Util printable SOM methods. Also used from EndToEndFlowInstance.isActive() which is weird because it doesn't resemble the other isActive() methods. In any case, this method should be moved somewhere else.
  • There are several methods that aren't used at all.
  • This class should be put into an .internal. package.

org.osate.aadl2.parsesupport.ParseUtil

Looks like it supports parsing in annexes and sublanguages? Need more info on this, but needs to be public.

org.osate.aadl2.util.Aadl2InstanceUtil

  • Most of the methods are not used. But some of them are used in the instantiation process and in other places.

org.osate.aadl2.util.Aadl2Util

  • Most of the methods seem legit.
  • Should we always be using isNull()? When do we have to worry about proxy nodes?

org.osate.aadl2.modelsupport.resources.OsateResourceUtil

  • Npw only has the methods toIFile(URI) and toResourceURI(IResource). These methods are used a lot and are unfortunately necessary for dealing mapping between EMF uris and Eclipse file resources.

org.osate.aadl2.modelsupport.scoping.Aadl2GlobalScopeUtil

  • Seems important: supports annex and sub-languages. Seems strange it is in a different plugin than ParseUtil.

org.osate.aadl2.modelsupport.util.AadlUtil

  • Does not appear to get a lot of use. But it should probably also be split up into different subjects.
  • Seems to be declarative model related.
  • getResourcePath() and makeSureFoldersExist() are out of place. Should be in OsateResourceUtil?

org.osate.aadl2.modelsupport.util.ResolvePrototypeUtil

  • This is used by the graphical editor and the properties linking/scoping classes.
  • Still not entirely sure what it does

org.osate.analysis.flows.FlowLatencyUtil

  • This is only meant for the flow latency analysis. Should be made .internal.

org.osate.analysis.flows.reporting.utils.ReportUtils

  • Only contains getReportPath(). This method seems generally useful for commands that generate reports. Should probably me merged into what ever replaces AbstractAaxlHandler.
  • The method is currently used directly from the GenericExport class, but this class should not directly produce the path name it. It should receive it as a parameter from the command handler.

org.osate.annexsupport.AnnexParseUtil

  • Supports annex parsing, seems legit.

org.osate.annexsupport.AnnexUtil

  • Supports annex parsing, seems legit.

org.osate.importer.Utils

  • Only used within the same plugin. Should be made .internal.

org.osate.importer.simulink.Utils

  • Only used within the same plugin. Should be made .internal.

org.osate.importer.simulink.generator.Utils

  • Only used within the same plugin. Should be made .internal.

org.osate.pluginsupport.ExecuteJavaUtil

This is used a few plugins. Seems legit, but my question is why do we do this?

org.osate.pluginsupport.PluginSupportUtil

  • Gets the list of plugin-contributed property sets. Definitely necessary, but might need a clean up.

org.osate.result.util.ResultUtil

  • These are used to build and manage anlaysis result objects. Defintely necessary, but might need a clean up.

org.osate.ge.internal.ui.util.UiUtil

  • Already internal so we can ignore it.

org.osate.ui.UiUtil

  • Seems like a reasonable conncept should be part of the 3rd party UI, but needs some cleanup.
  • The methods BestDecPoint(), OneDecPoint(), and TwoDecPoint() seem out of place. They are used by the latency analysis. (TwoDecPoint() is not used at all.)
  • Not sure about the usefulness of executeCommand() and openEditorAndExcecute().
    The first one isn't used, and the second one is only used by AbstractAaxlEditorMarkerResolution which isn't used anywhere. Seems obsolete.

org.osate.ui.utils.FileUtils

  • A copy of org.osate.ui.FileUtils!
  • Seems to be there because org.osate.ui.wizards.AadlExamplesWizard seems to be a. copy and modify of AadlBaAbstractyWizard?

In any case, FileUtils should be a replaced with appropriate Eclipse resource apis.

org.osate.utils.Aadl2Utils & org.osate.utils.Aadl2Visitors & org.osate.utils.FileUtils & org.osate.utils.FloatUtil & org.osate.utils.FloatRange & org.osate.utils.IntegerRange & org.osate.utils.PropertyUtils & org.osate.utils.UnitConversion

  • The whole thing seems questionable, although it only is used within the 'ba' subtree. Can just make it .internal. and be done with it. But can also get rid of junk.
  • FloatRange and IntegerRange, PropertyNotFound (a Runtime exception) are unused.
  • UnitConversion has only one method convertInMs(). That method is only called by PropertyUtils.getFloatValue(NamedElement, String, String), which itself doesn't seem to be used by anyone. It's a questionable use of convertInMs because it is not guaranteed to be dealing with time units.
  • FloatUtil is only used by UnitConversion
  • FileUtils is only used by AadlBaExamplesWizard (the copy files method). But this whole class seems unnecessary because it doesn't use the Eclipse Resource API, rather it uses java.io. AadlBaExamplesWizard should be changed to use the correct eclipse API.
  • Aadl2Utils and Aadl2Visitors see more heavy usage within the ba subtree, and may be harder to get rid of. But we still find a way of shielding them from public use.
    Should be made .internal.

org.osate.verify.util.VerifyJavaUtil & org.osate.verify.util.VerifyUtilExtension

  • I don't understand these classes.

org.osate.xtext.aadl2.properties.util.InstanceModelUtil & org.osate.xtext.aadl2.properties.util.PropertyUtils

  • These are used and probably contains useful methods.
  • PropertyUtils may be obsolete now?

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jul 29, 2020

Had a long discussion with @joeseibel about these classes. Most of them are not meant to be public API. Some of them need further review to determine which parts of them are worth making public, and possibly splitting up.

All these classes (if we keep them) should be made final and have a private no-arg constructor.

org.osate.aadl2.instance.util.InstanceUtil

  • Inspect BinPack and figure out why it is using InstanceUtil. @joeseibel things it might be legitimate, but needs further review.
  • Move isNoMode() to SystemOperationMode.
  • Move the class to an .internal. package.

org.osate.aadl2.parsesupport.ParseUtil

  • Keep this as is.

org.osate.aadl2.util.Aadl2InstanceUtil

  • Needs a review

org.osate.aadl2.util.Aadl2Util

  • Needs a review: What can be moved to the meta-model?
  • What can be trashed?
  • isNull() should only be called from the validator? (Says @joeseibel )

org.osate.aadl2.modelsupport.resources.OsateResourceUtil

  • Npw only has the methods toIFile(URI) and toResourceURI(IResource). These methods are used a lot and are unfortunately necessary for dealing mapping between EMF uris and Eclipse file resources.

org.osate.aadl2.modelsupport.scoping.Aadl2GlobalScopeUtil

  • Keep as is.

org.osate.aadl2.modelsupport.util.AadlUtil

  • Revisit this class
  • isok methods should be used by the validator only.
  • getContaining methods should be replaced with uses of EcoreUtil2.getContainerOfType().
  • getResource() is never used
  • makeSureFoldersExist() is duplicated by a better version in NewAbstractAaxlHandler. Should be part of the whatever new abstract command handler is created.
    vvtevlvvenjggebcncludlbcvvbuneuefutjhjrvitrb

org.osate.aadl2.modelsupport.util.ResolvePrototypeUtil

  • (Tries to resolve prototypes from the declarative model)
  • Should be .internal.

org.osate.analysis.flows.FlowLatencyUtil

  • Should be made .internal. — Not even exported from the plugin.

org.osate.analysis.flows.reporting.utils.ReportUtils

  • Only contains getReportPath(). This method seems generally useful for commands that generate reports. Should probably me merged into what ever replaces AbstractAaxlHandler.
  • The method is currently used directly from the GenericExport class, but this class should not directly produce the path name it. It should receive it as a parameter from the command handler.

org.osate.annexsupport.AnnexParseUtil

  • Public for annex parsers
  • Revisit: some methods should be private
  • Merge with AnnexUtil?

org.osate.annexsupport.AnnexUtil

  • Keep as is.

org.osate.importer.Utils

  • Only used within the same plugin. Should be made .internal.
  • On the other hand, should the plugin be removed entirely?

org.osate.importer.simulink.Utils

  • Only used within the same plugin. Should be made .internal.
  • On the other hand, should the plugin be removed entirely?

org.osate.importer.simulink.generator.Utils

  • Only used within the same plugin. Should be made .internal.
  • On the other hand, should the plugin be removed entirely?

org.osate.pluginsupport.ExecuteJavaUtil

  • Should be .internal. and only used from emv2 and alisa.

org.osate.pluginsupport.PluginSupportUtil

  • Should be .internal.
  • Possible rename, has more to do with contributed property sets than anything else.

org.osate.result.util.ResultUtil

  • Keep as is.

org.osate.ge.internal.ui.util.UiUtil

  • Already internal.

org.osate.ui.UiUtil

  • @joeseibel says It would be better to use IURIEditorOpener instead of UiUtil.openEditor()
  • Seems like a reasonable concept should be part of the 3rd party UI, but needs some cleanup.
  • The methods BestDecPoint(), OneDecPoint(), and TwoDecPoint() seem out of place. They are used by the latency analysis. (TwoDecPoint() is not used at all.)
  • Not sure about the usefulness of executeCommand() and openEditorAndExcecute().
    The first one isn't used, and the second one is only used by AbstractAaxlEditorMarkerResolution which isn't used anywhere. Seems obsolete.

org.osate.ui.utils.FileUtils

org.osate.utils.Aadl2Utils & org.osate.utils.Aadl2Visitors & org.osate.utils.FileUtils & org.osate.utils.FloatUtil & org.osate.utils.FloatRange & org.osate.utils.IntegerRange & org.osate.utils.PropertyUtils & org.osate.utils.UnitConversion

  • Make .internal. and only use in the ba plugins.

org.osate.verify.util.VerifyJavaUtil & org.osate.verify.util.VerifyUtilExtension

  • Make .internal. and only use in the alisa plugins.

org.osate.xtext.aadl2.properties.util.InstanceModelUtil

  • SHould be .internal.

org.osate.xtext.aadl2.properties.util.PropertyUtils

  • Almost certainly this is obsoleted by the properties code generator.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Aug 3, 2020

In project org.osate.modelsupport

  • moved org.osate.modelsupport.internalEObjectURIWrapperPropertyTester to org.osate.modelsupport.internal.propertytester
  • moved org.osate.modelsupport.util.ResolvePrototypeUtil to org.osate.modelsupport.internal.prototypes

In project org.osate.pluginsupport

  • moved org.osate.pluginsupport.ExecuteJavaUtil to org.osate.pluginsupport.internal.utils
  • moved org.osate.pluginsupport.PluginSupportUtil to org.osate.pluginsupport.internal.utils

In project org.osate.utils

  • Renamed package org.osate.utils to org.osate.utils.internal
  • Renamed package org.osate.utils.names to org.osate.utils.internal.names

In project org.osate.analysis.flows

  • movedorg.osate.analysis.flows.FlowLatencyUtil to org.osate.analysis.flows.internal.utils

In project org.osate.analysis.resource.budgets

  • Renamed package org.osate.analysis.resource.budgets.busload.model to org.osate.analysis.resource.budgets.internal.busload.model

@AaronGreenhouse
Copy link
Contributor Author

In project org.osate.verify

  • renamed package org.osate.verify.util to org.osate.verify.internal.util

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants