Java app for getting metadata of millions of files in a directory I am writing a Java app that gets the metadata of files in a directory and exports it to a .csv file. The app works fine if the number of files is fewer than a million. But if I feed in a path that has about 3200000 files in all of directories and sub-directories, it takes forever. Is there a way I can speed up things here?

    private void extractDetailsCSV(File libSourcePath, String extractFile) throws ScraperException {

    log.info("Inside extract details csv");

    try{
        FileMetadataUtil fileUtil = new FileMetadataUtil();

        File[] listOfFiles = libSourcePath.listFiles();

        for(int i = 0; i < listOfFiles.length; i++) {

            if(listOfFiles[i].isDirectory()) {
                extractDetailsCSV(listOfFiles[i],extractFile);
            }

            if(listOfFiles[i].isFile()){

                ScraperOutputVO so = new ScraperOutputVO();

                Path path = Paths.get(listOfFiles[i].getAbsolutePath());

                so.setFilePath(listOfFiles[i].getParent());
                so.setFileName(listOfFiles[i].getName());

                so.setFileType(getFileType(listOfFiles[i].getAbsolutePath()));

                BasicFileAttributes basicAttribs = fileUtil.getBasicFileAttributes(path);
                if(basicAttribs != null) {
                    so.setDateCreated(basicAttribs.creationTime().toString().substring(0, 10) + " " + basicAttribs.creationTime().toString().substring(11, 16));
                    so.setDateLastModified(basicAttribs.lastModifiedTime().toString().substring(0, 10) + " " + basicAttribs.lastModifiedTime().toString().substring(11, 16));
                    so.setDateLastAccessed(basicAttribs.lastAccessTime().toString().substring(0, 10) + " " + basicAttribs.lastAccessTime().toString().substring(11, 16));
                }

                so.setFileSize(String.valueOf(listOfFiles[i].length()));
                so.setAuthors(fileUtil.getOwner(path));

                so.setFolderLink(listOfFiles[i].getAbsolutePath());
                writeCsvFileDtl(extractFile, so);

                so.setFileName(listOfFiles[i].getName());
                noOfFiles ++;
            }
        }
    } catch (Exception e) {
        log.error("IOException while setting up columns" + e.fillInStackTrace());
        throw new ScraperException("IOException while setting up columns" , e.fillInStackTrace());
    }

    log.info("Done extracting details to csv file");
}

public void writeCsvFileDtl(String extractFile, ScraperOutputVO scraperOutputVO) throws ScraperException {
    try {
        FileWriter writer = new FileWriter(extractFile, true);
        writer.append(scraperOutputVO.getFilePath());
        writer.append(',');
        writer.append(scraperOutputVO.getFileName());
        writer.append(',');
        writer.append(scraperOutputVO.getFileType());
        writer.append(',');
        writer.append(scraperOutputVO.getDateCreated());
        writer.append(',');
        writer.append(scraperOutputVO.getDateLastModified());
        writer.append(',');
        writer.append(scraperOutputVO.getDateLastAccessed());
        writer.append(',');
        writer.append(scraperOutputVO.getFileSize());
        writer.append(',');
        writer.append(scraperOutputVO.getAuthors());
        writer.append(',');
        writer.append(scraperOutputVO.getFolderLink());
        writer.append('\n');
        writer.flush();
        writer.close();
    } catch (IOException e) {
        log.info("IOException while writing to csv file" + e.fillInStackTrace());
        throw new ScraperException("IOException while writing to csv file" , e.fillInStackTrace());

    }
}
}

I suspect that using the old File class of Java is the possible root problem of your solution right now. Since you're using Java 7, you should use those new classes. I've seen that you use some of them like Path, so it shouldn't be too difficult. I don't what your class look at moment so I've changed some method base on what I'm use to do. So the class I will be using is SimpleFileVisitor since this is the basic implementation of FileVisitor.

So I've created a class Walker (this is a very bad name, you should change it for something clearer for you, since I have no good idea right now) that extends SimpleFileVisitor. The class has an attribute extractFile that correspond to the filename of the csv. This class will have the preVisitDirectory, visitFile and visitFileFailed that we will override from the FileVisitor. I've also added your method writeCsvFileDtl, createDate (thanks to @unholysampler, you should read his answer too).

So the class should look like that :

import static java.nio.file.FileVisitResult.CONTINUE;
import java.io.FileWriter;
import java.io.IOException;
import java.nio.file.FileVisitResult;
import java.nio.file.Path;
import java.nio.file.SimpleFileVisitor;
import java.nio.file.attribute.BasicFileAttributes;
import java.nio.file.attribute.FileTime;

public class Walker extends SimpleFileVisitor<Path> {

    private String extractFile;

    public Walker(String extractFile) {
        this.extractFile = extractFile;
    }

    @Override
    public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attr)
            throws IOException {
        populateAndWrite(dir, attr);
        return CONTINUE;
    }

    @Override
    public FileVisitResult visitFile(Path file, BasicFileAttributes attr) {
        populateAndWrite(file, attr);
        return CONTINUE;
    }

    @Override
    public FileVisitResult visitFileFailed(Path file, IOException exc) {
        //You should determine if you need this method or not
        return CONTINUE;
    }

    private void populateAndWrite(Path file, BasicFileAttributes attr) {
        ScraperOutputVO so = new ScraperOutputVO();
        if (file.getParent() != null) {
            so.setFilePath(file.getParent().toString());
        }

        if (file.getFileName() != null) {
            so.setFileName(file.getFileName().toString());
        }

         so.setFileType(getFileType(file.toAbsolutePath().toString()));

        if (attr != null) {
            so.setDateCreated(createDate(attr.creationTime()));
            so.setDateLastModified(createDate(attr.lastModifiedTime()));
            so.setDateLastAccessed(createDate(attr.lastAccessTime()));
        }
        if (!attr.isDirectory()) {
            so.setFileSize(String.valueOf(attr.size()));
        }

        so.setAuthors(fileUtil.getOwner(file));

        so.setFolderLink(file.toAbsolutePath().toString());
        try {
            writeCsvFileDtl(extractFile, so);
        } catch (IOException e) {
            log.info("IOException while writing to csv file" +
            e.fillInStackTrace());
            throw new
            ScraperException("IOException while writing to csv file" ,
            e.fillInStackTrace());
        }
    }

    private String createDate(FileTime time) {
        String timeStr = time.toString();
        return timeStr.substring(0, 10) + " " + timeStr.substring(11, 16);
    }

    private void writeCsvFileDtl(ScraperOutputVO scraperOutputVO)
    throws ScraperException {
        try {
            FileWriter writer = new FileWriter(extractFile, true);
            writer.append(scraperOutputVO.getFilePath());
            writer.append(',');
            writer.append(scraperOutputVO.getFileName());
            writer.append(',');
            writer.append(scraperOutputVO.getFileType());
            writer.append(',');
            writer.append(scraperOutputVO.getDateCreated());
            writer.append(',');
            writer.append(scraperOutputVO.getDateLastModified());
            writer.append(',');
            writer.append(scraperOutputVO.getDateLastAccessed());
            writer.append(',');
            writer.append(scraperOutputVO.getFileSize());
            writer.append(',');
            writer.append(scraperOutputVO.getAuthors());
            writer.append(',');
            writer.append(scraperOutputVO.getFolderLink());
            writer.append('\n');
            writer.flush();
            writer.close();
        } catch (IOException e) {
            log.info("IOException while writing to csv file" +
             e.fillStackTrace();
            throw new ScraperException("IOException while writing to csv file",
                    e.fillInStackTrace());

        }
    }
}

The method populateAndWrite is use in preVisitDirectory and visitFile, basically it will populate each attribute of your object ScraperOutputVO and then send it to the the write method. I'm not sure if you to list directories, so if you don't want to just remove preVisitDirectory. I've added some nullcheck since those method can return null if you start at the root directory.

You'll maybe need to tweak some attributes, cause I didn't had access to your fileUtils and getFileType, so you should test to make sure you have the same values.

To launch the class you simply need to something like :

public static void main(String args[]){
    Path root = Paths.get("Path to your directory");
    Walker walker = new Walker("Name of your csv file");
    try {
        Files.walkFileTree(root, walker);
    } catch (IOException e) {
        //you should handle exception here
        //log.info("Problem walking the directory")
        e.printStackTrace();
    }
}

If you need more help just comment on the answer.

Edit: I find another amelioration that would lower the time of the execution. The problem is when you write to the file, you always create a new FileWriter, open the file, write to it and then close it. So for each file you hit, you're opening accessing the file and closing it which is causing a major drawback on performance. By using your current  writeCsvFileDtl it took me like 170 seconds to write 100 000 entries. By leaving open the the FileWriter writing took less than one second. You should do something like that, it is not very elegant (you could probably do something more readable or cleaner) but will enhance the performance.

import java.io.FileWriter;
import java.io.IOException;

