Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Single-line multi-statements

Single-line multi-statements

##Negative if

Negative if

##Single-line multi-statements

##Negative if

Single-line multi-statements

Negative if

missing space for indenting
Source Link
rolfl
  • 98.1k
  • 17
  • 219
  • 419

for ( iowner in usr_sum ) if ( usr_sum[iowner] > owner_sum ) {owner = iowner; owner_sum=usr_sum[iowner]}

for ( iowner in usr_sum ) if ( usr_sum[iowner] > owner_sum ) {owner = iowner; owner_sum=usr_sum[iowner]}

for ( iowner in usr_sum ) if ( usr_sum[iowner] > owner_sum ) {owner = iowner; owner_sum=usr_sum[iowner]}

for ( iowner in usr_sum ) if ( usr_sum[iowner] > owner_sum ) {owner = iowner; owner_sum=usr_sum[iowner]}
Source Link
rolfl
  • 98.1k
  • 17
  • 219
  • 419

I just copied your code, pasted it in to a terminal, ran it, and it did exactly what it is supposed to do. Now, how to make it better?

You don't mention it in the question, but using ls, awk, find, and sort together somehow seems unlikely to be the answer your tutor expected. You don't mention what your course is in, but I would have expected there to be a less 'cobbled together' solution, using perhaps bash only, or perl, python, or something else.

I certainly would not have solved this problem this way (I would have used perl).

Still, assuming your code meets the expectations for tool use (as in, you are supposed to use awk), there are some things that are off.

##Single-line multi-statements

You have a number of 1-liners that you should have expanded. You already have the massive awk code-block, why mix the compact 1-liner and block statements like you have done?

for ( iowner in usr_sum ) if ( usr_sum[iowner] > owner_sum ) {owner = iowner; owner_sum=usr_sum[iowner]}

The above should be:

for ( iowner in usr_sum )
{
 if ( usr_sum[iowner] > owner_sum )
 {
 owner = iowner
 owner_sum=usr_sum[iowner]
 }
}

Note, the use of block {} braces for the single if statement, and not using the semi-colon ; to separate statements on a single line.

These other statements should also be on their own lines (and the for statement should have a {} block):

for ( i=9; i<=NF; i++) name=name " " $i
...
sum=0;delete usr_sum;
...
sum+=1ドル;usr_sum[2ドル]+=1ドル;
sum_all+=1ドル;usr_sum_all[2ドル]+=1ドル;

##Negative if

Your main if-block would be better if done as a positive check, not a negative check. your code is:

if (type!="d")
{
 ... do not a directory stuff
}
else
{
 ... do directory stuff
}

but would be more readable as:

if (type == "d")
{
 ... do directory stuff
}
else
{
 ... do not a directory stuff
}

Breathing space

Your code is suffocating since it has little breathing space. Many of your statements are compressed around the operators and other symbols. Consider lines like:

 print 5,ドル3,ドルtype,name
 sum_all+=5ドル
 usr_sum_all[3ドル]+=5ドル

Which should be written as:

 print 5,ドル 3,ドル type, name
 sum_all += 5ドル
 usr_sum_all[3ドル] += 5ドル

Performance

The multiple calls to find may be hurting your performance. In general I can't help but think there's a better way to do this without all the nested OS calls.

Still, it ran fast enough for me on my machine, so it is probably not a real problem.

lang-bash

AltStyle によって変換されたページ (->オリジナル) /