search

caarmen / network-monitor

A network monitor tool for Android that executes an http GET to google.com every x seconds and logs the results into a file.
74 stars 27 forks source link

Added a feature that enables speedtests to be configured not to run on every tick #58

Closed motorolja closed 9 years ago

motorolja commented 9 years ago

While using the application I noticed if I wanted to have decent accuracy while running speed test my data usage got huge. Just setting the interval to very slow is not an option since I also want to keep track of the signal strength.

The download and upload speed does not change as much as the signal strength. So to enable high accuracy on the speed test I just made it so that the test is not run on every tick, there are different levels of how often one want to run the speed test.

The implementation for the network change and dbm change options are probably not the most accurate since I only check these when the application requests a speed test and not when the changes occurs.

caarmen commented 9 years ago

Many thanks for your submission. I currently am not at my pc: I've only looked at the pull request on my phone. I hope to be able to look at it more in detail soon.

I have a couple of comments though:

In the upload and download data sources, you added some static fields. Why are they static?

In the same files, in the switch case over the different intervals, there's a lot of copy /paste. I think this can be reduced simply by having one single implementation for all those cases which are copied /pasted.

In one file, I believe I saw an alert dialog added with the title and message hard-coded in Java. These strings should go into a string xml file. But I'm not even sure there should be a warning dialog there...

Thanks again! I hope to be able to look at this further, soon. On May 16, 2015 12:38 AM, "Rasmus Holm" notifications@github.com wrote:

While using the application I noticed if I wanted to have decent accuracy while running speed test my data usage got huge. Just setting the interval to very slow is not an option since I also want to keep track of the signal strength.

The download and upload speed does not change as much as the signal strength. So to enable high accuracy on the speed test I just made it so that the test is not run on every tick, there are different levels of how often one want to run the speed test.

The implementation for the network change and dbm change options are probably not the most accurate since I only check these when the

application requests a speed test and not when the changes occurs.

You can view, comment on, or merge this pull request online at:

https://github.com/caarmen/network-monitor/pull/58 Commit Summary

  • added GUI for toggling advanced speed test settings
  • added hooks to the GUI in speed test preferences
  • Added alternating intervals for download test
  • Added comments for the unimplemented advanced speed intervals
  • added function for checking dbm changes within a certain range
  • added function for checking network changes and added functions for network and dbm changes to upload test
  • Changed description since the advanced speedtest intervals are implemented

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/caarmen/network-monitor/pull/58.

motorolja commented 9 years ago

The static values in upload and download data are just there to reduce the cost of recreating them, though I assume there are currently only 1 instance of each data source always now which means that it does not matter whether or not they are static. I usually always want to do static on member classes when possible since they are generally more expensive to recreate.

Yes a lot of copy paste, should be put in a separate class. I am working on factoring them out.

The alert message is removed, wasent saying anything useful.

caarmen commented 9 years ago

Thanks for the refactoring!

Can you define some constants for -1 and -2 instead of using int literals?

In the switch case, you can use these constants for the first two cases, and then merge all the remaining cases into a single default case, since they all have exactly the same logic.

Thanks :-)

caarmen commented 9 years ago

Also it looks like mDifference could be a static final constant instead of a member field. And maybe with a slightly more descriptive name? Like SIGNAL_STRENGTH_VARIATION_THRESHOLD_DBM. Yeah, it's a bit verbose, but you know exactly what it is :-)

caarmen commented 9 years ago

Thanks for your last changes.

For info, I've created a branch in my repo, motorolja_speed_test_interval.

I've merged your master branch into that branch, I've done some changes, and I'm just working on testing right now.

Summary of changes:

You can see the changes here: https://github.com/caarmen/network-monitor/compare/dc9c699c45cfe1d545d51f433fc986bcca0fa953...motorolja_speed_test_interval

Once I've done enough testing, I'll merge this into the master :)

Thanks again!

caarmen commented 9 years ago

Merged into master.