Mahout
  1. Mahout
  2. MAHOUT-896

Improve readability of AbstractDifferenceRecommenderEvaluator class

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 0.6
    • Fix Version/s: 0.6
    • Labels:

      Description

      Change the variable and private method names so that they are internally consistent, and their purpose is more clear.

      1. MAHOUT-896.patch
        5 kB
        Anatoliy Kats

        Activity

        Hide
        Sean Owen added a comment -

        OK, do you have any specific suggestions?

        The fields and methods are...

        dataModel
        candidateItemsStrategy

        getDefaultCandidateItemsStrategy
        recommend
        setPreference
        removePreference
        getDataModel
        getAllOtherItems

        This seems pretty clear. This is hardly a complex class either. It also forms part of the public API.
        I have a hard time imagining a naming change I would support.

        Show
        Sean Owen added a comment - OK, do you have any specific suggestions? The fields and methods are... dataModel candidateItemsStrategy getDefaultCandidateItemsStrategy recommend setPreference removePreference getDataModel getAllOtherItems This seems pretty clear. This is hardly a complex class either. It also forms part of the public API. I have a hard time imagining a naming change I would support.
        Hide
        Anatoliy Kats added a comment -

        Sorry, I meant AbstractDifferenceRecommenderEvaluator. They are simple changes, I'll submit my specific proposals as a patch in half an hour or so.

        Show
        Anatoliy Kats added a comment - Sorry, I meant AbstractDifferenceRecommenderEvaluator. They are simple changes, I'll submit my specific proposals as a patch in half an hour or so.
        Hide
        Anatoliy Kats added a comment -

        Patch submitted. The method name processUser is uninformative, since the word 'process' can refer to anything. The original names for the parameters are also confusing: it is not clear that trainingUsers and testingUserPrefs refer to the same kinds of items. The word 'Pref' is, ahem, preferable, since the Preference class includes the User ID, Item ID, and Preference Value. So, I renamed all the variables to use that term only.

        What do you think?

        Show
        Anatoliy Kats added a comment - Patch submitted. The method name processUser is uninformative, since the word 'process' can refer to anything. The original names for the parameters are also confusing: it is not clear that trainingUsers and testingUserPrefs refer to the same kinds of items. The word 'Pref' is, ahem, preferable, since the Preference class includes the User ID, Item ID, and Preference Value. So, I renamed all the variables to use that term only. What do you think?
        Hide
        Sean Owen added a comment -

        OK. I think this is fairly trivial, renaming things like "testUserPrefs" to "testPrefs". I don't mind it either. The patch uses tab formatting, but I can fix that on this side.

        Show
        Sean Owen added a comment - OK. I think this is fairly trivial, renaming things like "testUserPrefs" to "testPrefs". I don't mind it either. The patch uses tab formatting, but I can fix that on this side.
        Hide
        Anatoliy Kats added a comment -

        It is. I guess I became a stickler for readability after a Google internship Code is read a lot more than it's written, right?

        Show
        Anatoliy Kats added a comment - It is. I guess I became a stickler for readability after a Google internship Code is read a lot more than it's written, right?
        Hide
        Sean Owen added a comment -

        I am ex-Google too, and agree. This change does not meaningfully alter readability, to me, or else I would have named it differently in the first place. But I don't mind changing it if you think it's more readable this way.

        Show
        Sean Owen added a comment - I am ex-Google too, and agree. This change does not meaningfully alter readability, to me, or else I would have named it differently in the first place. But I don't mind changing it if you think it's more readable this way.
        Hide
        Hudson added a comment -

        Integrated in Mahout-Quality #1198 (See https://builds.apache.org/job/Mahout-Quality/1198/)
        MAHOUT-896 rename some internal vars and methods

        srowen : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1206251
        Files :

        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/eval/AbstractDifferenceRecommenderEvaluator.java
        Show
        Hudson added a comment - Integrated in Mahout-Quality #1198 (See https://builds.apache.org/job/Mahout-Quality/1198/ ) MAHOUT-896 rename some internal vars and methods srowen : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1206251 Files : /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/eval/AbstractDifferenceRecommenderEvaluator.java

          People

          • Assignee:
            Sean Owen
            Reporter:
            Anatoliy Kats
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 0.5h
              0.5h
              Remaining:
              Remaining Estimate - 0.5h
              0.5h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development