1. Over 36 million developers use GitHub together to host and review code, project manage. General purpose redis client https://stackexchange.github.io/Stack.
2. Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute: Sign up. Here's how it works.

• Code Review Template

My long term goal is to make a tool similar to but first I need to learn how to program and how to properly implement AES.So far I've been using Python for simplicity but I plan to use a compiled (memory safe) language in the future.As far as I understand, 'rolling your own crypto' is a bad idea and should be left to experts. storing the IV, ciphertext and checksum alongside each other and being able to take them appart when decrypting (currently separated using commas and split using text.split(','), 3 comma's in case 1 or 2 comma's appear in the ciphertext?)The IV is always the same size as the block size, and the block size of AES stays fixed. So you would generally just prefix it to the ciphertext (using 16 bytes, possibly converted to 32 hexadecimal characters).Your code shows you do not make a clear distinction between bytes and encoding of bytes. Encoding mistakes are one the most common mistakes made by programmers starting cryptography.

Code Review Template

dividing the data into chunks of 16 bytes (128 bits), specifically padding the last chunk to get equal sized chunks. (e.g 63 chars can be divided into 3 chunks of 16 with some leftover, so I would have to add random data to make a full chunk. How do I get rid of this when decrypting again?)This is called padding, which is not required for all modes of operation (e.g. CTR, counter mode, doesn't require it). For CBC you'd generally use PKCS#7 compatible padding. outputting ciphertext and hashes in readable form (currently using hex-encoding)Hex encoding is fine, but you might wonder why you would do this in the first place. The values will be indistinguishable from random anyway.

Stack Exchange network consists of 175 Q&A communities including Stack Overflow. Credit.c code review. Ask Question 0. You need code that gets the number of digits and tests the length. RE: testing whether a value is between two other values. There's no shorter way than what you have. You must make two separate tests, comparing the test.

Base 64 is more efficient if the ciphertext has to be transported.Other notes ( on the removed implementation):. SHA-256 is not a Password Based Key Derivation Function, don't use it on passwords (with insufficient entropy);. random.randint(0, 0xFF) is indeed insecure, so why use it? You need a cryptographically secure RNG;. using a hash as checksum may not be secure, use a (H)MAC or authenticated cipher.

We have several part time testers who do both manual testing and test automation.When I receive a pull request from them, I noticed that I am reviewing their code less strictly and allow a minor code style or organization violations to get through. My reasoning is that I know because they work in specific iterations, if I am going to request changes, this would add at least an extra day.

Slowing down the turnaround time and increasing the merge conflict probability. Of course, but I do comment/reject if I notice major issues.How problematic is this situation? Should I take into account that an author of the pull request works part-time or not?

First of all, whether a team member works part-time is irrelevant. Your team has a sprint backlog based on its velocity, which is variable for each sprint (dependent on team capacity for that sprint). If someone works only four hours a day, the workload for that person should be planned accordingly. Within the time allotted, everyone must do a proper job.The question you should ask yourself is: are we building up by these loose reviews? Or, rephrasing this question: will these minor violations have to get fixed someday or cause quality issues (e.g.

For test maintenance)?If yes, you are also artificially raising your velocity and will run into issues later on. Everyone assumes the test is properly automated but the code quality is suboptimal.

E.g. You commit a story of 10 points in 2 days, but when done properly would take 3 days. Not only does this set a similar expectation (or, in fact, limit) for all future stories, it also creates 'hidden' work (i.e. The technical debt that must be fixed but typically is not added to the backlog or status reports).If you let these kind of things through but they are important in the long run, make them visible by creating backlog items for them!

Code style:Code style checking should automated with a linter (e.g. And good style-guide like ). No-one should be allowed to push code unless they ran the linter and fixed the coding linting errors.

This should save you a lot of time.Merge conflicts:Merge conflicts should be solved by the coder. I would expect before the push the latest code would be merged into their code first. So it should not matter how long they take, merge conflicts should not be your problems, because how can you be sure how to solve them if you didn't code it. Normal code would have tests, but testcode doesnt. (hmmm, maybe we should test our code?;-) Everyone should start their day with pulling the code from your main integration branch.Code-reviews:Code-reviews should focus on knowledge sharing and code-understandability.If you find issues you have two options:. Fix it yourself.

Send it backI would send it back, even it costs a day extra in cycle-time. How would they learn and grow? If they are really a person you rarely use and getting them to grow is a waste, choose to fix it yourself. Don't let messy-code slip through. You will hate yourself in the longterm as you never make time to clean it later on.