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

Geo attribution task #6793

Closed
wants to merge 18 commits into from

Conversation

czolnowski
Copy link
Contributor

After applying changes that @diosmosis has described in this PR: #6463 I would like to ask about next review and confirmation that I have changed code with @diosmosis ideas.

Things that I've done after review:

  • removed LocationFetcherProvider class and methods moved to class LocationFetcher
  • since LocationFetcher aggregate some logic then I decide to keep this class separate from providers. Also I have no idea how to rename this class. If I would move getLocation method to providers, then I should copy paste cache methods and variables and copy paste provider getter in command and tracker. (which is very bad idea)
  • removed getLocationDetail method from LocationFetcher
  • removed repository pattern from DAO and move these classes into DataAccess namespace
  • refactored LogsRepository::getVisitsWithDatesLimit method to enable reusability this method in Piwik
  • removed PDO dependecie and changed PDO cursor into segmented query
  • also update methods has bind and columns to update in single array (columns => values)
  • refactored execute method in command to extract code to separate methods - I think that this method is easier to read now

What about comments? Could you give some example of this, @diosmosis?

Big thanks for precise, valuable and insightful core review!

@mattab mattab modified the milestone: Piwik 2.11.0 Dec 10, 2014
@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Dec 10, 2014
@diosmosis
Copy link
Member

Hi @czolnowski, here is my review of the new code:

  • The issue w/ LocationFetcher depends on what the scope of LocationFetcher is:

    If it is meant to be a utility class that users create and then call ->getLocation, then it's scope is the same as LocationProvider, and there are now two unrelated sets of classes/hierarchies that get a location from user info. In this case LocationFetcher should be merged into the LocationProvider hierarchy so there is only one hierarcy/base-class that is responsible for getting a location from user info. Also, the name should indicate how it is different from other LocationProviders, eg, LocationProviderWithCacheAndFallback. The name doesn't have to be catchy, it should just be descriptive.

    If it is meant to be a service that exists as a singleton or in the DI container, then it's name is not detailed enough. What does it fetch the location for? How is it different from LocationProviders? I would suggest renaming to UserGeolocator or VisitorGeolocator. In this case, it should also be put into config/global.php & accessed via StaticContainer.

code issues:

  • Is there a reason that RawLogFetcher doesn't use Db::fetchOne()/Db::fetchAll()? If those are called, then the SQL will be logged which can be useful for debugging.

  • RawLogFetcher & RawLogUpdater creates SQL statements using implode(). This seems unnecessary; I don't think it will affect performance in any way, but the extra logic makes the code a little less readable to me. Is just doing something like:

    $sql = "UPDATE ".Common::prefixTable('log_visit')
         . " SET ...";
    

    not an option?

  • In AttributeHistoricalDataWithLocations::parseRowIntoValues, $location[$locationKey] can be stored in a local variable to increase code readability.

  • In LocationFetcher, the getProviderByIdCallback callback should not be required. Instead of passing LocationProvider IDs and a callback to turn the ID into a LocationProvider to the LocationFetcher, LocationProvider instances should be passed. If null is passed, the constructor can create the default instances itself.

  • In LocationFetcher, the $defaultProviderId is specified as a parameter instead of stored as member variable. This seems odd, will there ever be a need to call a location fetcher w/ one default, and then change it to another? I think it should be stored as member and specified upon construction.

  • AttributeHistoricalDataWithLocationsTest should derive from IntegrationTestCase since it sets up a fixture from within a test method.

  • AttributeHistoricalDataWithLocationsTest::getInputStream isn't used, is it necessary?

  • The ProviderTest base class is unnecessary, only one test derives from it.

Regarding code documentation:

Classes like RawLogFetcher, RawLogUpdater and LocationFetcher which can be re-used by other developers (core, pro or plugin) should be documented so the other developers have as much information as possible. Documentation should summarize the purpose of a class or method and provide information that is not immediately available from glancing at the code (such as potential performance issues) and common use cases (in the form of examples). At least, this is how I approach in-source documentation. Writing docs is still just writing, so it's an inherently creative task. So there are many ways to do it correctly. My advice would be to revise your writing yourself and try to say as much as possible w/ as few words. Good advice for coding too I think.

If you have any questions, let me know.

@mattab
Copy link
Member

mattab commented Feb 9, 2015

Hi @czolnowski
maybe you have some question or feedback after Benaka's code review?
we leave the PR open please let us know the status, maybe it could be done for 2.11 or 2.12?

@mattab mattab modified the milestones: Piwik 2.11.0, Short term Feb 9, 2015
@czolnowski
Copy link
Contributor Author

Hello @mattab,

this PR has very low priority and I'm still getting new urgent tasks. I'll try to solve this PR, but I'm afraid that there is too much to do.

@mattab
Copy link
Member

mattab commented Mar 8, 2015

Hi guys, this is followed up in #7381

@mattab mattab closed this Mar 8, 2015
@mattab mattab added the duplicate For issues that already existed in our issue tracker and were reported previously. label Mar 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate For issues that already existed in our issue tracker and were reported previously. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants