Hot answers tagged

30

I will go through your code line by line and give feedback. We will skip the general advice on don't use CGI as it's actually suited for what you are trying to do here. I wrote this answer in two stages, so some of the things here might have been said by other people. I'm attempting to provide full feedback, and I'm not copying from others. If several of us ...


27

You manage to do the right thing, but the way that you do it is a bit... what shall we say...? twisted? backwards? non-optimal! Your approach: Get list of all files in directory Loop over a range and add the possible names to a list Loop through the possible names and check if it matches an existing file My comments to that: Using the correct data ...


25

I recommend practicing Python 3 rather than Python 2 these days. According to PEP 8, isClean() should be is_clean(). One or more of your loops could be replaced by some use of the any() function. Note that this suggests that an is_explicit() function would be a more natural concept than an is_clean() function. I would expect song lyrics to contain a ...


23

C would have allowed you to just ignore errors and pretend everything was good. A naïve cat would be very short, but rather unreliable also. A well‐written cat will go and use all the error codes that are returned to produce a useful result. Python and similar languages would raise exceptions in these cases, leading to you getting tracebacks in case of ...


22

Fast comes at a price.... code complexity and perhaps readability. Assuming that your code produces the right results now.... and it is a big assumption because: it expects the word to be at the beginning/end of the line, or surrounded by spaces (not commas, punctuation, etc.) it does not look for the word inside another string, it will match 'are', but ...


22

There's a lot to say here, so let's not waste any time and jump right into it: You said that you didn't want to hear the using namespace std;-drill, but using namespace std; is so dangerous that I'm just going to ignore that and give it to you anyways. So here it goes: Don't use using namespace std;, you're going to shoot yourself in the foot eventually. ...


22

Efficient file I/O By default, files opened with fopen() are buffered, meaning that not every call to fread() or fwrite() will result in a system call. Instead, the C library has an internal buffer and will try to read and write larger chunks at a time. However, you are still paying for the overhead of a regular function call each time you call fread() and ...


22

