Critique invited

[Tiro Verus]

Recently I was seized by the desire to "rotate" the following text:

agdtk
laeha
pmlep
hmttp
aaaaa

into this:

alpha
gamma
delta
theta
kappa

After several detours, some of which were beset by sundry glitchings,
this is what I have:

#include<stdio.h>
#include<stdlib.h>
#include<string.h>

char * app_char (char c);

int main (int argc, char *argv[]) {
/* "rotate" vertically transcribed words to horizontal ones */
/* using a append_char function */

FILE *ifp; FILE *ofp;
char *line;
char *letters[5];
int i; int j;
char * temp = malloc (2 * sizeof(char));
char c = 'i';
int nrows = 6; int ncols = 6;

char **wordlist = (char **) malloc( nrows * sizeof(char *));
for (i = 0; i < 7; i++ ) {
wordlist = (char *)malloc (ncols *sizeof(char));
}

if ( ((ifp = fopen (argv[1], "r")) == NULL) ||
((ofp = fopen (argv[2], "w")) == NULL) ) {
printf ("can\'t open file\n");
exit (1);

}

while (!feof(ifp)) {
if (fscanf(ifp, "%s", line) != 1) {
break; }
printf ("the first letter in %s is %s\n", line, app_char(line[0]));
for (j = 0; j < 5; j++) {
temp = app_char(line[j]);
strcat (wordlist[j], temp);
}

} /* end of feof/fscanf loop */


/* the following is not C it was */
/* */
/* for $j (0..4) { */
/* $newlist[$j] .= $mline[$j]; */
/* */
/* how I arrived at the solution */


/* terminate the strings */
for (i =0; i < 5; i++) {
wordlist[6] = '\0';
}

for (j = 0; j < 5; j++) {
printf (" %s \n", wordlist[j]);
}

fclose (ifp); fclose (ifp);
return 0;
}

char * app_char (char c) {
char h = c;
char * twochar = malloc (2 * sizeof (char));
twochar[0] = h;
twochar[1] = '\0';
return twochar;
}

Any ideas, criticisms, etc. invited. Regards T.

 

[Barry Schwarz]

Recently I was seized by the desire to "rotate" the following text:

agdtk
laeha
pmlep
hmttp
aaaaa

into this:

alpha
gamma
delta
theta
kappa

After several detours, some of which were beset by sundry glitchings,
this is what I have:

#include<stdio.h>
#include<stdlib.h>
#include<string.h>

char * app_char (char c);

int main (int argc, char *argv[]) {
/* "rotate" vertically transcribed words to horizontal ones */
/* using a append_char function */

FILE *ifp; FILE *ofp;
char *line;
char *letters[5];
int i; int j;
char * temp = malloc (2 * sizeof(char));
char c = 'i';
int nrows = 6; int ncols = 6;

char **wordlist = (char **) malloc( nrows * sizeof(char *));

Don't cast the return from malloc. It doesn't help and can hurt by
suppressing diagnostics you would like to see.

for (i = 0; i < 7; i++ ) {
wordlist = (char *)malloc (ncols *sizeof(char));
}

All calls to malloc should be checked for success.

if ( ((ifp = fopen (argv[1], "r")) == NULL) ||
((ofp = fopen (argv[2], "w")) == NULL) ) {
printf ("can\'t open file\n");
exit (1);

Using EXIT_FAILURE instead of 1 will make you code more portable.

}

while (!feof(ifp)) {

feof does not tell you that the file is out of data until AFTER you
attempt to read beyond the last character.

if (fscanf(ifp, "%s", line) != 1) {

line is a pointer that was never initialized to point anywhere. This
invokes undefined behavior. You are not allowed to evaluate any
uninitialized variable.

break; }
printf ("the first letter in %s is %s\n", line, app_char(line[0]));
for (j = 0; j < 5; j++) {
temp = app_char(line[j]);

temp originally held the address of the area you allocated. You have
now lost that address and created a memory leak.

strcat (wordlist[j], temp);

wordlist[j] points to an uninitialized dynamically allocated area. It
does not point to a string. Therefore, you cannot use it as the
target address for strcat. This also invokes undefined behavior.

}

} /* end of feof/fscanf loop */


/* the following is not C it was */
/* */
/* for $j (0..4) { */
/* $newlist[$j] .= $mline[$j]; */
/* */
/* how I arrived at the solution */


/* terminate the strings */
for (i =0; i < 5; i++) {
wordlist[6] = '\0';

wordlist[6] does not exist. Each wordlist points to an area
capable of holding 6 char. These are wordlist[0] through
wordlist[5]. This also invokes undefined behavior.

}

for (j = 0; j < 5; j++) {
printf (" %s \n", wordlist[j]);
}

fclose (ifp); fclose (ifp);
return 0;
}

char * app_char (char c) {
char h = c;
char * twochar = malloc (2 * sizeof (char));

You create numerous 2 char allocations but never free any of them.

twochar[0] = h;
twochar[1] = '\0';
return twochar;
}

Any ideas, criticisms, etc. invited. Regards T.

<<Remove the del for email>>

 

[Capstar]

Recently I was seized by the desire to "rotate" the following text:

agdtk
laeha
pmlep
hmttp
aaaaa

into this:

alpha
gamma
delta
theta
kappa

After several detours, some of which were beset by sundry glitchings,
this is what I have:

#include<stdio.h>
#include<stdlib.h>
#include<string.h>

char * app_char (char c);

