5
\$\begingroup\$

I've created a basic password scrambler that can run a single string or a file. It will take your character, find the ascii code value of it, floor it, divide it by 16, grab whatever that equals out of the cipher array, and replace it with that character. For example:

e@ubuntu:~/bin/c$ ./ciph.exe -h
Usage: ciph -[s|f|t|h]
-s   Run a single string
-f   Run a file
-t   Run the test strings
-h   Run this help and exit
e@ubuntu:~/bin/c$ ./ciph.exe -s "orange dog"
Original: orange dog
Scrambled: bcjxnfzgbn
ekultek@ubuntu:~/bin/c$ ./ciph.exe -s "purple panda"
Original: purple panda
Scrambled: jqcjzfzjjxgj
e@ubuntu:~/bin/c$ ./ciph.exe -f test.txt
Retreived line testing
Scrambled: ffgf
Retreived line Another test
Scrambled: bxbfmfczffgf
Retreived line One more test
Scrambled: hxfzhbcfzffgf
Retreived line this is the final test
Scrambled: fm
e@ubuntu:~/bin/c$ ./ciph.exe -t
Running test strings...


Original: Adm1n1strat0r
Scrambled: bghxxxgfcjfhc
Original: TeSt
Scrambled: cfvf
Original: Ch1c4g0 Bu115!
Scrambled: jmxcjnhzpqxxvh

This is the first every program that I've written in C, and I would like some critique on my work, things I'd like to focus on (obviously critique the whole thing):

  • Are there better ways to parse command line arguments?
  • Is there a better way to strip a string of a new line?
  • Running through a file line by line, did I do it correctly?
#include <stdio.h>
#include <math.h>
#include <string.h>

#define MAX 100

void helpPage(void)
{
    /* * * * *
     * Help Page
     */

    puts("Usage: ciph -[s|f|t|h]");
    puts("-s   Run a single string");
    puts("-f   Run a file");
    puts("-t   Run the test strings");
    puts("-h   Run this help and exit");
}

char *strip(char *s)
{
    /* * * * *
     * Strip a string of a new line
     * Example:
     * strip("test\n");
     * test
     */

    return strtok(s, "\n");
}

static char *scramblePassword (char *pwd, char *result)
{
    /* * * *
     * Create a scrambled password
     * Example:
     *
     * scramblePassword("TeSt");
     *cfvf
     */

    char cipher[] = "kszhxbpjvcgfqnm";
    int i;

    for (i = 0; i < (int)strlen (pwd); i++)
    {
        int cc = (int) pwd[i]; /* get the ascii code for the character */
        result[i] = cipher[((int)floor(cc / 16) + cc % 16) % sizeof cipher];
    }
    result[i] = 0; /* NULL terminate the string */

    return result;
}