log is already declared in <math.h>. You don't need to declare it yourself. In fact, it could be harmful. As stated in another answer, do not use floating point math. In fact, you don't need to know the exact position of the leftmost bit. For your purposes, the value of codepoint is enough. For example, bitPos <= 7 is equivalent to codepoint < (...


21

1. Bugs If the open succeeds but the lseek fails (as it might if the file is a named pipe), the function omits to close the file descriptor, thus leaking it. If read reaches the end of the file (as it might if the file was truncated after the lseek), then it returns 0. This will cause the function to go into an infinite loop. There's no check on the result ...


21

Splitting line into words using words = line.split(" ") will only split the line on space characters. If two words are separated by a tab character, or other white space character other than a space, the split will not happen at that point, which may confuse your explicit checker. Using words = line.split() would split on spaces, tab characters, and any ...


20

Turning my comment into an answer by request. You should be able to do what you want by simply using the functionality already built into LibGDX. Caveat: I have not tried this, I just know that the class exists and it should be possible to extract the data you want from it, so: some assembly may be required on your part. LibGDX has functionality for ...


19

Your code has a few problems. You should learn to not use using namespace std;. It's generally frowned upon. You should never use while(!inFile.eof()). It's pretty much a guaranteed bug. You should use standard algorithms when they're applicable (as they are here). Prefer to fully initialize variables at creation (e.g., pass file name when you create a ...


17

Endianness Your program works on little endian machines only. If you care for it to work on any machine, you would need to fix up all of the fwrite() calls to output specifically in little endian format. Use struct for header I would suggest creating a struct for the file header. That way, you could fill in all the fields (in an endian correct manner) and ...


16

Here are some things that may help you improve your code. Don't abuse using namespace std I know you said you don't want to hear this, but your code is unusable as a library until this is addressed. Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. If you put it into a header (a .h file) it makes your ...


16

Some suggestions: Using argparse to parse arguments is preferable to any kind of interactive input, because it means that it's much easier to automate and integrate in other scripts, you could trivially support passing multiple songs in one command, it's what any experienced shell user would expect, and if the user adds --help or doesn't pass any arguments ...


15

Some notes: Those counter classes are probably overkill, keep it simple. Ruby is an OOP language, but it's not necessary to create a bunch of classes for simple scripts like this. Idiomatic: if not x -> if !x Idiomatic: { ... } for one-line blocks, do/end for multi-line. I'd write: fail("Usage: #{0} PATH (lines|words|find REGEXP)") unless ARGV.size >...


15

The posted code does the following: Read all lines, just for counting Create an array with the right size Read all lines again, this time for storing Print all lines in reverse order It's wasteful to read a file completely just to find the number of lines, and then need to read the file again to store the content. It would be better to store the lines in a ...


15

Few things. This number: unsigned long max = std::numeric_limits<unsigned long>::max(); Is 18446744073709551615 If we assume that the average size of a number is half this length (so 10 characters). Then the size of the file is: 184467440737095516150 bytes. => 180143985094819830 Kbytes => 175921860444150 MBytes => 171798691830 GBytes =...


15

fopen should have a b in the second parameter. This primarily prevents windows from replacing any \n byte with \r\n which would corrupt the file. fopen( fileName, "wb" ) Make the order of the fwrite parameters consistent first the sizeof and then the number of elements: // Writing the RIFF header: fwrite(&fChunkID, sizeof(fChunkID), 1, fout); ...


15

Functions Split the code into functions, also split the generation and the presentation. Your algorithm has some clear distinct tasks, so split along these lines: read input assemble chain construct new poem output This way, you can reuse parts of the code, save intermediary results and test the parts individually. generators instead of keeping all the ...


15

I see a number of things that may help you improve your program. Fix the bug There is a subtle bug in the original implementation. It contains these lines: // If the word contains one of the bad letters, don't accept it. for (unsigned int i = 0; i < sizeof(badLetters); i++) { if (currentLine.find(badLetters[i]) != std::string::npos) { ...


15

Not duplicating any of @Peilonrayz's code review points ... Stop reading entire files into memory when you can process the file line by line in one pass, and stop creating huge lists in memory which are then iterated over exactly once. Both of these things creates a huge unnecessary memory pressure which can be avoided by looping and/or using generator ...


14

using namespace std; is a bad habit. Declare variables as close to use as possible. (for example, declare count about the while loop rather than at the top). An eof loop control does not work the way you think it does. eof is not reached until after the end of file is attempted to be read past. This means that your last read can silently fail. The more ...


14

You’ve already noted that this could be done better without macros, so I won’t belabour the point. I will note, though, that your goal—“to refresh [your] skills at writing good solid macros”—makes about as much sense as refreshing your skills at writing code on punch cards. You are exercising an archaic practice that is dying out, and is unwelcome in any ...


13

Magic numbers 20 and 19? Instead of this: char fileName[20]; // Get the filename from the user: int fileNameLength = read(0,fileName,19); Define MAX_FILENAME_LENGTH somewhere, and use that as the buffer size parameter, and MAX_FILENAME_LENGTH + 1 as the array size containing the buffer, to account for the terminating null character. Write in code your ...


13

The purpose of constructors The purpose of constructors is to create objects. The constructor of FntToAtlasGenerator is something else entirely: it takes some input and generates some output in the filesystem, and the class has no other public methods and purpose. As such, the glyphs member variable is pointless, and all the code you have in this ...


12

There are quite some things you can improve here, this list may not cover everything that could be improved though: Adhere to the Java naming conventions. Classes are named in PascalCase for example, so it would be public class Meow. On other places it seems fine. Use proper indentation everywhere, this definately applies to the main method inside public ...


12

Actually, this can be more generic. Instead of hard-coding your map; fill it from your properties file. So change your prop file to : #db.id=a #db.user=testa #db.password=testa db.id=b db.user=testb db.password=testb Fill your map like this : private Map<String, String> schemaMap = new HashMap<String, String>(); public void fillMap() ...


12

It actually doesn't look too bad. Some minor nitpicks You shouldn't cast the return value of malloc. fByteRate and fBlockAlign can be const since they're being computed from const values only. I'm not entirely sure why pretty much all variables in WriteWavePCM are prefixed with f. I suppose this is meant to indicate that they are related to data being ...


12

Here is a list of things I would improve: define constants in upper-case (PEP8 reference) put the main program logic into the if __name__ == '__main__': to avoid it being executed on import naming according to PEP8 - getLine() to get_line() (or even better name would be something like get_stats()) remove unused sys import follow the PEP8 import organization ...


Only top voted, non community-wiki answers of a minimum length are eligible