Hot answers tagged

11

using namespace std; Don't do using namespace std;. It can lead to name collisions and other issues. Typing out the full names (e.g. std::cout) avoids such issues, and is also clearer for readers. using namespace std::literals; is reasonable inside a .cpp file, or a function. string_view literals std::string_view filename{""s}; s is the std::...


10

Header The header file includes a couple of headers that it doesn't need: #include <fstream> #include <iostream> That said, I would consider including <iosfwd> and declaring functions that work on streams (probably using >> and <<) rather than files, to make unit-testing possible. It also has using namespace std;, which is ...


8

Also, ⧺SL.io.50 Don't use endl. Don't explicitly compare against nullptr. Use the truth value of the pointer (or smart pointer!): if (argv[++i]). In this case, modifying the index as part of the long expression that refers to it several times is confusing. I get the idea that this is boilerplate code anyway, so use a proper library instead of pasting it. ...


7

Style: using namespace std; This is generally seen as poor practice because it imports all symbols from the std namespace in the main namespace, somewhat defeating the namespace isolation and highly increasing the risk of name collision. Explicitely importing only the used symbols is generally seen as better practice, even if it requires more typing: using ...


6

The parameters to the TAPHeader constructor and TAPBlock::operator<< could be passed as const references to avoid making copies of the objects being passed. explicit TAPHeader(const std::vector<uint8_t> &v); // ... friend std::ostream& operator<<(std::ostream& out, const TAPBlock &blk); In the TAPHeader constructor, you ...


6

Some comments to consider: In TAPBlock::read, I wouldn't try to get cute and declare the two variables lo and hi on the same line. Instead, I would do const uint8_t lo = in.get(); and then const uint8_t hi = in.get(); on the next line. (So just pay attention to const correctness). Also, TAPBlock::operator<< looks a little strange with its if-else. ...


5

Thou shalt not cast malloc. It is redundant (if you #include <stdlib.h>), and may lead to hard-to-find bugs (if you don't). sort_array is allocated at each iteration, yet never freed. That said, I don't see why it needs malloc at all. Declare it statically unsigned char sort_array[9]; DRY. The code for R, G, and B channels, thrice repeated, cries ...


5

General Observations It would be much easier to review this code if there was either a unit test or example code to see how this code should be used. It also isn't clear why the code deletes the key value pair within the function getValueFor(). The function name does not indicate that it is deleting the pair as well as getting the value for the key. Error ...


5

Although it makes the code a bit longer, I think I'd start by writing a little "buffer" class, on this general order: class buffer { std::vector<uint8_t> &data; std::vector<uint8_t>::iterator pos; public: buffer(std::vector<uint8_t> &data) : data(data) , pos(data.cbegin()) {} void ...


5

Don't overuse std::vector You are using std::vector for many things that shouldn't be vectors. For example, storing a single vertex should be done in a struct that holds the x, y and z coordinates: struct dvec3 { double x; double y; double z; }; I named it dvec3 because it is a vector (in the mathematical sense) of 3 doubles, and it follows the ...


4

Naming FileIO: The File class resides inside the System.IO namespace. FileIO is quite a misfortune chose of name. TextFileWrapper, TextFileUtil or TextFileManager would be a more appropriate name for this in my opinion. FileWrite: The general guidance is to start your methods with a verb. So, WriteFile would be a better name for this. ReadTxtFile: This name ...


4

Since the split files do not need to be readable text files, I would read & write in chunks of bytes, not in lines. This should be faster than reading and writing line by line. We can go with a chunk size of 4,096 bytes because 4,096 or 8,192 is a typical block size on most file systems. Although, I should note that when I tested your script on my system ...


4

You’ve got some excellent answers commenting on the code itself. I am here to comment on the algorithm. It is really hard to implement a color median filter. The median is defined for a set of scalar values (and thus it is straight-forward to define a median filter that works on a gray-scale image). But the median of a set of 3-vectors (such as RGB pixel ...


4

The biggest simplification and speed improvement would be to avoid using the split function. I would suggest just multiplying the 3-channel image directly. Something like this: cv::multiply(img_hsv, Scalar(1, 1, enhancement_times), img_hsv); Of course, we can’t know for sure which is faster until we compare the running time for each of the alternatives. In ...


4

Sorry it took so long—this is a very busy time of year for me (and it’s getting worse)—but as promised, here is an error-handling-focused review. This review is laser-focused just on the error-handling in the original code; other reviews do a fine job of covering other angles. The main focus is going to be error-handling with IOstreams, but I’ll brush on ...


3

int vs. size_t math ...,const int xsize,const int ysize) ... output = (RGB*)malloc(xsize * ysize * sizeof(RGB)); // int * int size_t math typically exceeds int (and maybe unsigned) math and may handle int overflow of xsize * ysize. Consider dropping the unneeded cast, re-order and size to the referenced data, rather than type. output = ...


3

Naming C reserves identifiers beginning with _ followed by any letter for the implementation, so they should not be defined by user programs. I can't quite remember whether leading _ is permitted in struct tags (that's a different namespace to object and function names), because it's simpler to just avoid using leading _ anywhere, and never have to know the ...