public class Writer {
    static FileWriter writer = null;
    public static void openFileWriter() {
        try {
            writer  = new FileWriter("C:/dev/file.txt", true);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
    public static void writeToTheFileWithOpenEachTime(){
          try {
                writer.append("firstColumn");
                writer.append(',');
                writer.append("secondColumn");
                writer.append(',');
                writer.append("thirdColumn");
                writer.append(',');
                writer.append("fourthColumn");
                writer.append(',');
                writer.append("fifthColumn");
                writer.append(',');
                writer.append("sixthColumn");
                writer.append(',');
                writer.append("sevenColumn");
                writer.append(',');
                writer.append("eigthColumn");
                writer.append(',');
                writer.append("ninethColumn");
                writer.append('\n');
                writer.flush();
            } catch (IOException e) {
                e.printStackTrace();
            }
    }

    public static void closeWriter() {

        try {
            writer.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

}


Validating a name with C#, ASP.NET Core 3.1 and Regex This class validates that a name contains only letters and optionally spaces, hyphens and apostrophes.
public class NameAttribute : ValidationAttribute
{
    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        if (value == null) return ValidationResult.Success;

        string name = value.ToString();
        var regex = new Regex(@"^[a-zA-Z]+(?:['-][a-zA-Z\s*]+)*$");

        return regex.IsMatch(name)
            ? ValidationResult.Success
            : new ValidationResult($"The name '{name}' is invalid, it should consist of only letters, and optionally spaces, apostrophes and/or hyphens.");
    }
}

One thing I wasn't sure on was the null check at the start, we've got the Required attribute if something is required and I don't want the validation to occur if the value is null. Is this an appropriate way of handling it?

From the separation of concerns perspective it seems fine that you do not want to validate against emptiness. On the other hand, one can argue that why do you treat an empty string as a valid name.
My suggestion is to create a ctor with a parameter, where the consumer of the attribute can define how should it behave in case of empty input.
public class NameAttribute : ValidationAttribute
{
    private static readonly Regex NameRegex = new Regex(@"^[a-zA-Z]+(?:['-][a-zA-Z\s*]+)*$", RegexOptions.Compiled);
    private readonly bool shouldTreatEmptyAsInvalid;
    public NameAttribute(bool shouldTreatEmptyAsInvalid = true)
    {
        this.shouldTreatEmptyAsInvalid = shouldTreatEmptyAsInvalid;
    }

    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        if (value == null)
            return shouldTreatEmptyAsInvalid
                ? new ValidationResult($"The name is invalid, it should not be empty.")
                : ValidationResult.Success;

        return NameRegex.IsMatch(value.ToString())
            ? ValidationResult.Success
            : new ValidationResult($"The name '{value}' is invalid, it should consist of only letters, and optionally spaces, apostrophes and/or hyphens.");
    }
}

I moved the creation of the Regex to a static variable, but you should take some measurements in order to be sure, which has the best performance. Please check this MSDN article for other alternatives.


ProjectEuler.net problem #50, Consecutive Prime Sum I am trying to solve ProjectEuler.net problem #50, Consecutive Prime Sum. Here is the problem:

The prime 41, can be written as the sum of six consecutive primes:
41 = 2 + 3 + 5 + 7 + 11 + 13

This is the longest sum of consecutive primes that adds to a prime below one-hundred.
The longest sum of consecutive primes below one-thousand that adds to a prime, contains 21 terms, and is equal to 953.
Which prime, below one-million, can be written as the sum of the most consecutive primes?

I have written some code which solve the problem just fine when the limit is 10, 100, 1000 or 10000 but when the limit is 1000000 as the problem requires, the program takes too much time to finish running!!
What can be done to my code to make the program faster?
package com.company;
import java.util.ArrayList;

class ConsecutivePrimeSum {
    public static int limit=1000000;
    int lengthOfTheLongest =0;
    int sumOfTheLongest =0;

    void solution(){
        ArrayList<Integer> arrayOfPrimes=generatePrimes();
        scanSequences(arrayOfPrimes);
        System.out.println("The longest sum is "+ sumOfTheLongest +"  and contains "+lengthOfTheLongest+" terms");
    }
    private ArrayList<Integer> generatePrimes(){
        ArrayList<Integer> arrayOfPrimes=new ArrayList<Integer>();
        for(int i=limit;i>=2;i--){
            if(isPrime(i)){
                arrayOfPrimes.add(i);
            }
        }
        return arrayOfPrimes;
    }

    /**
     * @param s
     * this scans ArrayList s, to get sequence of prime numbers and calculate
     * their sum and corresponding length
     * then assign the longest length to the variable lengthOfTheLongest, and its sum to variable sumOfTheLongest
     */
    private void scanSequences(ArrayList<Integer> s){
        ArrayList<Integer> cumulativeSums=generateCumulativeSums(s);
        for(int start=1;start<=s.size();start++){
            for (int end=s.size()-1;end>=start;end--){
                if(!isPrime(sumFromTo(cumulativeSums,start,end)))  continue;
                if(sumFromTo(cumulativeSums,start,end)>limit) break;
                if ((end-start+1)> lengthOfTheLongest){
                    sumOfTheLongest =sumFromTo(cumulativeSums,start,end);
                    lengthOfTheLongest =(end-start+1);
                }
            }
        }

    }

    /**
     * @param s
     * @return arraylist of cumulative sums of the elements in s
     */
    private ArrayList<Integer> generateCumulativeSums(ArrayList<Integer> s){
        int sum=0;
        ArrayList<Integer> cumulativeSums=new ArrayList<Integer>();
        for (int i=0;i<s.size();i++){
            cumulativeSums.add(sum=sum+s.get(i));
        }
        return cumulativeSums;
    }

    /**
     * @param a is an ArrayList whose elements are to be summed
     * @param start is the index of where summing should start
     * @param end is the index of where summing should end
     * @return the obtained sum
     */
    private int sumFromTo( ArrayList<Integer> a,int start, int end){
        int sum;
        sum=a.get(end)-a.get(start-1);
        return sum;
    }

    private static boolean isPrime(int number){
        boolean isPrime=false;
        int divider=2;
        int count=0;
        while(divider<=number){
            if(number%divider==0)
                count=count+1;
            divider=divider+1;
        }
        if(count==1)
            isPrime=true;

        return isPrime;
    }

}
class Test{
    public static void main(String [] args){
        ConsecutivePrimeSum consecutivePrimeSum=new ConsecutivePrimeSum();
        consecutivePrimeSum.solution();
    }
}

```

Generation of prime numbers is suboptimal. Use a sieve of Erathosthenes.
isPrime is highly suboptimal. You already generated an array of all necessary primes, so just binary search it.
Breaking the loop in

        if(sumFromTo(cumulativeSums,start,end)>limit)
            break;

looks like a bug. The intention is to loop by decreasing end, yet since the loop starts with the very long sequence, its sum is likely to exceed the limit right away. Shorter sequences with the same start are never tested.

Consider finding the proper end as an upper limit of a sumFromTo(cumulativeSums, start, end) <= limit predicate (hint: another binary search).
You are not interested in all sequences of the primes. Most of the primes execs 2 are odd. Notice that if the sequence has an even number of odd primes, its sum is even, that is not a prime. Once the correct end is established, you may safely decrease it by 2.
Style wise, give your operators some breathing space.

    for (int end = s.size() - 1; end >= start; end--) {

is much more readable than

    for (int end=s.size()-1;end>=start;end--){

PS: I am not aware of any math regarding sums of consecutive primes. It is very much possible that such math exists, and the goal of this problem is to discover it. That would be a true optimization.


Iterative Implementation of Towers of Hanoi Here is an implementation of Towers of Hanoi based on few observed patterns1 from the easier recursive solution:

function [] = myTowersOfHanoi(N, from, to, alt)
  % Accepts three integers: N - number of disks
  % from - number of start tower, to - number of end tower, alt - free tower.
  % Returs string outputs with succesive moves to complete the task of solving
  % the Towers of Hanoi with N disks moved from tower with number stored
  % in the second argument to one with number in third arg.

  totalNumberOfMoves = (2 ^ N) - 1;

  M = generateDiskMoves(totalNumberOfMoves);

  % These are the paths of disks if N is odd.
  path1 = [from, alt, to];  % Path of disk with odd number: from->alt->to
  path2 = [from, to, alt];  % Path of disk with even number.

  currentPositions = ones(1, N); % index-disk number, value-number of moves

  len = numel(path1);

  % If N (numer of disks) is even the paths are swapped.
  if mod(N, 2) == 0
    [path2, path1] = swapArrays(path1, path2);
  end

  % Solve
  for i = M
    from = -1;
    to = -1;

    if mod(i, 2) == 0  % if number of disk, i is even
      j = currentPositions(i); % j - number of moves for i-th disk

      % In C++ indexes: [0, size - 1] in Octave: [1, size]
      % so: mod(j - 1, len) + 1, to avoid index = 0.
      from = path1( mod(j - 1, len) + 1); % Cycle over 1->2->3

      j = j + 1;

      to =  path1( mod(j - 1, len) + 1);

      currentPositions(i) = j; % update moves of i-th disk

    else
      k = currentPositions(i);

      from = path2( mod(k - 1, len) + 1);
      k = k + 1;
      to =  path2( mod(k - 1, len) + 1);

      currentPositions(i) = k;
    end
      disp(sprintf('Move disk %d from %d to %d.', i, from, to))
  end

end

function [a2, a1] = swapArrays (a1, a2)
  [a2, a1] = deal(a1, a2);
end

% From: http://mathworld.wolfram.com/BinaryCarrySequence.html
function [M] = generateDiskMoves(N)
  % Accepts integer: N - total number of moves.
  % Returns a 1xN integer array with the first N consecutive disk moves
  % in Tower of Hanoi where the index is the move number
  % and the value is the disk number. m - is discarded.

  [m, M] = Omega2(N); % Generate the first N terms of: "Binary Carry Sequence".

  M = M .+ 1;         % Add one and get moves of disk in Tower of Hanoi.

  if N < 2            % Get only the first move.
    M = M(1);
  end
end

% From : https://oeis.org/A007814
function [m, M] = Omega2(n)
  % Accepts an integer: n.
  % Returns m: max power of 2 such that 2^m divides n, and
  % M: 1-by-K matrix where M(i) is the max power of 2 such
  % that 2^M(i) divides n.

  M = NaN * zeros(1, n);

  M(1) = 0;
  M(2) = 1;

  for k = 3 : n
    if M(k - 2) ~= 0
      M(k) = M(k - k / 2) + 1;
    else
      M(k) = 0;
    end
  end

  m = M(end);
end

Input: Move 4 disks from 1st to 3rd peg, 2nd is free.

myTowersOfHanoi(4, 1, 3, 2)

Output:

Move disk 1 from 1 to 2.
Move disk 2 from 1 to 3.
Move disk 1 from 2 to 3.
Move disk 3 from 1 to 2.
Move disk 1 from 3 to 1.
Move disk 2 from 3 to 2.
Move disk 1 from 1 to 2.
Move disk 4 from 1 to 3.
Move disk 1 from 2 to 3.
Move disk 2 from 2 to 1.
Move disk 1 from 3 to 1.
Move disk 3 from 2 to 3.
Move disk 1 from 1 to 2.
Move disk 2 from 1 to 3.
Move disk 1 from 2 to 3.

I would appreciate your opinion and suggestions related to:

MATLAB / Octave coding style and readability.
thoughts on / possible improvements of the algorithm.

1. The observations were that firstly: the sequence of transitions could be  described by a slightly modified formula: "Binary Carry Sequence" and secondly: individual disk transitions are following only two different cyclic paths which were based on the parity of the total number of the disks, N, and the parity of the number of the currently moving disk, i.e:

Wow. Some legible matlab code. I'm impressed. Too often matlab seems to be a "write only" language, in the sense that perl regex line noise or Iverson's APL can be write only.

No biggie, but I wouldn't mind seeing consistent comment formatting where (N, from, to, alt) appear in the left margin in each of four separate lines. Kudos for telling us about the args, anyway. In the matlab ecosystem this is maybe redundant, but speaking for myself I wouldn't mind seeing a reminder that there's no zero-origin going on here, by mentioning from > 0 or something. Saying it once would suffice - to & alt would clearly use the same convention. I see that later you spell this out in the "avoid index = 0" comment.

typo: Returs

Kudos on helpfully explaining that 2^N-1 is totalNumberOfMoves.

Your figure was helpful. The "from -> alt -> to" comment is on the redundant side.

Would you do the Gentle Reader a small favor, please, and bump the currentPositions and len assignments down slightly? Just a few lines. That way we have a full-line comment on the "odd" case, setting up dramatic tension for "what about even?", and the swapArrays immediately shows the even case.

Switching from j to k for the path2 case was maybe a little odd. Wouldn't hurt to stick with j, as we always assign it a value at top of loop. Switching to k made me wonder if variable value needs to survive until some subsequent iteration.

Renaming deal to swapArrays made sense, thank you.

Comment for generateDiskMoves is very nice. Except I'd delete that "m is discarded" remark, as that's not part of the public API.

Personally I view the comment for M .+ 1 as "% Convert to one-origin moves."

The "Get only the first move" comment is accurate and helpful, but consider something stronger: "% The trivial case requires just one move."

Omega2 accepts lowercase n. Consider using lowercase in the other functions. I had been thinking of upper as matrix and lower as scalar.

I wouldn't mind seeing a comment that spells out whether disk 1 is smallest or biggest disk.

As far as the algorithm goes, if results of a parity function were available, could you verify, or synthesize, the Omega2 results? Perhaps with less looping?


Laravel model and controller interaction I want to know if I'm going about creating and calling two functions from my model to my controller in the simplest and cleanest way.

Model:

public function getPosts()
{
    $post = $this->paginate(4);
    return $post;
}

public function getMonth($post)
{
    $post->month = date('M', strtotime($this->created_at));
    $post->month = strtoupper($post->month);
    return $post->month;
}

public function getDay($post)
{
    $post->day = date('d', strtotime($this->created_at));
    return $post->day;
}

Controller:

public function index()
{
    $post = $this->post->getPosts();
    $post->month = $this->post->getMonth($post);
    $post->day = $this->post->getDay($post);

    return View::make('posts.index', compact('post'));
}

I am unsure about if my controller is acting in a strict MVC way, being that I thought it's only job is to direct traffic, but it's doing more by calling functions from my model. Is this the best way to go about this?

PHP is not my area of expertise, so just some generic notes:

4 is a magic number here:

$post = $this->paginate(4);

Why is it 4? What the purpose if this number? A named constant or local variable would be readable with a descriptive name.

public function getMonth($post)
{
    $post->month = date('M', strtotime($this->created_at));
    $post->month = strtoupper($post->month);
    return $post->month;
}

Using a local variable with a proper name, like lowercase_month, in the first line would be readable and more descriptive.
These methods violates Command Query Separation since they return some data and modify the $post too.

  Functions should either do something or answer something, but not both.

Source: Clean Code by Robert C. Martin, Chapter 3: Functions, Command Query Separation p45

$post->month = date('M', strtotime($this->created_at));
$post->month = strtoupper($post->month);

I'd consider moving these calls to the Post class since it seems data envy. (Pseudocode.)

class Post {
    ...
    public void setMonth($created_at) {
        $lowercase_month = date('M', strtotime($created_at));
        $post->month = strtoupper($lowercase_month);
    }
    ...
}


Exercise 1_2 multiple console outputs. I would like to reduce the number of console output statements Please could we examine the below code for anyway to put all console output in one statements?

 #include <iostream>
 using namespace std;

 int main( )
 {  //print("") in Python
   cout<<"Hello " "everybody!"<<endl;
   cout<<"My name is AK."<<endl;
   cout<<"Goodbye."<<endl;
   cout<<""<<endl;

   //Poem
   cout<<"Twinkle, twinkle, little bat!"<<endl;
   cout<<"How I wonder what you're at?"<<endl;
   cout<<"Up above the world you fly,"<<endl;
   cout<<"Like a tea-tray in the sky."<<endl;
   return 0;
}

Scope

This is really just limited to style and code formatting.

Two (almost) substantive points

You should get in the habbit of not:

using namespace std;

It can get you into name-clash issues later, as you progress.

You should be aware that std::endl flushes the stream buffer. This is often unnecessary, unwanted and can be slow if used in a tight loop. You should use '\n' for most situations. If you are worried about
platform specific line-ending, don't be. '\n' adapts, just like
std::endl.

Code formatting

Install clang-format or similar to help you with formatting.

All I did was to integrate your endl into the strings and hit "auto-clang-format" and I got this:

#include <iostream>

int main() {
  std::cout << "Hello everybody!\n"
            << "My name is AK.\n"
            << "Goodbye.\n"
            << "\n"

            // Poem
            << "Twinkle, twinkle, little bat!\n"
            << "How I wonder what you're at?\n"
            << "Up above the world you fly,\n"
            << "Like a tea-tray in the sky.\n";
  return 0;
}

Which is "good enough for me". Clang format is very tunable, and I have it configured to something which works for me, and my team, in 99% of cases. So we don't spend time fighting the formatting of the code.

The above style with "one streaming operator" << at the beginning of each line is what we use most of the time. It makes sense when you have literals interspersed with variables and/or function calls.

For this very specific (and rather atypical?) case, you could, as someone else pointed out, also just stream it all as one continuous literal. C++ allows you stop/start string literals like this:

#include <iostream>

int main() {
  std::cout << "Hello everybody!\n"
               "My name is AK.\n"
               "Goodbye.\n"
               "\n"

               // Poem
               "Twinkle, twinkle, little bat!\n"
               "How I wonder what you're at?\n"
               "Up above the world you fly,\n"
               "Like a tea-tray in the sky.\n";
  return 0;
}

Hope that helps.


Nested loop to render tiles in a grid This code works but I'm not really sure if I wrote it good enough. It seems a bit vague but I can't really assess it properly. I'm particularly concerned with the xAxis variable. I don't like how I have to initialize it inside the first loop. I would rather have it next to yAxis. Is this a problem?

float yAxis = SPACE_BETWEEN_TILES;
float xAxis;

for(int i = 0; i < 3; i++) {
    xAxis = SPACE_BETWEEN_TILES;
    for(int v = 0; v < 3; v++) {
        shapeRenderer.rect(xAxis, yAxis, 10, 10);
        xAxis = TILE_WIDTH_AND_HEIGHT + xAxis;
    }
    yAxis = yAxis + TILE_WIDTH_AND_HEIGHT;
}

I think it is mostly fine as it is. I would declare variables at the level they are actually needed, not before. Also, xAxis = TILE_WIDTH_AND_HEIGHT + xAxis; can be written as xAxis += TILE_WIDTH_AND_HEIGHT.

i and v are not very good variable names (too short, and they don't fit together). What does v even stand for? vertical? In that case, I would use vertical and horizontal.

TILE_WIDTH_AND_HEIGHT would be shorter as TILE_DIMENSION.

You could restructure it like this if you want to:

    for (int y = 0; y < 3; y++) {
        for (int x = 0; x < 3; x++) {
            shapeRenderer.rect(SPACE_BETWEEN_TILES + TILE_DIMENSION * x, SPACE_BETWEEN_TILES + TILE_DIMENSION * y, 10, 10);
        }
    }

The performance loss from transforming additions to multiplications should be fine in most cases.


VGA sync generator for 640x480@60Hz I wrote my first module in Verilog. The purpose is to maintain two counters and emit signals corresponding to VGA's HSync and VSync, as well as HBlank and VBlank pulses to be used by a video controller for the blanking intervals. The only input is a clock signal of 25.175 MHz. All timing information was acquired from here.

module sync (clk, hblank, hsync, vblank, vsync) ;
  input clk;
  output reg hblank = 0, hsync = 1, vblank = 0, vsync = 1;
  reg [9:0] hcnt = 0, vcnt = 0;

  always @(posedge clk) begin
    case (hcnt)
      640: hblank <= 1;
      656: hsync <= 0;
      752: hsync <= 1;
      800: begin
        hblank <= 0;
        hcnt <= 0;
        vcnt <= vcnt + 1;
      end
    endcase
    case (vcnt)
      480: vblank <= 1;
      490: vsync <= 0;
      492: vsync <= 1;
      525: begin
        vblank <= 0;
        vcnt <= 0;
      end
    endcase
    hcnt <= hcnt + 1;
  end
endmodule

If you had ran simulation or loaded onto FPGA, you would noticed you didn't get the expected behavior. Run simulation and look at waveforms before loading to FPGA.

You have hcnt <= hcnt + 1; at the bottom of your always block, this will override hcnt <= 0; which is not what you want. <= is a non-blocking assignment which means it will be evaluated immediately but the value will not be updated until the end of the timestep. Order matters.

The simplest solution is to move hcnt <= hcnt + 1; above the case statement.

Be aware that your hcnt are counting from 0 to 800 which is 801 clocks. You may want to consider subtracting 1 from each case condition or initiating/resetting hcnt (and vcnt) as 1 instead of 0.

Your vcnt will only be 525 for one clock, which don't look intentional. Consider moving it inside the case(hcnt)'s condition 800.

I will recommend adding a reset input. And also recommend using an ANSI header Non-ANSI is required for Verilog-95 and pre IEEE1364. Since Verilog-2001, ANSI style is preferred mostly because it reduces the amount of typing.

Below is my suggestion. Note I haven't tested that all functional requirements are met (that should be done in your testbench).

module sync (
  input clk, rst_n,                       // <-- ANSI header
  output reg hblank, hsync, vblank, vsync
);
  reg [9:0] hcnt, vcnt;

  always @(posedge clk) begin
    if (!rst_n) begin // <-- synchronous reset logic
      hblank <= 1'b0;
      hsync <= 1'b1;
      vblank <= 1'b0;
      vsync <= 1'b1;
      hcnt <= 10'h001;  // <-- init as 1 so case index doesn't need to change
      vcnt <= 10'h001;  // <-- same as vcnt
    end
    else begin
      hcnt <= hcnt + 10'h001; // <-- default assignment, will be updated after the clock
      case (hcnt) // <-- uses the sampled value, not the result of the above line
        10'd640: hblank <= 1'b1;
        10'd656: hsync <= 1'b0;
        10'd752: hsync <= 1'b1;
        10'd800: begin
          hblank <= 1'b0;
          hcnt <= 10'h001; // <-- reset as 1, last assignment wins
          vcnt <= vcnt + 10'h001;
          case (vcnt)
            10'd480: vblank <= 1'b1;
            10'd490: vsync <= 1'b0;
            10'd492: vsync <= 1'b1;
            10'd525: begin
              vblank <= 1'b0;
              vcnt <= 10'h001; // <-- reset as 1, last assignment wins
            end
          endcase
        end
      endcase
    end
  end
endmodule

You may want to consider using the 2-alway block coding style. It does require more lines of code small designs (usually reduces lines of code for large/complex desings). The main benefit is you can access to the present state and next state of a flop.

// sequential logic (uses non-blocking assignment and is synchronous)
always @(posedge clk) begin
  if (!rst_n) begin
    hblank <= 1'b0;
    hsync <= 1'b1;
    vblank <= 1'b0;
    vsync <= 1'b1;
    hcnt <= 10'h001;
    vcnt <= 10'h001;
  end
  else begin
    hblank <= next_hblank;
    hsync <= next_hsync;
    vblank <= next_vblank;
    vsync <= next_vsync;
    hcnt <= next_hcnt;
    vcnt <= next_vcnt;
  end
end

// combinational logic (uses blocking assignment and is asynchronous)
always @* begin
  next_hblank = hblank; // <-- default keep previous
  next_hsync = hsync;
  next_vblank = vblank;
  next_vsync = vsync;
  next_hcnt = hcnt + 10'h001; // <-- default increment
  next_vcnt = vcnt;

  // calc next values, update as needed
  case (hcnt)
    10'd640: next_hblank = 1'b1;
    10'd656: next_hsync = 1'b0;
    10'd752: next_hsync = 1'b1;
    10'd800: begin
      next_hblank = 1'b0;
      next_hcnt = 10'h001;
      next_vcnt = vcnt + 10'h001;
      case (vcnt)
        10'd480: next_vblank = 1'b1;
        10'd490: next_vsync = 1'b0;
        10'd492: next_vsync = 1'b1;
        10'd525: begin
          next_vblank = 1'b0;
          next_vcnt = 10'h001;
        end
      endcase
    end
  endcase
end


Checking if all the characters from an A-Z is present in the string A string is said to be complete if it contains all the characters from
  a to z. Given a string, check if it complete or not.

  Input First line of the input contains the number of strings N. It is
  followed by N lines each contains a single string.

  Output For each test case print "YES" if the string is complete, else
  print "NO"

  Constraints 1 <= N <= 10 The length of the string is at max 100 and
  the string contains only the characters a to z.

Here are the two solutions that I wrote:

class TestClass {
    private static byte N;
    public static void main(String args[] ) throws Exception {
        BufferedReader keyboard = new BufferedReader(new InputStreamReader(
                System.in));
       N = Byte.parseByte(keyboard.readLine());
        for (byte i = 0; i < N; i++) {
            String testingString = keyboard.readLine();
            isCompleteString(testingString);
        }
    }

    private static void isCompleteString(String testingString) {
        String resultString= "";
        char[] toChar = testingString.toCharArray();
        int length = toChar.length;
        for (int i = 0; i < length; i++) {
            if(resultString.indexOf(toChar[i])==-1){
                resultString=resultString.concat(toChar[i]+"");

            }

        }

        if(resultString.length()==26)System.out.println("YES");
        else System.out.println("NO");
    }

}

This took 1.6469 secs against various inputs, while this code took:

class TestClass {
    private static byte N;
    public static void main(String args[] ) throws Exception {
        BufferedReader keyboard = new BufferedReader(new InputStreamReader(
                System.in));
       N = Byte.parseByte(keyboard.readLine());
        for (byte i = 0; i < N; i++) {
            String testingString = keyboard.readLine();
            isCompleteString(testingString);
        }
    }

    private static void isCompleteString(String testingString) {

        char[] toChar = testingString.toCharArray();
        int length = toChar.length;
        boolean[] findingAlphabet = new boolean[26];
        for (int i = 0; i < length; i++) {
            byte alphabets = (byte) toChar[i];
            findingAlphabet[122-alphabets]=true;

        }
        int count=0;
        for(boolean b:findingAlphabet){
            if(b)
            count++;
        }
        if(count==26)System.out.println("YES");
        else System.out.println("NO");
    }

}

1.5818 secs against various inputs.

How can I optimize these programs further (I have used BitSet)? What are other possible optimized methods of doing the same?

private static byte N;

We know that N<=100, but this is a very premature optimization. In theory, you're saving 3 bytes total. Actually, it'll get rounded up to a nearest multiple of 8 or whatever and you save nothing at all.
It's also no good idea top make everything static.
BufferedReader keyboard = new BufferedReader(new InputStreamReader(
            System.in));

But the input does not come from the keyboard. Or can you type 100 lines in two seconds? I'd call it in as I'm lazy, something like reader would be fine.
isCompleteString(testingString);

According to the name, isCompleteString returns boolean. And there should be a method doing just this. Coupling output with computation makes you method 100% non-reusable.
First version
    String resultString= "";

Switch on your warning to see unused variables.
    char[] toChar = testingString.toCharArray();
    int length = toChar.length;
    for (int i = 0; i < length; i++) {

This looks like a premature optimization. And toChar is a bad name as there's no char but char[]. The IDE proposes charArrays or chars, which is both better.
If you want to use a char[], then use simply
   for (char c : chars)

This is clearly non-optimal
        if(resultString.indexOf(toChar[i])==-1){
            resultString=resultString.concat(toChar[i]+"");
        }

for many reasons

indexOf - you're running through the string for each char
toChar[i]+"" creates a String from each char needlessly
resultString.concat(...) creates a new String every time

You could use a StringBuilder here.
But creating a String only to measure it's length is plain wrong. You could count the chars instead.
But counting the chars is plain wrong too. If you want to know if a beer crate is full, do you count the bottles? Or do you if there's a bottle missing?

    if(resultString.length()==26)System.out.println("YES");
    else System.out.println("NO");

This should be written as
   System.out.println(resultString.length()==26 ? "YES" : "NO");

apart from that 1. there should be no resultString, 2. there should be a method computing and a method printing.
Second version
    boolean[] findingAlphabet = new boolean[26];

No good name. What about foundChars or simply found?
        byte alphabets = (byte) toChar[i];

What byte? Never use a byte, unless either it's exactly what's needed or you have a big bunch of them.
        findingAlphabet[122-alphabets]=true;

Nice obfuscation. What about
       found[chars[i] - 'a'] = true;

? And bottle counting again
    int count=0;
    for(boolean b:findingAlphabet){
        if(b)
        count++;
    }

This should be
    for (boolean b : found) {
        if (!b) {
            return false;
        }
    }
    return true;

How can I optimaize these programs further (I have used BitSet)?

You haven't. But BitSet is slower than a boolean[] (except when memory locality comes in play).
It looks like most of the time gets spend on IO. Not surprising, even the first solution with doing 26 times indexOf on a 100 chars string is not computationally intensive.
It could be sped up by avoiding strings and working with the input bytes directly. But I'd concentrate on readability first, otherwise it'll become a huge mess.
You may want to specify input encoding as US-ASCII as it's sufficient and much faster than UTF-8 which may be your platform default.


Fahrenheit / Celsius temperature converter This is a simple bash script (hopefully with a little style as well) for converting temperatures from Fahrenheit to Celsius and vice versa.

#!/bin/bash

re='^-*[0-9]+([.][0-9]+)?$'
while true
do
    printf "\n\e[1;39mWhat is the temperature you wish to convert?\e[0m\n\n\t"
    read -p "Enter a numeric value  " n
    while [[ ! $n =~ $re ]]
    do
        printf '\t\e[3;3;31mInvalid selection\n\t\e[3;3;31m\e[0m\n\t'
        read -p "Enter a numeric value " n

    done
    echo
    printf "\n\tIs this \e[1;39m(F)\e[0mahrenheit or \e[1;39m(C)\e[0melsius\e[0m?\n\n"
    while true
    do
        read -sn 1 cf
        case $cf in
            [Ff] ) echo "$n""˚F = $(( ($n - 32) * 5 / 9 ))˚C"; break;;
            [cC] ) echo "$n""˚C = $(( $n * 9 / 5 + 32 ))˚F"; break;;
            * ) printf '\t\e[3;3;31mInvalid selection\n\e[3;3;31m\e[0m'
            printf '\tChoose \e[1;39m(F)\e[0mahrenheit or \e[1;39m(C)\e[0melsius\e[0m\n\n'
        esac
    done
    echo
    while true
    do
        printf "\tPress (1) to convert another\n\tpress (2) to quit"
        read -sn1 bye ; printf "\n\n"
        case $bye in
            [1] )
            break
            ;;
            [2] ) printf "Now exiting" && exit
            ;;
            * )
            printf '\t\e[3;3;31mInvalid selection\n\e[3;3;31m\e[0m'
            ;;
        esac
    done
done

Is there anything I could do to improve upon this? I am also curious if this would be portable / POSIX compliant. Everyone always says to use printf if portability is a concern. Is echo not portable?

I don't know enough bash to offer you a technical review.

But, more philosophically: Have you considered writing this more in the style of a "program" than a "Press 1 to convert another"-type interface? Like:

~$ temperature c 30
86 ˚F
~$ temperature F 105
40 ˚C

My reasoning is that the above is easier and more flexible to use programatically, you don't need to write input loops at all and you can condense down to your core functionality:

#!/bin/bash

case $1 in
    [Ff] ) echo "$(( ($2 - 32) * 5 / 9 )) ˚C"; break;;
    [cC] ) echo "$(( $2 * 9 / 5 + 32 )) ˚F"; break;;
    *    ) echo "Usage: $0 c|f num"
esac


Function to filter comments and ratings, with pagination support I have a web component that is rendering a list of items and I have this method for filtering these items based in a filterBy object that is being updated by other events and stored in the same component class.

filterBy = {
    comment: '', // string
    rating: [] // array of numbers
}

filterFeedback(items) {
    let filteredItems = [];
    const pagination = { offset: 0, limit: 10, totalCount: items.length };
    const { comment, rating } = this.filterBy;

    const filteringFn = (condition) => {
        items.map((item) => {
            if (filteredItems.length < pagination.limit) {
                if (condition(item)) {
                    filteredItems.push(item);
                }
            } else {
                return;
            }
        });
    }
    if (comment && (!rating || rating.length === 0) && comment !== '') {
        filteringFn((item) => item.comment && item.comment.includes(comment));
    } else if (!comment && rating && rating.length > 0) {
        filteringFn((item) => rating.some((value) => value === item.rating));
    } else if ((comment && comment !== '' )&& rating && rating.length > 0) {
        filteringFn((item) => item.comment && item.comment.includes(comment) && rating.some((value) => value === item.rating));
    } else {
        filteredItems = items ? items.filter((item, i) => i < pagination.limit) : [];
    }

    return filteredItems;
}

I was analyzing this code and trying to make it more clear and readable. I think the filteringFn saves me a lot of code duplication but still, I found the conditionals unreadable. Besides, it's not scalable in case I'd like to add more filters.

If one of your filters is undefined, just set it to always succeed.

If one of your parameters is undefined, set it to something so that you don't have to check it over and over.

Truncate output to pagesize at the end.

filterFeedback(items) {
    const pagination = { offset: 0, limit: 10, totalCount: items.length };
    const filtRatings=new Set(
        ( this.filterBy.rating && this.filterBy.rating.length ) ?
          this.filterBy.rating : items.map( item => item.rating )
    );
    const filtComment=this.filterBy.comment || '';

    return items.filter( item =>
        ( item.comment || '' ).includes(filtComment)
        &&
        filtRatings.has(item.rating)
    )
    .slice(0,pagination.limit);

}


C string library inspired by Python string functions I'm working on a C library which attempts to bring some of the Python string functions over to C.

This is where my full source is located, but the majority of what I have so far is here.

Are there any places where code efficiency can be improved?

#ifndef OCTO_H
#define OCTO_H

#include <stdlib.h>
#include <string.h>

int len(char* string){

    int i = 0;
    while(*string != '\0'){
        i++;
        *string++;
    }

    return i;

}

char* lstrip(char* string){

    if(!string)
        return;

    /* Trim off leading whitespace */
    while(*string == ' '){
        string++;
    }

    return string;

}

char* rstrip(char* string){

    /* Trim off trailing whitespace */
    char* end = string + len(string) - 1;
        while (end >= string && *end == ' ')
                end--;
        *(end + 1) = '\0';

    return string;

}

char* strip(char* string){

    /* Trim off both leading and trailing whitespace */
    char* lstring = lstrip(string);
    char* finalString = rstrip(lstring);

    return finalString;

}

char* slice(char* s, int start, int end){

    /* Create a new identical buffer */
    char* buff = (char*) malloc((end - start) + 2);
    strncpy(buff, s + start, (end - start) + 1);
    *(buff + (end - start) + 1) = '\0';
    return buff;
}

char* toUpperCase(char* s){

}

char* toLowerCase(char* s){

}

#endif /* OCTO_H */

Well efficiency can be improved by using the built-in string functions.
They have been optimized at the assembly level for most implementations.

Assuming you were writing len() in C rather than using some optimal assemble instructions. The canonical form looks more like this (though I am sure it can be done better). Though there is strlen() which does the same thing.

int len(char* string)
{
    char *end = string;
    for(;*end;++end)  /* Notice nothing here */;
    return end-string;
}

Looking at this:

char* lstrip(char* string){

    /* This makes it non optimal for strings we already know are good.
     * If I already know my string is fine I would want a function that
     * did not do this test.
     *
     * Of course you can then have a second function that does the test
     * then calls the non checked version (and if it is short the compiler
     * will inline
     */
    if(!string)
        /* Not returning a value from a function expecting a value is an error
         * Check the warning messages generated by the compiler
         */
        return;

    /*
     * ' ' is not white space it is just a space character
     *    What you are looking for is isspace(c)
     */
    while(*string == ' '){
        string++;
    }

    return string;
}

I would have written like this:

char* lstripNT(char* string){ return string?lstrip(string):NULL;}
char* lstrip(char* string){
    for(;isspace(string);++string) /*Nothing here*/;
    return string;

    // NOTE: there is an issue with this technique in that you
    //       are not returning the original string.
    //       If the string was dynamically allocated
    //       you now have issues with freeing it. Which means you
    //       specifically need to keep a copy of the original pointer
    //       for the purpose of freeing it when you are finished.
    //       This is NOT a good idea.
}

Because of the problem with dynamic allocation I would probably write like this:

char* lstrip(char* string)
{
    size_t  move = 0;
    char*   start= string;
    for(;isspace(start);++start,++move)
    {}
    if (move)
    {
        // Move all the characters down.
        // So we basically squish the white space.
        for(;*start;++start)
        {
            *(start-move) = *start;
        }
    }
    return string;
}

Right trim. Is not really optimal because you scan to the end (using len). Then you then scan backwards until you find a non space character. You can do this in a single traverse. Also you are not being consistent. lstrip() checks for NULL while this does not.

char* rstripNT(char* string){ return string?rstrip(string):NULL;}
char* rstrip(char* string){

      char* loop    = string;
      char* last    = NULL;
      int   isSpace = false;
      while(*loop)
      {
          isSpace = TRUE;
          last    = loop;
          for(;*loop && isspace(*loop);++loop)
          {}
          if (*loop)
          {
              isSpace = FALSE;
              for(;*loop && !isspace(*loop);++loop)
              {}
          }
      }
      if (isSpace)
      {
          *last = '\0';
      }
      return string;
}

The strip.

char* strip(char* string){

    /* Trim off both leading and trailing whitespace */
    char* lstring = lstrip(string);
    char* finalString = rstrip(lstring);

    return finalString;

}

This is conceptually what you want to do.
But I don't think that will be optimal way to do it. I would write this as a single pass.


Finding the Pythagorean triplet that sums to 1000 Project Euler problem 9 says:

  A Pythagorean triplet is a set of three natural numbers, a < b < c,
  for which,

  a2 + b2 = c2

  For example, 32 + 42 = 9 + 16 = 25 = 52.

  There exists exactly one Pythagorean triplet for which a + b + c =
  1000. Find the product abc.

I am a beginner in programming, especially Python. I got the answer for the problem, but how can I optimize it?

a=0
b=a+1
exit=False

#a loop
for x in xrange(1000):
    a+=1
    b=a+1
    c=b+1
    #b loop
    for y in xrange(1000):
        b+=1
        if(1000-b-a < 0):
            break
        c = 1000-a-b
        if(a**2 + b**2 == c**2):
                print "The number is ", a*b*c
                x=1000
                exit = True
        else:
                print "A is ", a ,".\n B is ", b, ".\n  C is ",c,"."
        if exit:
                break
    if exit:
            break

Here are some changes I would do in your code:

Since you already have two variables x and y from the loop, you don't need a and b.  Get the 3 numbers as - x, y, and 1000 - x - y, and use them.
You might well possibly be checking the values same pythagorean triplets twice. Once with x, y, and 1000 - x - y, and then with y, x, 1000 - y - x. For example, when x takes 2 and y takes 3, then you don't want to check this again for x = 3 and y = 2. This might happen, as your inner loop is running from the beginning every time. You can avoid this by iterating from x to 1000.
No need of exit boolean variable. Put those loops inside some function, and return as soon as the result is found.

Modified Code:

def specialPythagoreanTriplet(s):

    for a in xrange(s):
        for b in xrange(a, s):
            c = s - a - b;
            if a**2 + b**2 = c**2:
                return (a, b, c)

In addition to all that, you don't really need to iterate till 1000 for values of a and b. The maximum value that a could take should be much less than 1000. Let's find out.

You have:

  a + b + c = 1000, and
  a < b < c

Replace b and c with a, we get:

  a + a + a < 1000
  3a < 1000
  a < 333

Similarly, you can find out the threshold for b, I'm leaving it for you:

for a in xrange(s / 3):
    for b in xrange(a + 1, s):  # Change `s` to someother value
        # Same as previous code.


Absolute-difference function for std::size_t Because std doesn't provide an overloaded version of std::abs for std::size_t, I built a version to calculate distance between unsigned numbers.

It subtracts the smaller one from the larger one, to prevent a negative result, since in unsigned integer it would wrap around.

std::size_t size_type_abs(std::size_t a, std::size_t b)
{
    return std::max(a, b) - std::min(a, b);
}

Is this implementation correct and efficient?

This function computes absolute difference, not absolute value, so it shouldn't be called abs. absolute_difference? size_diff?

What will you use this function for? Are you using size_t for something other than sizes, so an absolute difference is meaningful?

std::max(a, b) - std::min(a, b) is harder to read than a simple conditional:

return a > b ? a - b : b - a;

This function isn't specific to size_t; it can be defined for any type that supports comparison and subtraction:

template<typename T>
T abs_diff(T a, T b) {
  return a > b ? a - b : b - a;
}

If you do want to define it only for size_t, using std::size_t would save some repetitions of std::.


Format RGB Long as hex string in VB6 This is how I convert e.g. RGB(255, 0, 0) (where red is stored in the least significant byte of a Long) to a 6-digit hex string FF0000:

Function RgbToHex(l As Long) As String
Dim r As Long
Dim g As Long
Dim b As Long
    r = l And ColorConstants.vbRed
    g = (l And ColorConstants.vbGreen) / (2 ^ 8)
    b = (l And ColorConstants.vbBlue) / (2 ^ 16)
    RgbToHex = Right$("0" & Hex(r), 2) _
             & Right$("0" & Hex(g), 2) _
             & Right$("0" & Hex(b), 2)
End Function

I'm a bit worried about the string manipulations, but as VB6 lacks printf-like formatting, this is the best I could come up with. Can it be improved?

The code you have works, and is very nicely-formatted.

There are only two things I would change, stylistically speaking:

Explicitly pass input parameters by value. This should have been the default, but it wasn't in classic COM-based VB, so you need to write it out. ByRef is actually a perverse choice for the default, since it is almost never what you want. It is certainly not what you want for primitive types like Long. (But it probably is what you want for strings, unless passing them out-of-process, e.g. in a COM server.)
As others have pointed out, l is a rather poor choice for a parameter name. Roland says one problem with it is that it can be difficult to distinguish from the digit 1 or a capital I. I'd say that's more a reflection on your choice of a font for your editor, but this is still a valid concern. My major issue with it is that it looks like a lame attempt at Systems Hungarian notation. I know this used to be all the rage in VB programming, but that was mostly cargo-cult. Unless you're using variants (and you probably shouldn't be) the language is sufficiently strongly-typed that embedding type information in the name of the variable serves little point. The only type prefixes I allow myself are for names of controls (mostly because it makes finding them in IntelliSense significantly easier; I always remember the type of control much more readily than I remember its name) and "member" prefixes (e.g., m_). Aside from that, the only prefixes you should be using are meaningful semantic ones, in the Apps Hungarian vein.

Therefore, I would rewrite your function's prototype to look like the following:

Function RgbToHex(ByVal color As Long) As String

Otherwise, the primary issue with your code is that it is slow!

Why? Well, there are a couple of reasons:

String operations are extremely slow in VB since they all require multiple BSTR allocations and deallocations, not to mention the fact that the calls to the string-manipulation functions themselves cannot be inlined.

Now, I notice that you did correctly force the use of the string-based Right function by using the $ type suffix, which is a standard trick to ensure as optimal code as possible. This is good. You didn't do the same thing for Hex. Personally, I have no idea whether it is actually necessary or not—is there a variant-based version of Hex? Maybe, I don't know. And I don't want to have to worry about it when I write or review code, so I always use the $ type suffix when I am working with strings. I suggest that you do the same. (Mat's Mug says Hex does have a variant version, so looks like this is sage advice after all!)

But the major problem here is that you're calling six different string-manipulation functions, plus concatenating those three intermediate strings. Slooooow! To optimize the code, it will be critical that we find a way to keep these calls to a minimum.
You are using floating-point operations in your byte-extraction code.

In your defense, constant expressions like 2 ^ 8 should be computed by the compiler at compile-time, so that your code never actually has to do an exponentiation operation. I would have made exactly that same assumption. Unfortunately, the VB 6 compiler (even with all optimizations enabled) is not that smart. These two exponentiation operations are done at run-time, with a call to the vbaPower function exported by the run-time library. Personally, I'd say this is a bug in the compiler, but it's not going to get fixed now, and you have no choice but to work around it.

The other bad news is, once you've got a floating-point value, the entire arithmetic operation has to be done in floating-point mode. So for each of your bit-extraction operations, the compiler is going to emit code that converts an integer value into a floating-point value, loads floating-point constants, performs an exponentiation operation, fixes up the result, and then does a division.

While it is true that floating-point operations are no longer as slow on modern machines as they used to be, generally obsoleting the classic optimization advice to avoid floating-point operations whenever possible, VB 6 is not sufficiently modern for that advice to hold. :-) There is no way to get the VB 6 compiler to emit SSE instructions, so you're stuck with the legacy x87 FPU, and that means that you're stuck with a bottleneck intrinsic in its design, arising from the inability to move values directly from the floating-point stack into integer registers and vice versa. The compiler emits code to handle all of this for you seamlessly, but it's still slow. So, at least in the context of VB 6, it's still valid optimization wisdom not to inter-mix floating-point and integer operations.

In this case, though, you don't need floating-point operations at all! You could have done everything in integer mode, just by explicitly using an integer division:

r = (l And ColorConstants.vbRed)
g = (l And ColorConstants.vbGreen) \ (2 ^ 8)
b = (l And ColorConstants.vbBlue)  \ (2 ^ 16)

Then, the constants even get folded correctly, with the exponentiation being done at compile-time.

As originally written, your function compiles to hundreds of lines of machine code. I made an optimized build and then disassembled it, revealing a total of over 1,000 bytes of machine code just for that one function. It contains tons of embedded function calls (slow), string allocations/deallocations (slow), and extremely sub-optimal code to store, compare, and manipulate floating-point values (slow).

Let's have a go at optimizing it.

Comintern made one attempt already, and it is interesting to study. Immediately one problem jumps out—it does not produce correct results. Specifically, the order of the red and blue component values are reversed, such that instead of AABBCC, his code gives CCBBAA. On little-endian architectures, like the x86, integer values are stored in memory with the least-significant byte at the smallest address. This means that "RGB" colors are actually represented in memory using an BBGGRR format, whereas you are wanting the standard, web-style RRGGBB format for your hexadecimal output. This is a minor bug, though, and should be fixable.

My real issue with his code is that it makes a series of function calls, specifically to the RtlMoveMemory API function, which he has aliased to CopyMemory. I've seen this technique before for optimizing code in VB 6, and while it has power, it's not a panacea. Why? Function calls are expensive, too! Memory copying is a relatively cheap operation, O(n) at the worst, and has a small constant multiplier assuming an optimal implementation (which RtlMoveMemory does provide), but the context switching required for a function call introduces a lot of overhead that it would be nice to avoid. This is especially important in a very small, do-one-simple-thing function like you have here. VB 6's compiler does a very poor job of inlining code, so the execution cost of this function would be dwarfed by the overhead cost of making these system calls.

To be fair, Comintern's code is still going to be significantly faster than your original attempt, but we can do even better.

How? By using bit-manipulation tricks to do the byte-swapping. Basically, you do what you would have done in C (or at least, what I would have done in C, maybe you're a real VB programmer and don't think in the language of blasphemy). This is basically what Roland Illig did in his code. You start with the input value, in AABBGGRR format. Then, you isolate each of the individual color components (except the alpha channel, which we don't care about):

The red component is trivial: you just mask off all but the bottom-most byte. Bitwise masking is done with a bitwise And operation.
For the green component, you first divide by &H100 (which is the same as a right-shift by 8, except that you can't do a right-shift in VB). That throws away the bottom-most byte, shifting everything down by one byte. Then you can simply mask off all but the bottom-most byte, which is the one that used to be the middle byte.
The same thing is true for the red component, except that we divide by a larger constant, &H10000, to achieve what is effectively a right-shift by 16.

My hope/expectation was that the VB 6 compiler would actually optimize these division operations to right-shift operations, since that will be faster—and indeed, that is exactly what it did! Notice that it is critical that we use the integer-multiply operator, \, here.

The next step is to take those isolated byte values, and pack them together in the proper order. Again, Roland's code does the same thing I would have done: uses a multiplication in lieu of a left-shift operator. Unfortunately, VB 6's compiler can't/won't optimize this multiplication into a left-shift. Fortunately, multiplications are not slow on modern processors, so this is not a big loss.

Finally, we want to make a single call each string-formatting function to pretty-print the final output. Roland's idea was to multiply the integer value by &H1000000 to ensure that it is properly zero-padded—very clever! I did it a bit differently: I decided to check and see if padding is actually necessary, and if not, skipping the call to Right altogether!

Public Function RgbToHex(ByVal color As Long) As String
    ' On little-endian architectures, "RGB" color values are actually stored in memory
    ' in an AABBGGRR format. However, web-style hex colors are always formatted in an
    ' AARRGGBB format. Neither Windows nor HTML care about the alpha-channel value
    ' (always the upper, most-significant bit), so we are going to simply ignore it.
    ' Otherwise, we have to do some bit-twiddling to swap the red and blue color channels.

    ' Extract the individual red, green, and blue color channels.
    ' (Although they are only bytes, continue to store these intermediate values
    ' as 32-bit integers for maximum speed.)
    Dim r As Long
    Dim g As Long
    Dim b As Long
    r = (color And &HFF)
    g = ((color \ &H100) And &HFF)
    b = ((color \ &H10000) And &HFF)

    ' Now, arrange these byte values in the correct order (RRGGBB).
    color = ((r * &H10000) + (g * &H100) + b)

    ' And finally, format it as a hexadecimal string, left-padding with zeros as necessary.
    ' Note that if the red component value is large enough, padding does not need to be
    ' done, which allows us to skip an expensive string operation.
    If (r >= &H10) Then
        RgbToHex = Hex$(color)
    Else
        RgbToHex = Right$("00000" & Hex$(color), 6)
    End If
End Function

If this were C, I would guess that Roland's solution would probably be faster, since it does not involve a conditional branch (and conditional branches can be expensive when they are not properly predicted). However, since this is VB, I suspect that my code will be slightly faster, mainly because string operations are so incredibly expensive that it is worth a mispredicted branch penalty to elide one if at all possible. And, if I may compliment myself, I'd say that my solution is slightly more readable from a code-review perspective. (But either solution would require explicit comments describing the rationale.)

Notice, also, that in my implementation, I have a comment about choosing to keep the intermediate values in a 32-bit Long, even though they are just bytes and could be represented using the Byte type. The reason for this is that it is inherently faster to work with 32-bit values on 32-bit architectures, since they fit directly into the processor's native 32-bit wide registers. (This is why you should always use Long in VB 6, rather than the 16-bit Short type. You pay an additional penalty when working with 16-bit values!) If we'd tried to store them as Bytes, they'd take up less memory, but the realistic limitations of VB 6's optimizer mean that there will be some unnecessary conversion going on. If VB supported unsigned types, using them would open up even more potential for the optimizer to do its magic, especially when doing bit-twiddling operations; alas, it doesn't, so we're stuck with signed types.

Can we make it even faster?

One quick and easy way to speed up the code is to change the compiler flags. Too few people do this in VB 6, since it is so well hidden. When building your EXE (File → Make *.exe), click the Options button to bring up a dialog. Switch to the "Compile" tab. "Compile to Native Code" is selected by default, and so is "Optimize for Fast Code", which are both good for speed. However, you should also turn on "Favor Pentium Pro" and "Create Symbolic Debug Info".

Creating debug info is just a good idea, in case you ever have to debug your application, and it is impossible to generate it later. This is arguably less useful for a strictly VB programmer who wouldn't know how to use another debugger with a gun to his head, but it is a skill that is truly worth learning, and if you ever have to call in an expert, a PDB file is invaluable. It doesn't hurt anything to generate it and ignore it.

Why favor the Pentium Pro? Granted, this sounds like a hyper-specific option to favor a certain obsolete family of microprocessor, and indeed it was at the time that VB 5 was released, but it is actually the best general choice because all modern processors bear far more similarity to the Pentium Pro than they do to other generations of processors (with the exception of the Pentium 4, but VB 6 can't optimize for that anyway, and optimizing for PPro is still not a pessimization). The Pentium Pro was a unique processor in a number of ways, but two things were most important from an optimization sense. First, it supported out-of-order execution, where individual instructions could be dynamically re-ordered by the processor to overcome instruction-level data dependencies. Thus, it is better if the compiler generates shorter sequences of instructions (RISC-style), because these can be better reordered. Second, the Pentium Pro has a stall for certain partial-register accesses. The details are complicated and beyond the scope of this answer, but the important fact is that while the earlier Pentium didn't have these, all modern processors do, so having the optimizer generate code accordingly is a good idea.

Other important checkboxes are buried behind the "Advanced Optimizations" button. The UI tries to make these sound scary, but if you understand what they do, and your code has been truly and correctly debugged, some of them are worth enabling. Most obviously, you can turn on the bottom option, "Remove Safe Pentium FDIV Checks". The early Pentium processors with the infamous FDIV bug are long dead now, so this is just pointless baggage to carry around. The other options actually can be slightly unsafe, but it is worth considering turning on the "Remove Integer Overflow Checks" option, and possibly the "Remove Floating Point Error Checks" and "Allow Unrounded Floating Point Operations" options. These three options have a massive impact on the object code that is generated by the compiler, and enabling them can cause a correspondingly massive speed-up in the execution time of your code.

As an example, when I threw the "Remove Integer Overflow Checks" switch and recompiled my code, I saw that it had used shift instructions, instead of multiplication instructions, like I earlier wished that it had! Better yet, removing these overflow checks means that the compiler no longer emits conditional branches after each integer (or floating-point) operation, and even though these branches will be properly predicted to fall through most of the time, leaving them out entirely is still faster. This is a cheap way to make your code run a lot faster, without having to rewrite any of it.

For the hardcore among you, here is my annotated disassembly for the final code generated by the VB 6 compiler with optimizations enabled. You should be able to see some of what I'm talking about in terms of the rather-optimal bit-twiddling instructions, as well as the less-than-optimal string-manipulation code. But being completely overwhelmed is also normal.


SQL database for a social network I'm trying to create a mini social network mobile application kind of like Instagram and I'm not sure about my database organisation, it feels like something is missing, especially with the Posts table.

CREATE TABLE Users(
'user_id' int(20) NOT NULL AUTO_INCREMENT,
'email' VARCHAR(255) NOT NULL,
'username' VARCHAR(25) NOT NULL,
'password' VARCHAR(255) NOT NULL,
'first_name' VARCHAR(255) NOT NULL,
'last_name' VARCHAR(255) NOT NULL,
'gender' ENUM('M', 'F'),
'city' VARCHAR(255),
'state' VARCHAR(255),
'country' VARCHAR(255),
'profile_picture_url' VARCHAR(255),
'birth_date' VARCHAR(255),
'date_created' DATE NOT NULL,
'date_updated' DATE,
'active' ENUM('yes','no') NOT NULL DEFAULT 'no',
'activation_key' VARCHAR(255) NOT NULL,
PRIMARY KEY ('user_id'),
UNIQUE KEY 'email' ('email'),
UNIQUE KEY 'username' ('username')
);

CREATE TABLE Posts(
'post_id' int(20) NOT NULL AUTO_INCREMENT,
'user_id' int(20) NOT NULL
'caption'  VARCHAR(255),
'latitude' FLOAT NOT NULL,
'longitude' FLOAT NOT NULL,
'type' ENUM('image', 'video'),
'post_url' VARCHAR(255) NOT NULL,
'date_created' DATE NOT NULL,
'date_updated' DATE,
PRIMARY KEY ('post_id'),
FOREIGN KEY ('user_id') REFERENCES Users('user_id')
);

CREATE TABLE Followings(
'user_id' INT NOT NULL,
'following_id' INT NOT NULL,
'date_created' DATE NOT NULL,
PRIMARY KEY ('user_id', 'following_id'),
UNIQUE INDEX ('following_id', 'user_id')
FOREIGN KEY ('user_id') REFERENCES Users('user_id')
FOREIGN KEY ('following_id') REFERENCES Users('user_id')
);

CREATE TABLE Comments(
'comment_id' INT(20) NOT NULL AUTO_INCREMENT,
'post_id' INT(20) NOT NULL,
'user_id' INT(20) NOT NULL,
'content' TEXT NOT NULL,
'date_created' DATE NOT NULL,
'date_updated' DATE,
PRIMARY KEY ('comment_id'),
FOREIGN KEY ('post_id') REFERENCES Posts('post_id')
FOREIGN KEY ('user_id') REFERENCES Users('user_id')
);

CREATE TABLE Messages(
'message_id' INT(20) NOT NULL AUTO_INCREMENT,
'user_id_from' INT(20) NOT NULL,
'user_id_to' INT(20) NOT NULL,
'content' text NOT NULL,
'date_created' date NOT NULL,
PRIMARY KEY ('message_id'),
FOREIGN KEY ('user_id_from') REFERENCES Users('user_id'),
FOREIGN KEY ('user_id_to') REFERENCES Users('user_id')
);

CREATE TABLE Likes(
'user_id' INT(20) NOT NULL AUTO_INCREMENT,
'post_id' int(20) NOT NULL,
'date_created' DATE NOT NULL,
PRIMARY KEY ('user_id', 'post_id'),
UNIQUE INDEX ('post_id', 'user_id'),
FOREIGN KEY ('post_id') REFERENCES Posts('post_id'),
FOREIGN KEY ('user_id') REFERENCES User('user_id')
);

Regarding the Posts table, would you recommend that I split it for each type of post, like one for the pictures and another one for the videos?

All the pictures/videos will be hosted on the AWS and therefore not on my server.

What would you recommend I change with these tables that I currently have, based on your experience? What tables should I implement?

Generally speaking, the schema looks good and well organized, however a few columns could use some refactoring. Here is what I can point to.

Users table

  'username' VARCHAR(25) NOT NULL,

This seems a bit small for a user name. While I'm pretty sure most users will choose names below 25 characters, I would expand it a bit just for the sake of safety. I think 50 or so will do.

  'password' VARCHAR(255) NOT NULL,

This smells at first sight. Maybe I'm wrong, but this sounds as a plain-text password, which is utterly wrong. Column size should match the output size of the used hash function, plus the size of the salt and hash parameters (or split those into separate columns). And don't even think of SHA1 or MD5 :P Use PBKDF2, bcrypt or scrypt for this. Look here for a more detailed discussion of password hashing.

  'first_name' VARCHAR(255) NOT NULL,
  'last_name' VARCHAR(255) NOT NULL,

This design is very specific for most occidental names of people, but wrong in the general case. People's names in many part of the world don't fit into first/last name, and for the most part a single, longer column will do. Read this article for more details about this.

Additionally, do you really need the real name of the user? Doesn't the username suffices? Or the real name is to be displayed somewhere?
All in all, I would put it as

'real_name' VARCHAR(1000) NOT NULL,

  'gender' ENUM('M', 'F'),

In many case, and specially in a social network environment (and more if you will allow search by gender), that restriction is not enough, as human gender identity goes far beyond male/female. Just for the record, New York recognizes 31 and Facebook 71, so I think your DB should at least take that into consideration.

What I would do is separate the possible genders into its own table (that you can fill separately) and put a foreign key in users:

CREATE TABLE Genders (
'gender_id' INT(20) NOT NULL AUTO_INCREMENT,
'name' VARCHAR(100) NOT NULL,
PRIMARY KEY ('gender_id'),
UNIQUE KEY ('name')
);

And in users:

'gender_id' INT(20),
FOREIGN KEY ('gender_id') REFERENCES Genders('gender_id')

  'city' VARCHAR(255),
  'state' VARCHAR(255),
  'country' VARCHAR(255),

Generally, this is a bad practice, as it would lead to repeated and inconsistent data, and difficult searches by city/country (as anyone can write it the way he wants). A better way is to normalize this into separate tables for cities/states/countries, and leave a reference to the city in the users table (and in turn, a city implies a state and a country):

CREATE TABLE Countries (
'country_id' INT(20) NOT NULL AUTO_INCREMENT,
'name' VARCHAR(100) NOT NULL,
PRIMARY KEY ('country_id'),
UNIQUE KEY ('name')
);

CREATE TABLE States (
'state_id' INT(20) NOT NULL AUTO_INCREMENT,
'name' VARCHAR(100) NOT NULL,
'country_id' INT(20) NOT NULL,
PRIMARY KEY ('state_id'),
UNIQUE KEY ('name','country_id')
);

CREATE TABLE Cities (
'city_id' INT(20) NOT NULL AUTO_INCREMENT,
'name' VARCHAR(100) NOT NULL,
'state_id' INT(20) NOT NULL,
PRIMARY KEY ('city_id'),
UNIQUE KEY ('name','state_id')
);

Those should be populated by you, then the users just select one of the options, and that allows proper searches. In users you only have a reference to city:

'city_id' INT(20) NOT NULL,
FOREIGN KEY ('city_id') REFERENCES Cities('city_id')

  'birth_date' VARCHAR(255),

As stated in the comments, this should be of type DATE.

  'active' ENUM('yes','no') NOT NULL DEFAULT 'no',

An enum is not the most optimal data type here. It's better to use BIT instead.

Table Posts

Generally, the layout looks good, I would not split it into images/videos, at least for now. This is due both storing basically the very same data, with only the file type differing. Only consider such a split if each type requires different columns. Other than that, I have this few tips about this table:

First of all, I would add a comment column so that users can write a bit about the photo/video they just posted (in addition to the simple caption).

'comment' TEXT NULL

  'post_url' VARCHAR(255) NOT NULL,

Is this really necessary? Why store the full URL when you can deduce it from other columns? Specifically, the id, or better, the caption, can be used to build unique URLs for each post without needing to generate a new piece of data.

Table Followings

  PRIMARY KEY ('user_id', 'following_id'),
  UNIQUE INDEX ('following_id', 'user_id')

This is redundant. The primary key is, by definition, unique. The unique key is therefore unnecessary and can be removed.

General comments

One thing I would do is to be explicit about nullability of each column, even if the default favors your particular case. Being explicit doesn't hurts, and it better helps readability, so I would suggest always stating each column as NULL/NOT NULL.

You might consider the posibility of allowing users like comments too, in addition to the main posts (as the likes table handles).

You can also consider the option of adding some history tables for registering each edition of posts (pretty much like StackOverflow does), in addition to the date_updated column, if you may need additional audits or rollback capability.


Data retrieval from Dynamic HTML page with time-out (Web scraping w. Python) The HTML page shows list of a friend network of a person (each Name has anchor <a> tag w. link to list of friend network). Since the page has a timer, I've written a py code to scrap the mth position (friend) of the nth count (page) by traversing through the cycle: (m->n->m->n....). And it works!

import urllib.request, urllib.parse, urllib.error
from bs4 import BeautifulSoup
import ssl

# Ignore SSL certificate errors
ctx = ssl.create_default_context()
ctx.check_hostname = False
ctx.verify_mode = ssl.CERT_NONE

url = input('Enter URL: ')
position = int(input('Enter position: '))  #Name/link Traverse
count = int(input('Enter count: ')) #Page Traverse
print("Retrieving:", url)
for c in range(count):      #returns range of indices
    html = urllib.request.urlopen(url, context=ctx).read() #opening URL
    soup = BeautifulSoup(html, 'html.parser')
    a_tags=soup('a')
    link=a_tags[position-1].get('href', None) #url = href(key) value pair
    content=a_tags[position-1].contents     #name=a_tag.contents
    url=link
    print("Retrieving:", url)

Input:

Enter URL: http://py4e-data.dr-chuck.net/known_by_Kory.html
Enter position: 1
Enter count: 10

Output:

Retrieving: http://py4e-data.dr-chuck.net/known_by_Kory.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Shaurya.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Raigen.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Dougal.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Aonghus.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Daryn.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Pauline.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Laia.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Iagan.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Leanna.html
Retrieving: http://py4e-data.dr-chuck.net/known_by_Malakhy.html

Questions:

Is there a better way to approach this? (libraries, workarounds to delay the timer)
My goals is to make an exhaustive 'list' of friends of all unique Names here; I don't want any code, just suggestions and approaches will do.

Currently you only get one new name per page request. However, the page contains all people known by that person. So I would implement a queue of yet to be visited people and a set of people already visited. If there are no more people left to visit, you have found all people (assuming there are no disjoint sets of people and that this is your actual goal).

In addition, using requests is usually a bit more user-friendly than using urllib directly.

import requests
from bs4 import BeautifulSoup, SoupStrainer
import re

STRAINER = SoupStrainer("a")

def get_name(url):
    match = re.match(r"http://py4e-data.dr-chuck.net/known_by_(.*).html", url)
    if match is not None:
        return match.groups()[0]

def find_all_people(start_url):
    with requests.Session() as session:
        queue = set([start_url])
        visited = set()
        while queue:
            url = queue.pop()
            visited.add(url)
            print(len(visited), "/", len(visited) + len(queue), url)
            response = session.get(url)
            soup = BeautifulSoup(response.text, "lxml", parse_only=STRAINER)
            queue.update(a["href"]
                         for a in soup.select("a")
                         if a["href"] not in visited)
        return list(map(get_name, visited))

if __name__ == "__main__":
    url = "http://py4e-data.dr-chuck.net/known_by_Kory.html"
    people = find_all_people(url)
    print(len(people))

This uses a request.Session to keep the connection alive, speeding it up a tiny bit. It also has a if __name__ == "__main__": guard to allow importing from this script from another script without the code running, a bs4.SoupStrainer to only parse the parts of the page needed and it uses the faster lxml parser.

It still takes quite some time to find all people. Finding that there are probably 5754 people takes only a few seconds.


implementing a queue using std::vector I tried implementing a queue using std::vector, using some properties of vector class. Is there scope for further improvement in my code, to make it more efficient?

#include<bits/stdc++.h>
using namespace std;
vector<int> q;

 void push(int d){
   q.insert(q.end(),d);
 }

 void pop(){
 q.erase(q.begin());
 }

 int front(){
 return *q.begin();
 }

 int main(){
    push(1);
    push(2);
    push(3);
    push(4);
    pop();
    cout<<front()<<endl;
     for(int i=0;i<q.size();i++) cout<<q[i]<<" ";
  }

There are a number of things that could be improved here, and none of them have to do with the efficiency of the program.  The problems with this code are more fundamental.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Don't use #include <bits/stdc++.h>

This is poor programming practice.  Here's why.

Eliminate global variables where possible

The q variable is global but should really be in main or still better, see the next suggestion.

Create a C++ object

Whenever you have some data structure and associated actions (such as push and pop in this case), you should be thinking object.  In this case, it's quite simple to create an object from the code you have.  I'll leave that fun to you.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Use more whitespace to enhance readability of the code

Instead of crowding things together like this:

 for(int i=0;i<q.size();i++) cout<<q[i]<<" ";

most people find it more easily readable if you use more space:

 for (int i=0; i < q.size(); i++) {
     std::cout << q[i] << " ";
 }

Use standard classes

Finally, there's the obvious: one could use std::queue.  I am assuming that you did not because you are still learning C++ and wanted to try to create a queue yourself.  That's fine as a programming exercise, but of course in real code, it's almost always better to use part of the standard library.


Password validator using regular expressions I wanted to learn some regex, but learning regex is super boring so I decided to write a password validator that will validate a password with regular expressions.

How it works:

You enter a password that has no echo thanks to the getpass library
The password is run against five different validations such as, upper case, lower case, special character, and digits. These are done by using the re (regex) library.
If the password does not pass one of the validations it will output a random password using the random library that will match the given guidelines.

What I would like to know is:

Are there better ways to create a random string?
Are there better regular expressions I could use to verify that password?

Source:

import getpass
import re
import random

def random_password(length=3):

    """ Create a a random password to display if the
       primary validation fails. """

    valid_upcase = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
    valid_lowcase = 'abcdefghijklmnopqrstuvwxyz'
    valid_specs = '!$@&'
    valid_digits = '1234567890'

    return ''.join((random.choice(valid_upcase) + random.choice(valid_lowcase) + random.choice(valid_specs) + random.choice(valid_digits) for i in xrange(length)))

def length_error(password):
    """ Validate that the password is over 8 characters
       and no more than 20 characters. """

    if 8 > len(password) :
        return False
    else:
        return True

def lower_error(password):

    """ Confirm that the password contains at least one
       lower case letter """

    if re.search(r"[a-z{1,9}]", password) is None:
        return False
    else:
        return True

def symbol_error(password):

    """ Make sure that the password contains at least one
       of the following special characters: ! @ $ & """

    if re.search(r"[!@$&{1,5}]", password) is None:
        return False
    else:
        return True

def upcase_error(password):

    """ Confirm that the password contains at least one
       upper case character. """

    if re.search(r"[A-Z{1,5}]", password) is None:
        return False
    else:
        return True

def digit_error(password):

    """ Confirm that the password contains at least one
       digit. """

    if re.search(r"\d{1,5}", password) is None:
        return False
    else:
        return True

def prompt(info):

    """ Get the password without echo. """

    return getpass.getpass(info)
    #return raw_input(info)  # # Uncomment this for echo

def validate_password(password):

    """ Where the validation occurs, if the password does
       not pass one of the following tests, it will output
       a random string that does pass the test. """

    if lower_error(password) is False or upcase_error(password) is False:
        print "Password did not match the uppercase or lowercase requirements."
        print "Random password to use: {}".format(random_password())
    elif digit_error(password) is False or symbol_error(password) is False:
        print "Password did not match the integer and special character requirements."
        print "Random password to use: {}".format(random_password())
    elif length_error(password) is False:
        print "Password did not meet the length requirements."
        print "Password must be over 8 characters and no more then 20 characters"
        print "Random password to use: {}".format(random_password())
    else:
        print "Password validated successfully."

def obtain_password():

    """ Main method. """

    password_to_verify = prompt("Enter password: ")
    validate_password(password_to_verify)

if __name__ == '__main__':
    obtain_password()

(Note: Python is not my main language, and this is my first answer on Code Review)

Looks alright, I can't see any obvious holes in the regex. Tried it a few times myself, worked as expected.

I don't really have a good answer for you "random string" question, but for:

Are there better regular expressions I could use to verify that password?

Well, I think you could take out the curly brackets out on them. For instance, when you're matching any lower case letter, at least once, you have:

if re.search(r"[a-z{1,9}]", password) is None:

Since you only want to know if one exists, you don't need the {1,9}. So, unless there's some strangeness with re.search that I don't know about, you could replace [a-z{1,9}] with just [a-z]. Ditto for the other validation functions.

Now, I'm not 100% certain with this, but it seems you could return either None or a number from your validation functions, and test whether it's equal to None in validate_password(). Like this:

def lower_error(password):
    return re.search(r"[a-z]", password)

def symbol_error(password):
    return re.search(r"[!@$&]", password)

def upcase_error(password):
    return re.search(r"[A-Z]", password)

def digit_error(password):
    return re.search(r"\d", password)

Now lower_error() etc return None, or a number.

def validate_password(password):
    if lower_error(password) is None or upcase_error(password) is None:
    ...
    elif digit_error(password) is None or symbol_error(password) is None:
    ...

I was a bit worried about 0  being equal to None, but typing 0 is None into the Python REPL says False.

EDIT: By the way, just noticed length_error() only checks if it's less than 8 characters, not that it's under 20.


Is it reasonable to expect this AngularJS function to perform well? I have this function which is associated with a timer that I'm creating in AngualarJS. Should I be concerned about the function being able to execute quickly enough for the timer to be accurate.

$scope.start = function() {
    if($scope.counting === true) return false;
    $scope.counting = true;
    $scope.counter = $timeout(function updateCount() {
        $scope.count++;
        $scope.hh = Math.floor($scope.count / 360000);
        $scope.mm = Math.floor(($scope.count - ($scope.hh * 360000)) / 6000);
        $scope.ss = Math.floor(($scope.count - ($scope.hh * 360000) - ($scope.mm * 6000)) / 100);
        $scope.ms = Math.floor($scope.count - ($scope.hh * 360000) - ($scope.mm * 6000) - ($scope.ss * 100));
        $scope.counter = $timeout(updateCount, 10);
    }, 10);
};

After digging around a bit, it seems the most appropriate approach for this is to create a kind of "fix" for latency resulting from CPU time that will quickly cause inaccuracies in a javascript timer. I wrote the below to demonstrate. As you can see, the setTimeout named timer1 includes a check based on the difference between the start Date object and an updated Date object. Using this value, we can calculate (with better, not perfect, accuracy) how off our incrementing time is. The setTimeout named timer2 does not include this check and, as you will see, the latency begins to accumulate.

<!DOCTYPE html>

    <title>Accurate Timer Test</title>

Corrected Example:<input type="text" value="0" name="correct" id="correct" />

Latency Example:<input type="text" value="0" name="latency" id="latency" />

Start

Stop


Python Making Methods Modular and DRY I'm learning object oriented programming and am still trying to understand the best way to structure a class for modularity and attempting to follow DRY principles. I currently created a class containing multiple requests that share the same self.reponse block repeatedly. I am not sure the best way to abstract this outside of each function call since I am also returning the response object within each method.

My code currently looks like this:

import requests
import urllib3
import logging
import re
from itertools import zip_longest

log = logging.getLogger()

server = "https://example.com"
organization = "/example/"
username = "example@example.com"
PAT = "xxxxxxxxxxxxxxxxxxxxx"

urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)

class InvokeRequest():
    def __init__(self):
        self.server = server + organization

    # Get All Repositories
    def getRepositories(self, project):
        self.URL = self.server + project + "/_apis/git/repositories"
        self.response = requests.get(self.URL, auth=(username, PAT), verify=False)
        return self.response.json()

    # Get All Build Definitions
    def getBuildDefinitions(self, project):
        self.URL = self.server + project + "/_apis/build/definitions/"
        self.response = requests.get(self.URL, auth=(username, PAT), verify=False)
        return self.response.json()

    # Get Specific Build Definition
    def getBuildDefinitionID(self, project, ID):
        self.URL = self.server + project + "/_apis/build/definitions/" + str(ID)
        self.response = requests.get(self.URL, auth=(username, PAT), verify=False)
        return self.response.json()

    # Get All Release Definitions
    def getReleaseDefinitions(self, releaseID):
        self.URL = self.server + releaseID + "/_apis/git/releaseID"
        self.response = requests.get(self.URL, auth=(username, PAT), verify=False)
        return self.response.json()

request = InvokeRequest()

repos = request.getRepositories("DevOps")

list_of_repos = []
for repo in repos["value"] or []:
    list_of_repos.append(repo["name"])

buildDefinitions = request.getBuildDefinitions("DevOps")

build_definition_IDs = []
build_pipeline_names = []
for build in buildDefinitions["value"]:
    if build["queueStatus"] == "enabled":  # Skip over disabled builds
        build_definition_IDs.append(build["id"])
        build_pipeline_names.append(build["name"])

associated_build_repos = []  # Repos containing an associated build pipeline
for ID in build_definition_IDs:
    buildDefinition = request.getBuildDefinitionID("DevOps", ID)
    log.debug(buildDefinition["repository"].get("name"), "has pipeline")
    try:
        associated_build_repos.append(buildDefinition["repository"].get("name"))
    except KeyError as e:
        log.error(e)

For now, I am only passing a single project "DevOps", however, this will change in the future. I want to find a way to prevent repeating this specific block of code:

self.response = requests.get(self.URL, auth=(username, PAT), verify=False)
return self.response.json()

I have considered creating a base class called InvokeRequestConfig() or something similar to create the setup of the server and response object, that way I could inherit that class in InvokeRequest(). however I think that adds a lot of complexity that might not be necessary.

What is the best way to reuse the code above to make it more modular and follow best practices of OOP and DRY?

All the methods in InvokeRequest are basically the same. The only part that differs is the URL. Just pass that in instead of passing in the parts of the URL and constructing the URL in the function.

Before I show the function that I came up with though, I'll note that there's no reason InvokeRequest should be a class, and there's also no reason that self.URL and self.response in those methods should be instance attributes. I'm far from practiced in OOP, but making arbitrary bits of code into classes isn't helpful. If you were passing the self.server data into the constructor, then maybe there would be some justification. As it stands now though, I'm going to get rid of the class and instance attributes.

To start out with, here's the basic part that was repeated in all the methods:

def get_response(url):
    response = requests.get(url, auth=(username, PAT), verify=False)
    return response.json()

It now expects that the url will be passed in like I mentioned. You can ease making the URL though using a helper:

def build_url(*parts):
    return BASE_URL + "".join(parts)

If you haven't encountered var-args before, *parts just means that the function accepts as many arguments as you want, and bundles them into a tuple.

Now you can write:

json = get_response(build_url(project, "/_apis/git/repositories"))  # Was getRepositories
json = get_response(build_url(releaseID, "/_apis/git/releaseID"))  # Was getReleaseDefinitions
json = get_response(build_url(project, "/_apis/build/definitions/", str(ID)))  # Was getBuildDefinitionID

You could also put the call to build_url inside get_response and have *parts as the parameter to get_response:

def get_response(*parts):
    url = build_url(*parts)
    response = requests.get(url, auth=(username, PAT), verify=False)
    return response.json()

json = get_response(project, "/_apis/git/repositories")
json = get_response(releaseID, "/_apis/git/releaseID")
json = get_response(project, "/_apis/build/definitions/", str(ID))

Which you want to use depends on if get_response will always use a URL returned by build_url, or if it can differ.

You could wrap the calls to get_response in some functions too to avoid needing to manually put the URL together each time:

def get_build_definition_ID():
    return get_response(project, "/_apis/build/definitions/", str(ID))

Also note the naming convention I used. Python uses snake_case, not camelCase.