[PATCH 2/2] Fixing up the code as Dirk suggested

Dirk Hohndel dirk at hohndel.org
Mon Sep 10 21:31:54 PDT 2012


Miika,

I noticed that you were still missing a few of my recommendations
(you keep passing '0' as a parameter that's a gboolean). I fixed all
these up and streamlined the code a little, and then I noticed that the
statistics displayed were actually garbled - it displayed values like
"57ft" for SAC rates... definitely not reasonable :-)

I didn't want to send you back to redo one more time, so I fixed that
part of the code as well and will be pushing all of this out to Linus
tonight. 

It seems pretty obvious that you didn't test the second patch very well
(what was shown was quite obviously bogus). Please take a little more
care before submitting patches in the future.

Once Linus pushes this out, please check that none of my fixes broke the
actual intended functionality of your code. :-)

One thing I noticed is that the current code doesn't refresh the display
when the chosen units are changed in the preferences. That's clearly not
a show stopper (the user could simply close the statistics window and
reopen it) but it may be something to look into for the future.

/D

Miika Turkia <miika.turkia at gmail.com> writes:

> Using already existing macro for splitting seconds into minutes:seconds.
> Moving repetitive code to a function (couldn't think of the suggested
> clever macro, but this should pretty much do the trick).
> Now the statistics are updated every time the process_all_dives function
> is called. It might make sense to actually verify the structures need to
> be re-allocated, but such optimization is currently not implemented.
>
> Signed-off-by: Miika Turkia <miika.turkia at gmail.com>
> ---
>  dive.h       |    2 +
>  save-xml.c   |    2 -
>  statistics.c |  132 +++++++++++++++++++++++++++++-----------------------------
>  3 files changed, 67 insertions(+), 69 deletions(-)
>
> diff --git a/dive.h b/dive.h
> index 6826132..e45d962 100644
> --- a/dive.h
> +++ b/dive.h
> @@ -464,4 +464,6 @@ extern const char *star_strings[];
>  
>  #define AIR_PERMILLE 209
>  
> +#define FRACTION(n,x) ((unsigned)(n)/(x)),((unsigned)(n)%(x))
> +
>  #endif /* DIVE_H */
> diff --git a/save-xml.c b/save-xml.c
> index 3bea2ad..7ce0afb 100644
> --- a/save-xml.c
> +++ b/save-xml.c
> @@ -7,8 +7,6 @@
>  
>  #include "dive.h"
>  
> -#define FRACTION(n,x) ((unsigned)(n)/(x)),((unsigned)(n)%(x))
> -
>  static void show_milli(FILE *f, const char *pre, int value, const char *unit, const char *post)
>  {
>  	int i;
> diff --git a/statistics.c b/statistics.c
> index 86128ae..c5af8ab 100644
> --- a/statistics.c
> +++ b/statistics.c
> @@ -13,7 +13,6 @@
>  #include <stdlib.h>
>  #include <stdarg.h>
>  #include <time.h>
> -#include <stdbool.h>
>  #include <gdk/gdkkeysyms.h>
>  
>  #include "dive.h"
> @@ -80,7 +79,6 @@ static stats_t stats_selection;
>  static stats_t *stats_monthly = NULL;
>  static stats_t *stats_yearly = NULL;
>  
> -bool init_yearly = 1;
>  GtkTreeIter yearly_iter;
>  
>  enum {
> @@ -213,19 +211,28 @@ static void add_cell_to_tree(GtkWidget *tree, char *value, int index, gboolean r
>  static char * get_minutes(int seconds)
>  {
>  	static char buf[80];
> -	int minutes = seconds / 60;
> -	int sec = seconds % 60;
> -	snprintf(buf, sizeof(buf), "%d:%.2d", minutes, sec);
> +	snprintf(buf, sizeof(buf), "%d:%.2d", FRACTION(seconds, 60));
>  	return buf;
>  }
>  
> -void process_interval_stats(GtkWidget *tree, stats_t stats_interval, GtkTreeIter *parent)
> +void add_cell(GtkWidget *tree, GtkTreeIter *parent, unsigned int val, int cell)
>  {
>  	double value;
>  	int decimals;
>  	const char *unit;
>  	char value_str[40];
>  
> +	value = get_depth_units(val, &decimals, &unit);
> +	snprintf(value_str, sizeof(value_str), "%.*f %s", decimals, value, unit);
> +	add_cell_to_tree(tree, value_str, cell, 0, parent);
> +}
> +
> +void process_interval_stats(GtkWidget *tree, stats_t stats_interval, GtkTreeIter *parent)
> +{
> +	double value;
> +	const char *unit;
> +	char value_str[40];
> +
>  	/* Year or month */
>  	snprintf(value_str, sizeof(value_str), "%d", stats_interval.period);
>  	add_cell_to_tree(tree, value_str, 0, 1, parent);
> @@ -241,29 +248,17 @@ void process_interval_stats(GtkWidget *tree, stats_t stats_interval, GtkTreeIter
>  	/* Longest duration */
>  	add_cell_to_tree(tree, get_minutes(stats_interval.longest_time.seconds), 5, 0, parent);
>  	/* Average depth */
> -	value = get_depth_units(stats_interval.avg_depth.mm, &decimals, &unit);
> -	snprintf(value_str, sizeof(value_str), "%.*f %s", decimals, value, unit);
> -	add_cell_to_tree(tree, value_str, 6, 0, parent);
> +	add_cell(tree, parent, stats_interval.avg_depth.mm, 6);
>  	/* Smallest maximum depth */
> -	value = get_depth_units(stats_interval.min_depth.mm, &decimals, &unit);
> -	snprintf(value_str, sizeof(value_str), "%.*f %s", decimals, value, unit);
> -	add_cell_to_tree(tree, value_str, 7, 0, parent);
> +	add_cell(tree, parent, stats_interval.min_depth.mm, 7);
>  	/* Deepest maximum depth */
> -	value = get_depth_units(stats_interval.max_depth.mm, &decimals, &unit);
> -	snprintf(value_str, sizeof(value_str), "%.*f %s", decimals, value, unit);
> -	add_cell_to_tree(tree, value_str, 8, 0, parent);
> +	add_cell(tree, parent, stats_interval.max_depth.mm, 8);
>  	/* Average air consumption */
> -	value = get_volume_units(stats_interval.avg_sac.mliter, &decimals, &unit);
> -	snprintf(value_str, sizeof(value_str), "%.*f %s/min\t", decimals, value, unit);
> -	add_cell_to_tree(tree, value_str, 9, 0, parent);
> +	add_cell(tree, parent, stats_interval.avg_sac.mliter, 9);
>  	/* Smallest average air consumption */
> -	value = get_volume_units(stats_interval.min_sac.mliter, &decimals, &unit);
> -	snprintf(value_str, sizeof(value_str), "%.*f %s/min\t", decimals, value, unit);
> -	add_cell_to_tree(tree, value_str, 10, 0, parent);
> +	add_cell(tree, parent, stats_interval.min_sac.mliter, 10);
>  	/* Biggest air consumption */
> -	value = get_volume_units(stats_interval.max_sac.mliter, &decimals, &unit);
> -	snprintf(value_str, sizeof(value_str), "%.*f %s/min\t", decimals, value, unit);
> -	add_cell_to_tree(tree, value_str, 11, 0, parent);
> +	add_cell(tree, parent, stats_interval.max_sac.mliter, 11);
>  	/* Average water temperature */
>  	value = get_temp_units(stats_interval.min_temp, &unit);
>  	if (stats_interval.combined_temp && stats_interval.combined_count) {
> @@ -283,7 +278,8 @@ void process_interval_stats(GtkWidget *tree, stats_t stats_interval, GtkTreeIter
>  
>  static void key_press_event(GtkWidget *window, GdkEventKey *event, gpointer data)
>  {
> -	if (event->string != NULL && event->keyval == GDK_Escape)
> +	if ((event->string != NULL && event->keyval == GDK_Escape) ||
> +			(event->string != NULL && event->keyval == GDK_w && event->state & GDK_CONTROL_MASK))
>  		gtk_widget_destroy(window);
>  }
>  
> @@ -347,6 +343,23 @@ static void process_all_dives(struct dive *dive, struct dive **prev_dive)
>  		stats.min_depth.mm = dive_table.dives[0]->maxdepth.mm;
>  		stats.selection_size = dive_table.nr;
>  	}
> +
> +	/* allocate sufficient space to hold the worst
> +	 * case (one dive per year or all dives during
> +	 * one month) for yearly and monthly statistics*/
> +
> +	if (stats_yearly != NULL) {
> +		free(stats_yearly);
> +		free(stats_monthly);
> +	}
> +	size = sizeof(stats_t) * (dive_table.nr + 1);
> +	stats_yearly = malloc(size);
> +	stats_monthly = malloc(size);
> +	if (!stats_yearly || !stats_monthly)
> +		return;
> +	memset(stats_yearly, 0, size);
> +	memset(stats_monthly, 0, size);
> +
>  	/* this relies on the fact that the dives in the dive_table
>  	 * are in chronological order */
>  	for (idx = 0; idx < dive_table.nr; idx++) {
> @@ -357,52 +370,37 @@ static void process_all_dives(struct dive *dive, struct dive **prev_dive)
>  				*prev_dive = dive_table.dives[idx-1];
>  		}
>  		process_dive(dp, &stats);
> -		if (init_yearly) {
> -			/* allocate sufficient space to hold the worst
> -			 * case (one dive per year or all dives during
> -			 * one month) */
> -			if (stats_yearly == NULL) {
> -				size = sizeof(stats_t) * (dive_table.nr + 1);
> -				stats_yearly = malloc(size);
> -				stats_monthly = malloc(size);
> -				if (!stats_yearly || !stats_monthly)
> -					return;
> -				memset(stats_yearly, 0, size);
> -				memset(stats_monthly, 0, size);
> -			}
>  
> -			/* yearly statistics */
> -			tm = gmtime(&dp->when);
> -			if (current_year == 0)
> -				current_year = tm->tm_year + 1900;
> -
> -			if (current_year != tm->tm_year + 1900) {
> -				current_year = tm->tm_year + 1900;
> -				process_dive(dp, &(stats_yearly[++year_iter]));
> -			} else
> -				process_dive(dp, &(stats_yearly[year_iter]));
> -
> -			stats_yearly[year_iter].selection_size++;
> -			stats_yearly[year_iter].period = current_year;
> -
> -			/* monthly statistics */
> -			if (current_month == 0) {
> +		/* yearly statistics */
> +		tm = gmtime(&dp->when);
> +		if (current_year == 0)
> +			current_year = tm->tm_year + 1900;
> +
> +		if (current_year != tm->tm_year + 1900) {
> +			current_year = tm->tm_year + 1900;
> +			process_dive(dp, &(stats_yearly[++year_iter]));
> +		} else
> +			process_dive(dp, &(stats_yearly[year_iter]));
> +
> +		stats_yearly[year_iter].selection_size++;
> +		stats_yearly[year_iter].period = current_year;
> +
> +		/* monthly statistics */
> +		if (current_month == 0) {
> +			current_month = tm->tm_mon + 1;
> +		} else {
> +			if (current_month != tm->tm_mon + 1)
>  				current_month = tm->tm_mon + 1;
> -			} else {
> -				if (current_month != tm->tm_mon + 1)
> -					current_month = tm->tm_mon + 1;
> -				if (prev_month != current_month || prev_year != current_year)
> -					month_iter++;
> -			}
> -
> -			process_dive(dp, &(stats_monthly[month_iter]));
> -			stats_monthly[month_iter].selection_size++;
> -			stats_monthly[month_iter].period = current_month;
> -			prev_month = current_month;
> -			prev_year = current_year;
> +			if (prev_month != current_month || prev_year != current_year)
> +				month_iter++;
>  		}
> +
> +		process_dive(dp, &(stats_monthly[month_iter]));
> +		stats_monthly[month_iter].selection_size++;
> +		stats_monthly[month_iter].period = current_month;
> +		prev_month = current_month;
> +		prev_year = current_year;
>  	}
> -	init_yearly = 0;
>  }
>  
>  /* make sure we skip the selected summary entries */
> -- 
> 1.7.9.5
>

-- 
Dirk Hohndel
Intel Open Source Technology Center


More information about the subsurface mailing list