#212 Patch to remove the use of MiscUtilities.MenuItemCompare.

closed-accepted
5
2008-11-26
2008-11-18
Townsfolk
No

This patch removes the use of MiscUtilities.MenuItemCompare from the core, and replaces it with a new MenuItemTextComparator. The new class uses the Comparator API and is in a new package: org.gjt.sp.util.compare

Discussion

  • Kazutoshi Satoda

    The package is miss-organized. The package org.gjt.sp.util must not
    depend on jEdit. See pakcage.html in the source tree.

    Then, I propose placing MenuItemTextComparator in org.gjt.sp.jedit.menu
    package, because it depends on org.gjt.sp.jedit.menu.EnhancedMenuItem.

    Followings are some minor issues. Please try these, too.
    - Put imports inside of the fold for imports, if there is.
    - Add a comment to @deprecated tag of MiscUtilities.MenuItemCompare,
    describing the reason of deprecation and prefered alternative.
    - Try making MiscUtilities.MenuItemCompare as a subclass of
    MenuItemTextComparator, to eliminate the code duplication.
    - Provide javadoc for MenuItemTextComparator. I recommend moving the
    first line of the first comment block of the file. The comments for
    file is likely overlooked since they are not processed by javadoc.
    - The local variable compareValue looks useless. Consider just returning
    the value.

    Note for committer (possibly me):
    Make sure to add the new file for new comparator as a copy of
    MiscUtilities.java, to keep svn blame or svn log useful.

     
  • Townsfolk

    Townsfolk - 2008-11-20

    >The package is miss-organized. The package org.gjt.sp.util must not
    >depend on jEdit. See pakcage.html in the source tree.
    >
    >Then, I propose placing MenuItemTextComparator in org.gjt.sp.jedit.menu
    >package, because it depends on org.gjt.sp.jedit.menu.EnhancedMenuItem.

    Cool. Will do.

    >Followings are some minor issues. Please try these, too.
    >- Put imports inside of the fold for imports, if there is.
    >- Add a comment to @deprecated tag of MiscUtilities.MenuItemCompare,
    > describing the reason of deprecation and prefered alternative.
    >- Try making MiscUtilities.MenuItemCompare as a subclass of
    > MenuItemTextComparator, to eliminate the code duplication.
    >- Provide javadoc for MenuItemTextComparator. I recommend moving the
    > first line of the first comment block of the file. The comments for
    > file is likely overlooked since they are not processed by javadoc.

    All great suggestions, I'll apply them and create a new patch.

    >- The local variable compareValue looks useless. Consider just returning
    > the value

    I prefer a single entry point and single exit point. The compareValue is set to a default value, modified in the various conditions and returned at the end. It's also easier to debug when it's done this way since there's one location where the value is returned.

    Thanks for the points Kazutoshi.

    Cheers,
    Eric

     
  • Matthieu Casanova

    Hi, could you also please respect the coding standards of jEdit
    like

    if (...)
    {

    instead of
    if (...) {

    thanks

     
  • Kazutoshi Satoda

    > I prefer a single entry point and single exit point. ...
    I hate this practice because it tends to introduce more variables which
    is a primary source of complexity. And in this case, it is a mixed
    change which is not in purpose of the patch.

    I hope you change your mind reading the following page.
    http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement

    But, anyway, I won't reject the patch only because of such a small
    issue. Feel free to do as you believe.

     
  • Townsfolk

    Townsfolk - 2008-11-22

    File Added: MiscUtilities.MenuItemCompare.patch

     
  • Townsfolk

    Townsfolk - 2008-11-22

    Fixed patch file - contains the formatting changes, Javadoc additions and delegation of MiscUtilities.MenuItemCompare to MenuItemTextComparator.

    No, that discussion definitely doesn't change my mind. In general I prefer the single entry, single exit, but there are always exceptions - like guard clauses. I find multiple exists leads to messier code, but that's definitely my opinion, and either way can be easily abused.

     
  • Townsfolk

    Townsfolk - 2008-11-22

    File Added: MiscUtilities.MenuItemCompare.patch

     
  • Townsfolk

    Townsfolk - 2008-11-22

    sorry. the last patch some how missed the MenuItemTextComparator.

     
  • Kazutoshi Satoda

    • assigned_to: nobody --> k_satoda
     
  • Kazutoshi Satoda

    I don't think a separated package is worth adding for such a small
    class. Please put it in the existing menu package, or explain the reason
    to separate it.

    Small issues:
    - Some new imports are still outside of the fold for imports.
    (in RecentDirectoriesProvider.java and VFSBrowser.java)
    - Import of EnhancedMenuItem in MiscUtilities.java is no longer used.
    - An unnecessary space is added after @Deprecated for MenuItemCompare.

     
  • Townsfolk

    Townsfolk - 2008-11-22

    File Added: MiscUtilities.MenuItemCompare.patch

     
  • Townsfolk

    Townsfolk - 2008-11-22

    >I don't think a separated package is worth adding for such a small
    >class. Please put it in the existing menu package, or explain the reason
    >to separate it.

    Done.

    >Small issues:
    >- Some new imports are still outside of the fold for imports.
    > (in RecentDirectoriesProvider.java and VFSBrowser.java)

    Thanks, done.

    >- Import of EnhancedMenuItem in MiscUtilities.java is no longer used.

    Fixed.

    >- An unnecessary space is added after @Deprecated for MenuItemCompare.

    I appreciate the high standards, which is truly a mark of a great programmer.

    While I understand that the extra space forces some extra time to check the changes in the patch, this is an open-sourced project and its livelihood depends on the community working on patches, fixes, and features outside of their work life. If we really want to eliminate this type of minor oversight, maybe we should adopt some sort of source code formatter which anyone can use.

     
  • Kazutoshi Satoda

    Thanks, the patch looks good enough.
    Sorry, but, a remaining small issue I'm not sure:
    - Since the comparator is in menu package, some imports looks no longer
    needed. I think removing (not adding) them is better, isn't it?

    For the formatting issue, WhiteSpace plugin provides the option to
    remove trailing spaces at saving. But I can't use this because this
    could be more unnecessary changes depending how the file was previously.

    Checking the actual diff (or running some diff tools) just before commit
    or submitting patch is the best practice for me. I don't think this as
    an extra work because I must do that for actual changes anyway, not only
    for formatting. Saving coutless others' time is more important than
    saving merely my own, at least for a coward, me.

    If you think defining a standard and adopting a formatter is better,
    asking it on jedit-devel might be good.

     
  • Townsfolk

    Townsfolk - 2008-11-22

    File Added: MiscUtilities.MenuItemCompare.patch

     
  • Townsfolk

    Townsfolk - 2008-11-22

    Odd. Eclipse didn't pick up the import statement.

    I'll look into creating a Jalopy file and posting it to the devel group for review. Thanks. :)

     
  • Townsfolk

    Townsfolk - 2008-11-22

    File Added: MiscUtilities.MenuItemCompare.patch

     
  • Kazutoshi Satoda

    Only an import in MenuItemTextComparator.java was removed, but there are
    still questionable imports in *Provider.java.

     
  • Townsfolk

    Townsfolk - 2008-11-26

    File Added: MiscUtilities.MenuItemCompare.patch

     
  • Townsfolk

    Townsfolk - 2008-11-26

    K. Hopefully that fixes it.

    Cheers.

     
  • Kazutoshi Satoda

    Thanks, committed in r14107.

    Bah, sorry, I missed my own note to make the new file as a copy in svn.
    So embarrassed...

     
  • Kazutoshi Satoda

    • status: open --> closed-accepted
     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks