20
A few things jump out at me immediately:
Use sizeof() whenever you need the size of something with non-dynamic allocation. In other words, all of your write calls on the WaveHeader struct members should be using sizeof, not hard coded sizes.
Allocation and initialization should be separate concerns. There's no need to have the same method that initializes a ...
18
First of all: nice work! It's easy to read and understand.
Program organization
It's very good that you split the task to small functions.
Reading the body of main reveals nicely the overall flow.
Ideas for further improvement:
place does two things: it reads input from user and updates the state of the board. It would be good to separate these logically ...
17
Here are a few comments. In general, your code deviates in surprising ways from the UNIX commands (and not just by missing flags or options):
Your ls command does more than you claim it does. os.walk recursively "walks" down from the current directory, so it returns the content of all subfolders as well (without indicating in which subfolder each file or ...
16
Here are some things that may help you improve your code.
Omit C prolog and epilog if possible
The part you have correctly labeled "C prologue" in your code is used by the compiler to be able to access local variables. However both inputs and outputs to this assembly routine are in registers, so no stack manipulations are needed. For that reason, we can ...
16
Design
This is a very thin C like wrapper around sockets. In my opinion there are a lot of style changes that are need to make this good C++ or usable by modern C++ library.
Main issues:
Two phase construction
Error Codes escape public interface (use exception)
You member variables have state not related to the object after construction.
Your use of ...
15
Minimize math operations
if (((double)after.tv_sec*1000000 + (double)after.tv_usec)-((double)before.tv_sec*1000000 + (double)before.tv_usec) > timer){ //time difference in microsec accuracy
You do four conversions from integer types to a double precision floating point type. And do two multiplications times a million.
Consider
//...
15
Names beginning with _ are reserved for the implementation; I'm surprised that -Wpedantic doesn't help with this!
Include what you use - it helps if your implementation and test program include malloc.h before any standard headers. In this case, it's depending on <stddef.h> (or one of the other headers which define size_t).
With -Wconversion, I get ...
14
The comments in the clearmem procedure for the Linux block look a bit confusing. You could just have a summary of the procedure commented above, and have the individual comments for each line specify the meaning of the assembly instructions.
Specifically, the lines that describe the C++ code don't quite reflect on the assembly code. clearmem itself doesn'...
14
Prefer the C++ way
As you seem to expect, one of the key issues with your code is that it's not very C++y.
1. Prefer std::string over C-style strings.
std::string is better in almost every way. It takes care of its own memory and grows when needed. The speed is usually comparable to C-style strings (sometimes faster, sometimes slower). The code written ...
13
I can't say I particularly like this code as it is right now. It seems to me that there are two reasonable approaches: if you think most of what you zero will be in main memory, then you probably just want the most compact code possible for the job. If you think it'll be used to zero data that might be in the cache a noticeable amount of the time, then you ...
13
In C, I'd rather accept a pointer to a struct to be initialized than return newly allocated heap memory. It's clear that the caller owns the memory. The caller has the flexibility to pass you a pointer to a struct on its stack.
void initWAVHeader(WaveHeader *hdr, uint32_t sample_rate, uint16_t bit_depth, uint16_t channels);
The same goes for the rest of ...
13
Concept
I don't believe that there is a cross-platform way to read the ARP cache. From the use of /proc filesystem, I deduce that you are targeting Linux. In Linux, you could read /proc/net/arp as you have done, or run the command ip neigh, which does something similar to your program. (On OS X, you could run arp -a -n instead.)
A comment worth a ...
12
1. Comments on your code
I like the way you've used docopt. But why do you require the user to run rename-flac.py -o in order to read the documentation for the schemes? Surely it would be clearer and simplier if the documentation for the schemes were included in the usage documentation?
Your program would be easier to test if you put the top-level code into ...
12
Use binary mode
Currently, you are opening your files like this:
in = fopen(optarg, "r");
out = fopen(optarg, "w");
On a UNIX system that would be fine, but on a Windows system, that could result in CRLF pairs being converted into a single LF when reading, and single LF characters converted to CRLF pairs when writing. This is particularly ...
12
This script might get the job done, but it is a rather crude and inefficient hack. Ideally, you should avoid polling every quarter second (or polling at all). Also, I see no reason to write any files to /tmp.
The ideal way to do it in Linux is to use udev. If you don't want to write a persistent udev rule, or you don't have root access, you can run /sbin/...
11
Well, this is actually pretty decent code. There still is a lot of stuff that could be improved, I tried to focus on some more relevant ones.
Subroutine Prototypes
In a declaration like sub foo ($@&*) { ... }, we call the weird thing in parens a prototype. The prototype primarily changes how a call to that sub is parsed, and can set properties like ...
11
Your code is so full of irrelevant content that distracts from the purpose of the code.
The most Beautiful shell scripts are the ones that do a job, and do it well. They do it in a standard way with the least surprise. They accept input from pipes, redirects, and files, and they output in a way that is redirectable, loggable, and readable.
The following is ...
11
A few notes:
Overall, you are doing a lot of unnecessary work. You could just use
getifaddrs() to get the IP address, MAC address, and interface
name. There are plenty of code examples online on how you can use it to obtain all of that information, including an example in the function documentation itself.
You could also read directly from the file
/sys/...
11
Style
Minor nitpicking: your style is not 100% consistent.
int main(int argc, char** args){
// should be
int main(int argc, char **args){
while(1) {
// should be
while(1){
Pattern
I think this block is very, very unusual.
switch(parents){
case false: remove_dir(path, verbose); break;
case true: do_parents(path, verbose); break;
}
A switch ...
11
The points jacwah made in his answer are basically valid.
You don't seem to be using any feature from <sys/wait.h> so that could be dropped from your list of headers. Similarly, you aren't using <errno.h> directly; you aren't using anything from <sys/stat.h>; you aren't using anything from <string.h> — so those could be dropped from ...
11
First of all, you should definitely read the Python style guide (PEP0008). It contains tons of good formatting and style info to make code more readable and neat for you and other users. I'll reference some of the points relevant to you but the whole page is good to commit to memory.
You should rearrange the imports so they're grouped for readability. Plain ...
11
lsblk
The -s option to lsblk was introduced to util-linux rather recently, in release 2.22. You may experience compatibility issues on slightly older GNU/Linux installations.
But I don't see why you would want the -s option at all — it just gives you an inverted device tree. For example, on my machine:
$ lsblk -o name -n -s -l
sda1
sda
sda2
sda
sr0
vg-...
10
I can see nothing wrong, but I think it is not optimal. My compiler warns me
about floating point conversions of those '1E9' constants. If I rewrite your
timeAdd as follows, the code is easier to read and the generated code is
significantly smaller:
#define BILLION 1000000000
Time timeAdd(Time t1, Time t2)
{
long sec = t2.tv_sec + t1.tv_sec;
long ...
10
It's a relatively small program and pretty well written, so there's not a lot on which to comment, but here are a few things that may help you improve your program.
Handle error conditions
As it stands, if the user specifies an input file that does not exist, fopen will return a NULL value. Rather than emitting some kind of error message, this causes the ...
10
Here are some suggestions for how you might improve your code.
Use the appropriate #includes
This program fragment requires headers, which should be included:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
Use const where practical
Because the validate_path function does not alter the passed string, the parameter should be ...
10
Let's start with the obvious, coding best practices:
You should strive to make variable names easy to remember and meaningful. I think you could improve a bit on this, e.g. percent \$ \rightarrow \$ percentage_remaining, get \$ \rightarrow \$ _read_battery_descriptor.
brightness_regex isn't related to the class and should probably be a global variable:
...
10
I see some things that may help you improve your code.
Fix the bug
The program loops infinitely (actually, as @CrisLuengo correctly points out in a comment, it's not actually infinite. However, it loops \$2^{54}\$ times for each 1024 bytes in the input file on a 64-bit machine, so it will certainly feel infinite for any human being waiting for the copy to ...
10
There's two things I'd change to simplify your code:
Use a two dimensional array. Sure you can do the conversion from a 2d position to 1d easily, but the compiler can do and board[1][1] is rather more obviously the middle of the board than board[4].
Instead of hardcoding your logic for what positions you have to check, think about a programmatic approach ...
9
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" };
...
9
Adding checks? Just specify
set -e
at the beginning of the script. Should any command fail, the script will stop running, instead of wreaking more damage.
Using variables for repeated paths might make the script more readable.
tomcat1=/home/deploy/tomcatfirst
tomcat2=/home/deploy/tomcatsecond
sh "$tomcat1"/bin/catalina.sh stop
(BTW, shouldn't the ...
Only top voted, non community-wiki answers of a minimum length are eligible