int main(int argc, char *argv[])
{
    /* * * *
     * Main loop
     */

    char *test[] = {"Adm1n1strat0r", "TeSt", "Ch1c4g0 Bu115!"};
    char result[MAX] = ""; /* Empty string */
    size_t optInt;

    for (optInt = 1; optInt < argc && argv[optInt][0] == '-'; optInt++)
    {
        switch(argv[optInt][1])
        {
            /* Help page */
            case 'h': helpPage(); return 0;

            /* Scramble a single string */
            case 's': printf("Original: %s\nScrambled: %s\n", argv[2], scramblePassword(argv[2], result)); return 0;

            /* Run the test strings */
            case 't':
            {
                int i;
                puts("Running test strings...\n\n");
                for(i = 0; i < sizeof(test)/sizeof(*test); i++)
                {
                    printf("Original: %s\nScrambled: %s\n", test[i], scramblePassword(test[i], result));
                }
                return 0;
            }

            /* Run through a file list */
            case 'f':
            {
                FILE *passFile;
                char *line = NULL;
                size_t len = 0;
                ssize_t read;

                passFile = fopen(argv[2], "r");
                if(passFile == NULL) /* If the file doesn't exist */
                {
                    puts("That file does not exist, exiting..");
                    return -1; /* exit.. */
                }
                while ((read = getline(&line, &len, passFile)) != -1)
                {
                    char *newLine = strip(line);
                    printf("Retreived line %s\nScrambled: %s\n", newLine, scramblePassword(newLine, result));
                }
                fclose(passFile);
            }
        }
    }
    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

In addition to @vnp's answer, I went over the whole code from top to bottom and noticed just what I found. Each of my comments applies to the code above the comment. It's pretty much, but don't be worried about it. It's just that in C, there are many ways to shoot yourself in the foot, and as a beginner, you are not expected to know them all.

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

Nitpick: the headers from the C standard library should be sorted alphabetically. Their order does not matter at all, therefore many programmers resort to that order.

#define MAX 100

The name of this constant is too broad. It clearly names a maximum, but maximum what?

void helpPage(void)

The void in the parentheses is good. (It's something I would not expect in many beginner programs.) So whatever resources you are learning from, keep them.

{
    /* * * * *
     * Help Page
     */

The above comment is unnecessary, since it only repeats the function name.

    puts("Usage: ciph -[s|f|t|h]");

The word usage typically starts with a lowercase u.

The usage line is incorrect, since it doesn't explain where to put the filename, or the direct string, or whether the program can do multiple files at a time. It should look like this:

usage: ciph [-ht] [-f file] [-s string]

First, the options that don't take any parameter, sorted alphabetically. Then the options with parameters, also sorted alphabetically.

This still doesn't say whether the file or the string options may be repeated, but now that I think of it, most other programs don't say that also. This information belongs into the man page of the program, not in the usage line.

    puts("-s   Run a single string");
    puts("-f   Run a file");
    puts("-t   Run the test strings");
    puts("-h   Run this help and exit");
}

char *strip(char *s)

The C language standard reserves all identifiers starting with str for its own future use, which means that you as the programmer should not define them. The str is the typical prefix for string functions (like in strlen, strcpy), and your strip is completely unrelated to that. Nevertheless, the name is reserved.

{
    /* * * * *
     * Strip a string of a new line
     * Example:
     * strip("test\n");
     * test
     */

A few nitpicks again: The semicolon is unnecessary, and I would write the example in a single line, like this: `strip("test\n") => "test".

    return strtok(s, "\n");
}

static char *scramblePassword (char *pwd, char *result)

Is there a reason that you defined this function static, but not the other ones? To be consistent, all functions except for main should be declared static, since they are only used in this source file.

{
    /* * * *
     * Create a scrambled password
     * Example:
     *
     * scramblePassword("TeSt");
     *cfvf
     */

    char cipher[] = "kszhxbpjvcgfqnm";
    int i;

The type int is not the best choice here. You should change that to size_t, which is an unsigned integer. This will prevent compiler warnings about comparison between signed and unsigned.

    for (i = 0; i < (int)strlen (pwd); i++)

Instead of computing the length of the string each time (which takes longer time for longer strings), you should just check whether you have reached the end of the string by writing pwd[i] != '\0'.

    {
        int cc = (int) pwd[i]; /* get the ascii code for the character */

The type int is wrong here and can lead to undefined behavior. When you feed some non-ASCII characters to your program (like Chinese characters, or emojis), it may crash on platforms where char can also hold negative numbers. Therefore you should change the int to unsigned char.

        result[i] = cipher[((int)floor(cc / 16) + cc % 16) % sizeof cipher];

You don't need the (int)floor part. When you divide an integer by an integer, the result is also an integer, rounded towards zero. (Or was it rounded down? I don't remember since I only use division for positive numbers.) After removing the call to floor, you don't need to include <math.h> anymore.

The sizeof cipher is wrong, since the size of a string literal includes the terminating null character. This means when cc / 16 + cc % 16 is 15, that your scrambled string will end there and will be shorter than the input string. And indeed, trying ./ciph -s hello-world confirms the bug.

    }
    result[i] = 0; /* NULL terminate the string */

    return result;
}

int main(int argc, char *argv[])
{
    /* * * *
     * Main loop
     */

    char *test[] = {"Adm1n1strat0r", "TeSt", "Ch1c4g0 Bu115!"};
    char result[MAX] = ""; /* Empty string */
    size_t optInt;

Oh, so you know about the size_t type. But here it is wrong, since argc is of type int, and you should not compare size_t with int. So change this one to int.

    for (optInt = 1; optInt < argc && argv[optInt][0] == '-'; optInt++)

Since you are already using the getline function (which comes from POSIX, not from C), you should also use the getopt function, which frees you from much of the manual work and makes your program behave like all others.

    {
        switch(argv[optInt][1])
        {
            /* Help page */
            case 'h': helpPage(); return 0;

The comment is redundant. Every statement should be in its own line. Don't try to save screen space here, it will only confuse other programmers.

            /* Scramble a single string */
            case 's': printf("Original: %s\nScrambled: %s\n", argv[2], scramblePassword(argv[2], result)); return 0;

            /* Run the test strings */
            case 't':
            {
                int i;

This should be size_t again.

                puts("Running test strings...\n\n");
                for(i = 0; i < sizeof(test)/sizeof(*test); i++)
                {
                    printf("Original: %s\nScrambled: %s\n", test[i], scramblePassword(test[i], result));
                }
                return 0;
            }

            /* Run through a file list */
            case 'f':
            {
                FILE *passFile;
                char *line = NULL;
                size_t len = 0;
                ssize_t read;

                passFile = fopen(argv[2], "r");

Accessing argv[2] is wrong. What if I run ./ciph -f file1 -f file2?

Also, when processing an option that takes a parameter, you have to do optInd++ for the parameter.

                if(passFile == NULL) /* If the file doesn't exist */
                {
                    puts("That file does not exist, exiting..");
                    return -1; /* exit.. */

It is more common to return 1 instead of -1. The -1 is used almost everywhere for reporting an error, but returning from main is one of the very few exceptions.

                }
                while ((read = getline(&line, &len, passFile)) != -1)
                {
                    char *newLine = strip(line);
                    printf("Retreived line %s\nScrambled: %s\n", newLine, scramblePassword(newLine, result));
                }

After using getline, you have to free the allocated memory, as explained in the getline man page.

                fclose(passFile);
            }
        }
    }
    return 0;
}

When I run your program as ./ciph or ./ciph string, it outputs nothing. It should output the help page instead.

When I run your program as ./ciph -s "aaaaaaaaaaaa...aaaaaa", feeding it a long string, it crashes because the result buffer is too small.

\$\endgroup\$
4
  • \$\begingroup\$ Can you give me some links to the manuals and such please? \$\endgroup\$
    – jakehimton
    Apr 3 '17 at 2:31
  • \$\begingroup\$ Also does C have an accepted coding style? Such as pythons pep8, and rubys, rubocop. \$\endgroup\$
    – jakehimton
    Apr 3 '17 at 14:26
  • 1
    \$\begingroup\$ In a Linux terminal, run man strcpy to get the manual for the strcpy function. With getopt, it's more complicated since there are several things called getopt. You need to run man 3 getopt to get the manual from section 3 of the whole manual. For more information about that, run man man. \$\endgroup\$ Apr 3 '17 at 15:40
  • \$\begingroup\$ C has several accepted coding styles. One of the most popular is K&R. \$\endgroup\$ Apr 3 '17 at 15:41
3
\$\begingroup\$

For a very first program, not bad at all. Issues:

  • "The file does not exist" error message could be misleading. There is a number of reasons why fopen() may fail (for example, insufficient permissions), so it is better to leave it to the system. Check out perror().

  • For consistency, replace "Retrieved line" with "Original".

  • Comments line /* Run the test strings */ are to be avoided. If you feel that the purpose of a particular code is unclear, factor it out to a properly named function (just like you did with strip). For example,

       case 'f':
           scramble_lines_from_file(argv[2]);
           break;
    
  • Test argv[2] before using it.

  • I don't see why scramblePassword should return result;: the result is already known to the caller.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.