3

Why use an int to indicate the log type when you can just use an enum That way, you could just write the enum to the OutputStreamWriter directly (while avoiding the worry of receiving an incorrect logType: IllegalArgumentException): public class Logger implements Closeable { public enum LogType { FATAL, ERROR, WARN, INFO, DEBUG, TRACE; } ... ...


3

Auto-boxing has been around long enough that I think you only need one log method. Even here I'd avoid single letter names. public void log(Object toLog, Boolean newLine) { osw.write(getTime() + '\n' + String.valueOf(toLog) + (newLine? '\n' : "")); } You don't need to instantiate a date formatter each time you get the time, do you? You don'...


3

Avoid using namespace std Avoid using namespace std, it is considered bad practice. It saves only a little bit of typing, but you can run into issues if names of local classes and variables clash with those defined in the standard library. Use INT_MAX instead of INF Don't #define INF to some arbitrary value, just set it to exactly the largest possible value ...


3

Unconditional masks This: if (byte & (1 << (CHAR_BIT - bit - 1))) *value = (*value & ~1) + 1; /* 1 */ else *value = *value & ~1; /* 0 */ is really just *value &= ~1; if (byte & (1 << (CHAR_BIT - bit - 1))) *value |= 1; Const arguments void hide_file(char *filename, char *src_png_name, char *...


3

Since you are already requiring the filename to start with the correct drive name, your target will already look like a normal valid path. So you could at least check if what the user entered is actually the correct path, short-circuiting the whole search: def search(drive, target): if os.path.isfile(target): return os.path.abspath(target) ......


3

Pass a std::istream& instead of a filename Your function has two responsibilities: opening the file and reading it. I would let it only read from an already opened file. This means the function can be shorter and have less responsibilities, and at the same time flexibility is increased because now it can read from any std::istream, not just from a std::...


3

Don't use long double Almost no code needs long double; it has much more precision than you need, and it is much less efficient than a float. Really, stop using it. Use size_t for sizes, counts and indices This is something that has been mentioned multiple times in answers to earlier questions posted by you. An int is not guaranteed to be large enough to ...


3

It's not necessary to represent this as a class; a read and write function will suffice Don't use double-underscores; that's reserved for name mangling The second argument of Union[str, Path] is more generically expressed as os.PathLike Pickled objects are already length-aware. The much easier approach than what you've shown here is to omit any manual length ...


3

if I can make some improvements to it or if there are any error. Lack of error checking fopen(), malloc(), ftell(), fread(), (all I/O functions), etc. deserve to have their return values checked for errors. In particular when a file may fail to open or memory fail to allocate, handle such cases to not forget the other. e. g.: do not forget to fclose(), or ...


3

Don't raise BaseException Typo: posible -> possible Don't type-hint a list; instead use List[str] in the case of your posible_config_files Consider making a reusable solution that separates "all existing config files" from "the first existing config file" Suggested from pathlib import Path from typing import Iterable def config_files(...


2

Let's look at the signature of the function: bytes read_block(uint32_t offset, uint32_t length, const string& filename) Instead of using the (optional) std::uint32_t we should accept the type we actually need for seekg() and read(), namely std::ifstream::pos_type and std::streamsize. Consider reordering the arguments ...


2

General Feedback The class looks okay. I like how the method names follow PSR-12 recommendation of method names in camelCase, however it is also recommended that an underscore not be used to indicate private properties1 Property names MUST NOT be prefixed with a single underscore to indicate protected or private visibility. That is, an underscore prefix ...


2

Try using pathlib over os when you're dealing with file/glob operations. It has a lot cleaner interface, and is part of standard library in python 3. Name constants in your code with the CAPITAL_SNAKE_CASE. It is recommended guideline from the PEP-8. Split your code into individual functions doing a single task each. Put the execution flow for your code ...


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