Re: [patch #8527] Adding GRAPH command with simple scatterplot and histogram (incl tests and doc)

[John Darrington]

I finally got around to reviewing this patch.

In a word: Ausgezeichnet!

0. There is a potential internationalisation issue in the function 
show_scatterplot:

 ds_init_cstr (&title, var_to_string (cmd->dep_vars[0]));
 ds_put_cstr (&title, " vs ");              
 ds_put_cstr (&title, var_to_string (cmd->dep_vars[1]));
 if (cmd->byvar)
   {
     ds_put_cstr (&title, " by ");                
     ds_put_cstr (&title, var_to_string (cmd->byvar));
   }    

This is not been marked for translation (and cannot be anyway, because it has 
been constructed on-the-fly)

You should instead use something similiar to 

if (cmd->byvar)
 ds_put_format (&title, _("%s vs. %s by %s"),  var_to_string (xxx), 
var_to_string (yyy), var_to_string (zzz));
else
 ds_put_format (&title, _("%s vs. %s"),  var_to_string (xxx), var_to_string 
(yyy));


That way, the translators get complete strings to translate instead of single 
words.  
(Without context, how would you translate the word "by" into German? bei, 
durch, indem, neben ....?)

Also, note that in other places we have used "vs." (with a period) as the 
abbreviation for the latin "versus".





Other than that, I can find only minor cosmetic issues:


1. Copyright date:  
   +++ b/src/language/stats/graph.c
   +  Copyright (C) 2012, 2013  Free Software Foundation, Inc.

   It's now 2014 right! - if this file was derived from another existing file, 
then
   it is correct that the existing dates should be kept.  But the current year 
should be appended.


2. 
+  //  xrchart_line (cr, geom, npp->slope, npp->intercept,
+  //            npp->y_first, npp->y_last, XRCHART_DIM_Y);

   We don't use // style comments (because older compilers don't support them), 
and it is best not to comment
   out code anyway - just remove it if its not needed.


3. In a few places: there is missing whitespace:  Eg:
         scatterplot = scatterplot_create(input,
                                   cmd->dep_vars[0], 
                                   cmd->dep_vars[1],
                                   cmd->byvar,
                                   &byvar_overflow,
                                   ds_cstr (&title),
                                   cmd->es[0].minimum, cmd->es[0].maximum,
                                   cmd->es[1].minimum, cmd->es[1].maximum);
  scatterplot_chart_submit(scatterplot);
  ds_destroy(&title);


  In PSPP we try to follow the GNU Coding Standards, which recommend a space 
before each (
  See http://www.gnu.org/prep/standards/standards.html#Formatting


4.  Although it hurts my imperialistic pride, 

          msg (MW, _("Maximum number of scatterplot categories reached." 
                 "Your BY variable has too many distinct values."
                 "The colouring of the plot will not be correct"));

    we decided some years ago to use US instead of commonwealth English.  That 
is,
    "coloring" instead of "colouring".`



Thanks for the patch.  A number of people have been asking for this, for some 
time.

J'





-- 
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

signature.asc
Description: Digital signature