int main (int argc, char *argv[]) {
/* "rotate" vertically transcribed words to horizontal ones */
/* using a append_char function */

FILE *ifp; FILE *ofp;
char *line;
char *letters[5];
int i; int j;
char * temp = malloc (2 * sizeof(char));
char c = 'i';
int nrows = 6; int ncols = 6;

char **wordlist = (char **) malloc( nrows * sizeof(char *));

Don't cast the return from malloc. It doesn't help and can hurt by
suppressing diagnostics you would like to see.

for (i = 0; i < 7; i++ ) {
wordlist = (char *)malloc (ncols *sizeof(char));
}

All calls to malloc should be checked for success.

if ( ((ifp = fopen (argv[1], "r")) == NULL) ||
((ofp = fopen (argv[2], "w")) == NULL) ) {
printf ("can\'t open file\n");
exit (1);

Using EXIT_FAILURE instead of 1 will make you code more portable.

}

while (!feof(ifp)) {

feof does not tell you that the file is out of data until AFTER you
attempt to read beyond the last character.

if (fscanf(ifp, "%s", line) != 1) {

line is a pointer that was never initialized to point anywhere. This
invokes undefined behavior. You are not allowed to evaluate any
uninitialized variable.

break; }
printf ("the first letter in %s is %s\n", line, app_char(line[0]));
for (j = 0; j < 5; j++) {
temp = app_char(line[j]);

temp originally held the address of the area you allocated. You have
now lost that address and created a memory leak.

strcat (wordlist[j], temp);

wordlist[j] points to an uninitialized dynamically allocated area. It
does not point to a string. Therefore, you cannot use it as the
target address for strcat. This also invokes undefined behavior.

}

} /* end of feof/fscanf loop */


/* the following is not C it was */
/* */
/* for $j (0..4) { */
/* $newlist[$j] .= $mline[$j]; */
/* */
/* how I arrived at the solution */


/* terminate the strings */
for (i =0; i < 5; i++) {
wordlist[6] = '\0';

wordlist[6] does not exist. Each wordlist points to an area
capable of holding 6 char. These are wordlist[0] through
wordlist[5]. This also invokes undefined behavior.

}

for (j = 0; j < 5; j++) {
printf (" %s \n", wordlist[j]);
}

fclose (ifp); fclose (ifp);
return 0;
}

char * app_char (char c) {
char h = c;
char * twochar = malloc (2 * sizeof (char));

You create numerous 2 char allocations but never free any of them.

twochar[0] = h;
twochar[1] = '\0';
return twochar;
}

Any ideas, criticisms, etc. invited. Regards T.

<<Remove the del for email>>

 

[CBFalconer]

Recently I was seized by the desire to "rotate" the following text:

agdtk
laeha
pmlep
hmttp
aaaaa

into this:

alpha
gamma
delta
theta
kappa
.... snip ...

Any ideas, criticisms, etc. invited. Regards T.

You need to gather the input lines into something. I suggest a
linked list of objects that look like:

struct object {
size_t lgh;
char *line;
struct object *next;
}

Having collected such a list, you now need a process for
extracting the ith char of an object, with the proviso that the
char is either '\0' or ' ' whenever i exceeds the length of the
objects line:

char ithchar(size_t i, struct object *obj);

which you can use to build the output, possibly into another list
of struct object. So doing would have the advantage of self
checking, in that two transforms should be the identity transform.

 

[Emmanuel Delahaye]

Recently I was seized by the desire to "rotate" the following text:

agdtk
laeha
pmlep
hmttp
aaaaa

into this:

alpha
gamma
delta
theta
kappa

Any ideas, criticisms, etc. invited. Regards T.

Sounds complicated to me:

#include <stdio.h>
#include <string.h>

#define N(a) (sizeof(a)/sizeof*(a))

int main (void)
{
static char const *as[] =
{
"agdtk",
"laeha",
"pmlep",
"hmttp",
"aaaaa",
};

size_t i;
size_t const width = strlen (*as);

for (i = 0; i < width; i++)
{
size_t j;
for (j = 0; j < N (as); j++)
{
putchar (as[j]);
}
putchar ('\n');
}

return 0;
}

D:\CLC\V\VERUS>bc proj.prj
alpha
gamma
delta
theta
kappa

 

[J.E./C.Y.Cripps]

You need to gather the input lines into something. I suggest a
linked list of objects that look like:

struct object {
size_t lgh;
char *line;
struct object *next;
}

Having collected such a list, you now need a process for
extracting the ith char of an object, with the proviso that the
char is either '\0' or ' ' whenever i exceeds the length of the
objects line:

char ithchar(size_t i, struct object *obj);

which you can use to build the output, possibly into another list
of struct object. So doing would have the advantage of self
checking, in that two transforms should be the identity transform.

Interesting, I was just reading the input from a file. I'm not
really sure if in my original "inspiration" all the vertical "lines"
were the same length.

 

[Tiro Verus]

for (i = 0; i < width; i++)
{
size_t j;
for (j = 0; j < N (as); j++)
{
putchar (as[j]);
}
putchar ('\n');
}

Thank you, I tried this many times and I gave up.

 

[Tiro Verus]

<big snip>

ED: Sounds complicated to me:

#define N(a) (sizeof(a)/sizeof*(a))

for (j = 0; j < N (as); j++)

And this (these 2 lines) aren't complicated????

 

[Emmanuel Delahaye]

And this (these 2 lines) aren't complicated????

There are idiomatic. Not complicated at all.