HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Implementation of the ls command with several options

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thewithoptionsseveralimplementationcommand

Problem

As part of a past assignment, I've expanded on this implementation of ls to have more options available, including non-standard ones.

It currently supports these (described on this page):

  • -a



  • -d



  • -h



  • -i



  • -l



  • -p



  • -Q



  • -R



  • -S



  • -t



  • -U



Other than that, it doesn't list a total field nor does it have coloring. It also doesn't have the same adaptive column alignments, but I have it done with fixed amounts so that it's still easier to read each column with -l.

I've already ran this through Valgrind with and without different options, and there appears to be no memory leaks. I think it can still be structured better and simplified, though.

```
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include

static char* global_dir = ".";

struct Options
{
bool using_a;
bool using_d;
bool using_h;
bool using_i;
bool using_l;
bool using_p;
bool using_Q;
bool using_R;
bool using_S;
bool using_t;
bool using_U;
};

static void init_opts(struct Options* opts)
{
opts->using_a = false;
opts->using_d = false;
opts->using_h = false;
opts->using_i = false;
opts->using_l = false;
opts->using_p = false;
opts->using_Q = false;
opts->using_R = false;
opts->using_S = false;
opts->using_t = false;
opts->using_U = false;
}

struct Options get_opts(int count, char* args[])
{
struct Options opts;
init_opts(&opts);
int opt;

while ((opt = getopt(count, args, "adhilpQRStU")) != -1)
{
switch (opt)
{
case 'a': opts.using_a = true; break;
case 'd': opts.using_d = true; break;
case 'h': opts.using_h = true; break;
case 'i': opts.using_i = true; break;
case 'l': opts.using_l = true; break;
case 'p': opts.using_p = true; break;
case 'Q': opts.using_Q = true; break;

Solution

Looks pretty clean for the most part! Here's the source of the actual GNU implementation used on most systems for comparison (if you already haven't taken a look at it). Just a few notes:

-
Looks like part of this function could be converted into a for loop:

void readable_fs(double size, char* buf)
{
    const char* units[] = { "", "K", "M", "G", "T" };
    int i = 0;

    while (size > 1024)
    {
        size /= 1024;
        ++i;
    }

    sprintf(buf, "%.*f%s", i, size, units[i]);
}


Would still retain the same functionality, but reduce LOC and make it more readable/concise.

-
I also noticed you have a magic number in the above function (as well as some other places). I think it best in this situation to just place a comment above its usage to specify what that number is.

-
I'm not a big fan of the using_t name, since the _t is reserved for POSIX type implementations. t_option would be better? But if you change that one name you may as well be consistent and change all the others to be similar.

-
Prefer snprintf() to sprintf().

-
You should use size_t in your for loop counter variable here:

for (long int i = 0; i < count; ++i)


You could use this in other places too, as I see you are starting from 0 and only counting up (meaning you'd have no negative numbers, right?). I'm not saying size_t is a good fit for every situation though, use the smallest type that will fit the situation as appropriately as possible to. stdint.h is your best friend here.

-
No reason to return 0;, can/should be removed.

Code Snippets

void readable_fs(double size, char* buf)
{
    const char* units[] = { "", "K", "M", "G", "T" };
    int i = 0;

    while (size > 1024)
    {
        size /= 1024;
        ++i;
    }

    sprintf(buf, "%.*f%s", i, size, units[i]);
}
for (long int i = 0; i < count; ++i)

Context

StackExchange Code Review Q#114479, answer score: 9

Revisions (0)

No revisions yet.