Elixir / Phoenix login controller, allowing multiple attempts I have a login method in my controller.  I would like to get rid of the imperative "thinking" and write something more functional.

  def login(conn, %{"login" => %{"password" => password}}) do

    conn = conn |> fetch_session

    if count_attempt_login(conn) <= 10 do
      conn
        |> attempt_login(password)
    else
      conn
        |> redirect(to: "/")
    end

  end

  defp count_attempt_login(conn) do
    (conn |> get_session(:count_attempt_login)) || 1
  end

  defp attempt_login(conn, password) when password == @password_login do
      conn
        |> put_session(:is_admin, true)
        |> delete_session(:count_attempt_login)
        |> redirect(to: "/dashboard")
  end

  defp attempt_login(conn, _) do
    conn
      |> put_session(:count_attempt_login, count_attempt_login(conn) + 1)
      |> put_flash(:error, "Impossible to connect with this password")
      |> redirect(to: "/dashboard/auth")
  end

Personally I would do the inc and count of the attempts in one place,
You can still split it, but you actually need this every time an attempt is going to happen.

You could also replace the if with a case. You don't have to, but I think it looks nicer :)

The next thing is, remove the when password == @password_login and just use @password_login in the function definition

  def login(conn, %{"login" => %{"password" => password}}) do
    case inc_and_get_login_attempts(conn) <= 10 do
      true ->  conn |> try_to_login(password)
      _ ->     conn |> redirect(to: "/")
    end
  end

  defp inc_and_get_login_attempts(conn) do
    attempts = (conn |> get_session(:count_attempt_login)) || 0
    put_session(conn, :count_attempt_login, attempts + 1)
    attempts + 1
  end

  defp try_to_login(conn, @password_login) do
    conn
      |> put_session(:is_admin, true)
      |> delete_session(:count_attempt_login)
      |> redirect(to: "/dashboard")
  end

  defp try_to_login(conn, _) do
    conn
      |> put_flash(:error, "Impossible to connect with this password")
      |> redirect(to: "/dashboard/auth")
  end


Multi-threaded code to handle messages from a provider Having a look at my code, is there a way to be losing elements?

import com.google.common.collect.Maps;
import com.google.common.collect.Sets;

import java.util.Map;
import java.util.Set;
import java.util.UUID;

public class Main {
  private final static Map<String, Set<String>> data = Maps.newConcurrentMap();

  public static void main(String[] args) throws InterruptedException {
    new Main().init("X");
  }

  public void init(String code) throws InterruptedException {
    data.put(code, Sets.newConcurrentHashSet());

    subscribeToDataFrom(code);

    Set<String> elements = doHttpRequest();
    data.get(code).addAll(elements);

    data.get(code)
        .stream()
        .map(e -> e + "_MAPPED")
        .forEach(System.out::println);

    data.remove(code);
  }

  private void subscribeToDataFrom(String code) {
    Runnable r = () -> {
      while (true) {
        onMessageFromProvider(code, UUID.randomUUID().toString());
      }
    };

    new Thread(r).start();
    new Thread(r).start();
    new Thread(r).start();
    new Thread(r).start();
    new Thread(r).start();
  }

  public Set<String> doHttpRequest() throws InterruptedException {
    Set<String> resultsToReturn = Sets.newHashSet();
    resultsToReturn.add("B");
    resultsToReturn.add("C");
    resultsToReturn.add("D");
    resultsToReturn.add("E");
    resultsToReturn.add("F");
    Thread.sleep(1000);

    return resultsToReturn;
  }

  public void onMessageFromProvider(String code, String element) {
    final Set<String> newSet = data.computeIfPresent(code, (k, v) -> {
      v.add(element);
      return v;
    });

    if (newSet == null) {
      System.out.println("Ok, now I can do something different with: " + element);
    }
  }
}

I want to be able to execute the init code, get response from doHttpRequest, and once we have it, merge those results with the streamed results by subscribeToDataFrom, without losing any data.

Do I need any synchronised keyword to guarantee that, or is my code thread-safe and there's no way I'm losing any element coming from HTTP / Streaming?

There are some things to adress.
"synchronized"
Like you recognized: Using synchronized variants of certain maps or set classes will not get you away from the burden to define a proper "monitor". Often these variants are not even neccessary to solve the problem so that the synchronization elements will only reduce performance without participating within the synchronisation neccessity. A monitor is an area of code execution that has some assertions to the behaviour of the code under concurrency situations. The simplest situation would be "restrict access to the monitor to only one thread". So putting a synchronized-keyword "somewhere" will not solve the problem until your monitor is properly defined.
Responsibilities
You have to be clear about what are critical code fragments that should properly behave under concurrency. I do not want to say that I am THE reference but for me it took a while to interprete your code and identify what it is all about. Even (I would say especially) in concurrency we should be aware of clear responsibilities.
Busy waiting
Some other thing is "busy waiting" (while (true)). I reject those constructs. You should always provide a loop exit AND a mechanism to avoid busy waiting.
Usecase
As far as I understood your code you have a stream of elements from multiple arbitrary sources to print out or do whatever you want to do with it. It seems to me like a "Buffer" but currently I am not that sure about it.
Abstraction from element sources
Currently the elements of the stream come from 2 sources:

A http request, returning a set of elements that are provvided asynchronously to the stream
from multiple threads that provide message elements

My suggestion is to abstract from the sources where the elements come from. So the first responsibility will be a class that can asynchronously receive elements and output them in a sequence. Following algorithms now only need to depend on objects of this class:
public static class ElementJoiner {

    private BlockingDeque<String> queue;
    private String code;

    public ElementJoiner(String code) {
        this.queue = new LinkedBlockingDeque<>();
        this.code = code;
    }

    public void registerElement(String element) {

        try {
            this.queue.put(element);
        } catch (InterruptedException e) {
            throw new RuntimeException(e);
        }

    }

    public void registerElements(Collection<String> elements) {
        elements.forEach(element -> registerElement(element));
    }

    public String getElement() {

        try {
            return this.queue.pollFirst(5, TimeUnit.SECONDS);
        } catch (InterruptedException e) {
            throw new RuntimeException(e);
        }

    }

}

The internal deque will have a special behaviour if no elements are in the queue: If no element is available it will block a certain amount of time (here 5 seconds). Either a new element was insert that can be returned OR it will return null.
Abstract from Thread "this" and other threads
After all the only difference between registering elements from other "Threads" (UUIDs) and registering the elements of the http request is that you perform the registration in "this" Thread (the main-Thread). You only delay registration for a second (Thread.sleep(1000)).
So the task is to abstract from where elements are received. Furthermore it doesn't matter from which thread the elements come from. We have to tell one thread...

... where to receive or how to produce elements (supplier)
... where to register the received or produced elements (consumer)

Now it is easy to formulate the start and behaviour of a thread:
private void startAsynchronousElementSource(Consumer<Set<String>> consumer, Supplier<Set<String>> supplier) {

    Runnable r = () -> {
        while (true) {
            consumer.accept(supplier.get());
        }
    };

    new Thread(r).start();

}

You can see a high symmetry as we input a set of elements and pass these elements immediately to the consumer. The only thing we define here is what to do asynchronously (maybe starting the thread, but I ignore that for now).
Stream preparation
We have "ElementJoiner"-class that does pretty well. Now we want it to work together with a stream. A standard stream cannot exit as long as there is no indicator for exiting. If you have a Ranged-Stream you limit the stream through the boundaries defined. The stream we want to have has a special indicator so we have to define a so called Spliterator.
public class ElementSpliterator extends Spliterators.AbstractSpliterator<String> {

    private Supplier<String> elementSupplier;

    protected ElementSpliterator(Supplier<String> elementSupplier) {
        super(Long.MAX_VALUE, Spliterator.SIZED);
        this.elementSupplier = elementSupplier;
    }

    @Override
    public boolean tryAdvance(Consumer<? super String> action) {

        String element = elementSupplier.get();

        if (element != null) {
            action.accept(element);
            return true;
        } else {
            return false;
        }

    }

}

The tryAdvance-method handles the case if the supplier returns null to end the stream.
The supplier in our case will be the "ElementJoiner" which getElement-method will return null if the timeout has passed.
Defining simulation data
Here I harmonized the return value to be a Set.
private Set<String> getElementsFromOtherSource() {
    return Collections.singleton(UUID.randomUUID().toString());
}

public Set<String> getElementsFromHttpRequest(){

    Set<String> resultsFromHttpRequest = new HashSet<>();

    resultsFromHttpRequest.add("B");
    resultsFromHttpRequest.add("C");
    resultsFromHttpRequest.add("D");
    resultsFromHttpRequest.add("E");
    resultsFromHttpRequest.add("F");

    try {
        Thread.sleep(1000);
    } catch (InterruptedException e) {
        throw new RuntimeException(e);
    }

    return resultsFromHttpRequest;
}

Reformulating the init-method
I don't think we need the "code"-attribute in this use case neither the data-HashMap.
The first thing we will do is instantiating the ElementJoiner:
ElementJoiner elementJoiner = new ElementJoiner(forCode);

Then we can instantiate the stream with the "elementJoiner" as an element supplier AND the ElementSpliterator with the handle to identify the stream end.
Stream<String> elementsStream = StreamSupport.stream(new ElementSpliterator(elementJoiner::getElement), true /* parallel */);

After that we can start to produce elements asynchronously that will be registered in the "elementJoiner". You see that there is no difference in formulating the element source for the "other" and the "http request".
    // Other sources

    Consumer<Set<String>> consumer = elements -> elementJoiner.registerElements(elements);

    Supplier<Set<String>> otherSupplier = () -> getElementsFromOtherSource();

    startAsynchronousElementSource(consumer, otherSupplier);
    startAsynchronousElementSource(consumer, otherSupplier);
    startAsynchronousElementSource(consumer, otherSupplier);
    startAsynchronousElementSource(consumer, otherSupplier);

    // http request source, while other source already supply elements

    Supplier<Set<String>> httpRequestSupplier = () -> getElementsFromHttpRequest();

    startAsynchronousElementSource(consumer, httpRequestSupplier);

The final statement produces the output on the console:
elementsStream.map(e -> e + "_MAPPED").forEach(System.out::println);

Where is the synchronizsation???
The work of synchronisation is done by the "LinkedBlockingDeque". As we only delegate to ONE synchronized method of the "LinkedBlockingDeque" in "getElement" and "registerElement" we do not need our own synchronisation.


Finding array indices that add up to another number I'm writing a method that takes (1) an array of ints and (2) an int.  The purpose of the method is to find the indices of the two numbers in the array that add up to the value passed in as the second parameter to the method (sum) and return those values in an int[].  Exactly two numbers in the passed in array will add up to "sum", so as soon as they are identified the method should finish.  I have come up with two solutions so far, but both seem ugly to me:

//this method is bad because it has multiple returns
private int[] findIndexes(int[] intAr, int sum) {
    for (int i = 0; i < intAr.length; ++i) {
        for (int k = i + 1; k < intAr.length; ++k) {
           if(intAr[i] + intAr[k] == sum) {
              return new int[] {i + 1, k + 1};
           }
        }
    }
    //should never reach here
    return null;
}
//this method is bad because it uses "break" outside of a switch statement
private int[] findIndexes(int[] intAr, int sum) {
    int[] indexAr = new int[2];
    outer:
    for (int i = 0; i < intAr.length; ++i) {
        for (int k = i + 1; k < intAr.length; ++k) {
            if(intAr[i] + intAr[k] == sum) {
                indexAr[0] = i + 1;
                indexAr[1] = k + 1;
                break outer;
            }
        }
    }
    return indexAr;
}

Is there a better way to write this method that does not violate the coding standards mentioned above?

The coding standards I was looking at are located here.

There's nothing wrong with a break statement used outside a switch statement. (2nd example)

If a professor or some other authority figure is enforcing that rule, that's a pain, but the code is perfectly fine. There's even a similar example in the Java Tutorials on Branching


Remove nth node from a linked list end Solving it by the logic:

find the length
traverse to length-n+1
remove item

I've written the following code

class ListNode:
 def __init__(self, x):
     self.val = x
     self.next = None

# Definition for singly-linked list.
# class ListNode:
#     def __init__(self, x):
#         self.val = x
#         self.next = None

class Solution:
    # @param A : head node of linked list
    # @param B : integer
    # @return the head node in the linked list
    def removeNthFromEnd(self, A, B):
        temp = A
        len = 0
        while temp is not None:
            len += 1
            temp = temp.next
        result = ListNode(None)
        result.next =  A

        if B >= len:
            if B ==1:
                result.next = None
            else:
                result.next = A.next
        else:
            for i in range(len-B-1):
                A = A.next
            if B == 1:
                A.next = None
            else:
                A.next = A.next.next
        return result.next

if __name__ == "__main__":
        a = ListNode(1)
        b = ListNode(2)
        c = ListNode(3)
        d = ListNode(4)
        e = ListNode(5)
        a.next = b
        b.next = c
        c.next = d
        d.next = e
        A = Solution().removeNthFromEnd(a, 1)
        while A is not None:
            print A.val
            A = A.next

It seems duplicating some logic. How can we improve this code?

Firstly, the logic of the removeNthFromEnd function can be split into these parts:

calculate length of list
calculate index \$i\$ from the beginning of the list, based on B
remove \$i\$th element from the beginning

1. Calculating the length

def list_len(head):
  length = 0
  while head:
    head = head.next
    length += 1
  return length

2. Calculating index from the beginning of the list

i = list_len(A) - B

3. Removing \$i\$th element

def remove_ith(head, i):

  if i == 0:
    return head.next

  lst = head
  for _ in range(i-1):
    lst = lst.next

  lst.next = lst.next.next

  return head

So your removeNthFromEnd function will look like this:

def remove_nth_from_end(head, n):
  length = list_len(head)
  i = length - n

  if not (0 <= i < length):
    return head

  return remove_ith(head, i)

I took the liberty and renamed removeNthFromEnd to obey PEP8, a Python style guide, according to which function names should be in lower_snake_case.

I also renamed parameters from A, B to head, n. Descriptive names with some meaning are always better than just some random letter, even if it's more typing. However sometimes a letter is enough, such as in case of n or i. Especially if the function name is removeNthFromEnd – one would expect such function takes an argument n.

This basic solution has the same performance disadvantage as your solution – in the worst case, you traverse the list twice.

That's not very efficient, at least not time-wise. So I did a little optimization, which in this case might be considered cheating: I convert the linked list into a list of nodes. That way the linked list is only traversed once and you have constant-time access to all nodes. The disadvantage is of course increased memory usage, which is \$O(n)\$ compared to \$O(1)\$ of your solution, where \$n\$ is the length of the list. Optimally you would use a doubly-linked list. That way you would traverse the list at most once (on average only half of the list would be traversed), and the auxiliary space complexity would be constant, as with your original solution.

def remove_nth_from_end_(head, n):

  as_list = []
  node = head

  # Convert linked list to "array".
  while node:
    as_list.append(node)
    node = node.next

  # Calculate index from beginning.
  i = len(as_list) - n

  # Check that the list is not empty (in which case nothing can be removed)
  # and the index is within the bounds of the list.
  if as_list and i in range(len(as_list)):

    if i == 0:
      # Removing first element – just shift the head.
      head = head.next

    else:
      # Set next-pointer of the previous element to the next element.
      # This is where the constant-time access happens.
      as_list[i-1].next = as_list[i].next

  return head

To further optimize the space complexity, you could only keep the last n+1 elements while traversing the list. That would reduce the auxiliary space complexity to \$O(i_{e})\$, \$i_{e}\$ being the index of the removed element based from the end of the list.


Babel codemod to transform require statements I'm starting to explore the world of ASTs and codemods. As such my first foray is a simple transform to move statements like let fs = require('fs') to const fs = require('fs').

const declaratorKinds = { let: null, var: null }

export default function(babel) {
 const { types: t } = babel

 return {
   visitor: {
     VariableDeclaration(path) {
       if (!(path.node.kind in declaratorKinds)) { return }

       const d = path.node.declarations[0]

       if (!(
         d.init
         && d.init.callee
         && d.init.callee.name === 'require')) { return }

       path.replaceWith(
         t.VariableDeclaration('const', path.node.declarations)
       )
     }
   }
 }
}

Here's a live ast explorer version.

I'm a newbie here so i'm interested in any feedback, but I'd particuarly like to know about:

Is VariableDeclarator the best node type to base this transform on?
Is there a way I could better filter for require declarations?

The only problem with this code that jumps out is declaratorKinds = { let: null, var: null } for use with path.node.kind in declaratorKinds. It would be much better to just use an array or set.

const declaratorKinds = ['let', 'var']
...
if (declaratorKinds.includes(path.node.kind))

As far as I can tell VariableDeclaration is the best way to handle this, I have no experience with babel transforms, so I may be missing something.

Bugs:

var fs = require('fs'), temp; will be transformed into const fs = require('fs'), temp; which will throw a syntax error.
var temp = 1, fs = require('fs') will not be transformed.

A better solution would be to check each VariableDeclarator item in path.node.declarations, this could look something like the following. (Note that this rewrites multiple simultaneous declarations into separate declarations - the style I prefer)

export default function({types}) {
  return {
    visitor: {
      VariableDeclaration(path) {
        const { kind, declarations } = path.node

        if (!['let', 'var'].includes(kind)) {
          return
        }

        const requireDeclarations = declarations.filter(d => {
          return d.init && d.init.callee && d.init.callee.name == 'require'
        })
        const otherDeclarations = declarations.filter(d => !requireDeclarations.includes(d))

        if (requireDeclarations.length) {
          path.replaceWithMultiple([
            ...requireDeclarations.map(d => types.VariableDeclaration('const', [d])),
            ...otherDeclarations.map(d => types.VariableDeclaration(kind, [d])),
          ])
        }
      }
    }
  }
}

Demo here (forked from your demo)


Getting limited user input, with echo I've written a code snippet that will get a (max.) 14 character string from the user input, and while the user is typing, simultaneously echo it out to the screen.

It seems a bit long and ugly, so I was wondering if any of you would happen to have ideas on how to shorten, and generally optimize this.

for(int i = 0; i < 15; i++)
{
    char npt = _getch(); // Get input

    if(i == 14) // Last character (user has to press \r or \b)
    {
        if(npt == '\r') // enter, break loop
            break;
        else if(npt == '\b') // backspace, re-loop
        {
            if(!name.empty())
                name.erase(std::prev(name.end()));
            if(i >= 1)
                i -= 2;
            else
                i--;
        }
        else // other input, meaning re-loop
            i--;

        SetConsoleCursorPosition(hOut, cDraw);
        printf("              "); // clear string area
        SetConsoleCursorPosition(hOut, cDraw);
        std::cout << name; // echo string
        continue;
    }
    else
    {
        if(npt == '\r')
            break;
        else if(npt == '\b')
        {
            if(!name.empty())
                name.erase(std::prev(name.end()));
            if(i >= 1)
                i -= 2;
            else
                i--;
        }
        else
            name += npt; // add input to string

        SetConsoleCursorPosition(hOut, cDraw);
        printf("              ");
        SetConsoleCursorPosition(hOut, cDraw);
        std::cout << name;
    }
}

c++ isn't really my language, but I believe this would be an identical loop:

for(int i = 0; i < 15; i++)
{
  char npt = _getch(); // Get input

  if(npt == '\r')
    break;
  else if(npt == '\b')
  {
    if(!name.empty())
      name.erase(std::prev(name.end()));
    if(i >= 1)
      i -= 2;
    else
      i--;
  }
  else if(i == 14)
    i--;
  else
    name += npt; // add input to string

  SetConsoleCursorPosition(hOut, cDraw);
  printf("              ");
  SetConsoleCursorPosition(hOut, cDraw);
  std::cout << name;
}

The only difference between your first if and the else is the final else line, so I moved the conditional to the end to avoid repeating the same logic.

Additionally, I assume there's some way you could pad name with blank spaces so that you only have to call SetConsoleCursorPosition and print once per iteration.


Optimize way of reading and writing file in node.js I want to read the content of file then want to make few updates to content and then write that content to another file. I have created the code with node.js as follows.

var filePath = path.join('public/index.html'),
    exactFileName = path.basename(filePath),
    outputDir = "output/";
function readContent(callback) {
    //start reading file
    fs.readFile(filePath, {encoding: 'utf-8'}, function (err, content) {
        if (err) return callback(err)
        callback(null, content);

        //Check if directory available or not, if no then create new directory
        if(!fs.existsSync(outputDir)){
            fs.mkdirSync(outputDir, 0766, function(err){
                if(err){
                    console.log(err);
                    response.send("ERROR! Can't make the directory! \n");
                }
            });
        }

        /*
            will process content here and then write to output file
        */

        //write the data in another file
        fs.writeFile(outputDir+''+exactFileName, content, function (err) {
            if (err) return console.log(err);
            console.log('Content written to '+ exactFileName);
        });
    })
}

readContent(function (err, content) {
    console.log(content);
});

I need inputs on:

optimizing this code
direction on how we can apply it for multiple files

Interesting question,

there is really not much to optimize from a speed perspective. From a code organization perspective you should really investigate libraries that help with asynchronous coding.

From a pure coding perspective, fs.mkdirSyn will ignore the callback function you provide.

From a reliability perspective, your code will try to write to the output folder if that folder could not be created. That is not good. Also where would response be defined in that callback function?

You can apply this to multiple files by providing filePath as a parameter to readContent. readContent is by the way a terrible function name, it misleads the reader in thinking it just reads content whereas it reads contents, checks directories, optionally creates directories, processes content and writes out content. You get my point..

I am also not sure why you call the callback in readContent before all the work is done, an error might occur in creating a directory or in writing out the new content and the callback would never know..

Update:
This code works and organizes the functionality into steps without using a 3rd party library for handling asynchronous calls:

var path = require('path'),
    fs   = require('fs');
    outputDir = "output/";

function readFile( path, callback )
{
  fs.readFile( path, {encoding: 'utf-8'}, callback );
}

function verifyFolder( folderName , callback )
{
  if( fs.existsSync(outputDir) )
    return callback(true);
  fs.mkdir( outputDir, 0766, function(err){
    callback( err ? false : true );
  });
}

function processContent( content , callback ){
  callback( undefined, content.toUpperCase() );
}

function writeContent( path, content , callback )
{
  fs.writeFile( path, content, function (err) {
    if (!err)
      console.log( 'Content written to '+ path );
    callback( err, content );
  });
}

function processFile( filePath, callback) {

  var content;

  readFile( filePath, readFileCallback );

  function readFileCallback( err, data ){
    if(err)
      return callback( err, data );
    content = data;
    verifyFolder( outputDir , verifyFolderCallback );
  }

  function verifyFolderCallback( exists ){
    exists ? processContent( content , processContentCallback ) : callback( outputDir + ' does not exist' );
  }

  function processContentCallback( err, content )
  {
    if(err)
      return callback( err, content );
    writeContent( outputDir + path.basename(filePath) , content, callback );
  }
}

function logResult( err, result ){
  console.log( err || result );
}

processFile( path.join('killme.js') , logResult );


Get parent folder ids by folder id(s) My Scenario:

#TmpDistinctFolderIds table contains one or many FolderId.
fnGetAllParentFolderIds function receives the input as FolderId and return it's all possible parent folder ids (a parent folder can have N number of sub folders).

I'm using the below block to get each folder's all possible hierarchy parent folder ids. This approach is working as expected. No issues with the approach.

But can I achieve the same with out using WHILE execution or any other better approach?

DECLARE @TmpFolderId INT;
DECLARE @RelatedParentFolderIds TABLE (FolderID INT, ParentFolderID INT);

WHILE EXISTS (SELECT * FROM #TmpDistinctFolderIds)
BEGIN
    SELECT TOP 1 @TmpFolderId = FolderId FROM #TmpDistinctFolderIds

    -- Get all possible hierarchy level parent folder ids
    INSERT INTO @RelatedParentFolderIds (FolderID, ParentFolderID)
    SELECT FolderID, ParentFolderID
    FROM [dbo].[fnGetAllParentFolderIds] (@TmpFolderId)

    DELETE FROM #TmpDistinctFolderIds WHERE FolderId = @TmpFolderId
END

This is typically something you do with a cursor:

DECLARE @tmpFolderId INT;
DECLARE @RelatedParentFolderIds TABLE (FolderID INT, ParentFolderID INT);

DECLARE folderCurs CURSOR LOCAL FAST_FORWARD FOR SELECT FolderID FROM #TmpDistinctFolderIds;
OPEN folderCurs
FETCH NEXT FROM folderCurs INTO @tmpFolderId
WHILE @@FETCH_STATUS = 0
BEGIN
    -- Get all possible hierarchy level parent folder ids
    INSERT INTO @RelatedParentFolderIds (FolderID, ParentFolderID)
    SELECT FolderID, ParentFolderID
    FROM [dbo].[fnGetAllParentFolderIds] (@tmpFolderId)

    FETCH NEXT FROM folderCurs INTO @tmpFolderId
END


Chaining 3 dependent HTTP request subscriptions one, which is polling Here is a function that triggers after selecting from a dropdown to choose the floor. As you can see, there are 3 levels of API calls (regular HTTP GET requests with JSON responses) that chain together. The first 2 are regular requests and the 3rd one polls. How can I write this in a better way that utilizes RXJS and observable functions?

onChange(floor)
 {
    this.selectedFloor = floor;
    this.floorService.getRoomsInFloor(floor).subscribe((rooms:IRoom[])=>{
        rooms.forEach((room:IRoom)=>{
            this.floorService.getDeviceInRoom(room.roomId).subscribe((device:IDevice)=>{
                this.floorService.getLatestStatusOfDeviceInRoom(device.deviceId).subscribe((status:IStatus)=>{
                    if(status)
                    {
                    status.occupied == "true"? this.rooms[room.roomId][4] = "red" : this.rooms[room.roomId][4] = "green";

                    }

                    this.renderRooms();
                });
            });
        });
    });
 }

getRoomsInFloor(floorid)
{

    return this.http.get(`example.com/floor/${floorid}/rooms`).map(res => res.json()).catch(this._serverError)
}

getDeviceInRoom(roomid)
{
    return this.http.get(`example.com/devices/${roomid}`).map(res => res.json()).catch(this._serverError)

}

getLatestStatusOfDeviceInRoom(deviceid)
{
    return Observable.timer(0,2000).switchMap(()=> this.http.get(`example.com/roomPresences/${deviceid}/latest`).map(res => res.json()).catch(this._serverError));
}

First of all, you are using some kind of C# code style in your typescript. I suggest the ng lint command-line to conform with typescript/angular code style. Moreover prefixing interfaces with I is not a typescript convention (https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines).

I think the difficult part of your code with rxjs is how to know which room we have to update when we receive a status.

I find a way to keep track of the room without changing the api of the floorService via zipping the observables returned by the floorService with an observable containing the room.

Here is a proposal with rxjs, I tried to fill the blanks in your posted code to make it run so focus on the zipWithRoom function and the onChange method:

the component with the dropdown:

import { Component } from '@angular/core';
import { FloorService } from './floor.service';
import { Observable } from 'rxjs/Observable';
import 'rxjs/add/observable/zip';
import 'rxjs/add/observable/of';
import 'rxjs/add/observable/from';
import 'rxjs/add/operator/mergeMap';
import 'rxjs/add/operator/filter';

export interface IRoom {
  roomId: number;
}

export interface IDevice {
  deviceId: number;
}

export interface IStatus {
  occupied: string;
}

function zipWithRoom<T>(observable: Observable<T>, theRoom: IRoom) {
  return Observable.zip(observable, Observable.of(theRoom), (result: T, room: IRoom) => ({result, room}));
}

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.css']
})
export class AppComponent {
  selectedFloor;
  rooms: { [roomId: number]: Array<String> };

  constructor(private floorService: FloorService) {
  }

  onChange(floor: any) {
    this.selectedFloor = floor;

    this.floorService.getRoomsInFloor(floor)
    // we create a sequence from the room array
      .mergeMap((rooms: Array<IRoom>): Observable<IRoom> => Observable.from(rooms))
      .mergeMap((room: IRoom) =>
        zipWithRoom<IDevice>(this.floorService.getDeviceInRoom(room.roomId), room))
      .mergeMap(({result, room}) =>
        zipWithRoom<IStatus>(this.floorService.getLatestStatusOfDeviceInRoom(result.deviceId), room))
      .filter(({result, room}) => !!result)
      .subscribe(({result, room}) => {
        // little change to the following line to make it more readable
        this.rooms[room.roomId][4] = result.occupied === 'true' ? 'red' : 'green';
        this.renderRooms();
      });
  }

  private renderRooms() {
  }
}

spec file:

import { TestBed, async } from '@angular/core/testing';

import { AppComponent } from './app.component';
import { FloorService } from './floor.service';
import { Observable } from 'rxjs/Observable';

describe('AppComponent', () => {

  const mockedFloorService = jasmine.createSpyObj(['getRoomsInFloor', 'getDeviceInRoom', 'getLatestStatusOfDeviceInRoom']);

  beforeEach(async(() => {
    TestBed.configureTestingModule({
      declarations: [
        AppComponent
      ],
      providers: [
        {provide: FloorService, useValue: mockedFloorService}
      ]
    }).compileComponents();
  }));

  it('should update the status of the rooms', async(() => {
    const fixture = TestBed.createComponent(AppComponent);
    const app = fixture.debugElement.componentInstance;

    app.rooms = {
      1: [],
      2: []
    };

    mockedFloorService.getRoomsInFloor.and.returnValue(Observable.of([{roomId: 1}, {roomId: 2}]));
    mockedFloorService.getDeviceInRoom.and.returnValues(Observable.of({deviceId: 11}), Observable.of({deviceId: 12}));
    mockedFloorService.getLatestStatusOfDeviceInRoom.and.returnValues(
      Observable.of({occupied: 'true'}),
      Observable.of({occupied: 'false'})
    );

    app.onChange(2);
    expect(mockedFloorService.getRoomsInFloor).toHaveBeenCalledWith(2);
    expect(app.rooms[1][4]).toBe('red');
    expect(app.rooms[2][4]).toBe('green');
  }));
});


Project Euler #5 (Smallest multiple) in Swift I found this one a little bit easier than the previous problems, but I was already familiar with the GCD/LCM formulas.

import Foundation

func printTimeElapsedWhenRunningCode(title:String, operation:()->()) {
    let startTime = CFAbsoluteTimeGetCurrent()
    operation()
    let timeElapsed = CFAbsoluteTimeGetCurrent() - startTime
    println("Time elapsed for \(title): \(timeElapsed) s")
}

func greatestCommonDenominator(a: Int, b: Int) -> Int {
    return b == 0 ? a : greatestCommonDenominator(b, a % b)
}

func leastCommonMultiple(a: Int, b: Int) -> Int {
    return a * (b / greatestCommonDenominator(a, b))
}

func leastCommonMultiple(input: [Int]) -> Int {
    var result: Int = input[0]
    for value in input {
        result = leastCommonMultiple(result, value)
    }
    return result
}

func solution5() {
    let firstTwentyInts: [Int] = [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20]
    let answer: Int = leastCommonMultiple(firstTwentyInts)

    println(String(answer))
}

printTimeElapsedWhenRunningCode("Least Common Multiple", solution5)

And for the record, I know that solution5() is ugly as a function, but for the purposes of this review, you should think of that more like you might think of main in another programming language.  Ultimately, I've only packaged those lines of code into a function so I can throw it in printTimeElapsedWhenRunningCode() which is a nice way to time my solutions.

This particular solution consistently runs in under 0.001 seconds (some times as low as 0.0004 seconds) for me.

In general, I like it; nice solution. Functions are well separated and I understood what it was doing right away, just from the function names.

You can get rid of one of your leastCommonMultiple functions (and make it more Swifty I think) by passing it a Range instead of an Array and by using reduce():

func greatestCommonDenominator(a: Int, b: Int) -> Int {
    return b == 0 ? a : greatestCommonDenominator(b, a % b)
}

func leastCommonMultiple(range: Range<Int>) -> Int {
    return reduce(range, 1) { (a, b) in a * (b / greatestCommonDenominator(a, b)) }
}

func solution5() {
    let answer = leastCommonMultiple(1...20)
    println(answer)
}

That change also seems to speed it up significantly. On my machine it changed the average run time from 0.0001 seconds to 0.00002 seconds; I believe that's simply from not having to create the Array though.


Variables, references and pointers - Write the function zero (...) so that the following code works correctly I have following homework exercise from lecture "Variables, references and pointers":

  Write the function zero (...) so that the following code works
  correctly:
      int x = 3;
      zero(x);
      cout << x << endl;  // writes 0

This is my solution:

#include <iostream>

using namespace std;

void zero(int &);

int main(void) {
    int x = 3;
    zero(x);
    cout << x << endl;
    return 0;
}

void zero(int & num) {
    num = 0;
}

What do you think about it?

This isn't too complicated, so there isn't that much to say. Your code is perfectly clear, but there are a couple things that could be improved.

using namespace std;

This is okay-ish for beginning programming classes, but using namespace std should usually be avoided. See Why is “using namespace std” considered bad practice?

void zero(int &);

You could just not forward declare the function, and instead just define the function here. It's a little less code to write. It comes down to preference

int main(void) {

Don't use void to say that a function takes no arguments. It's a C-ism, and should be avoided in C++. Just write int main() {.

int main(void) {
    ...

    return 0;
}

There's no need to have a return 0; at the end of main(). For the specific case of the main function, return 0; is implied at the end of the function.


Basic ul li elements in JavaScript listeners I am learning JavaScript and I have written a code snippet. Can someone suggest some improvements?

Array.prototype.pushIfDoesntExist = function (item){
    if(this.indexOf(item) === -1) {
        this.push(item);
        return true;
    }
    return false;
}

var Testing = {
  clickedList: [],
  applyListeners: function() {
       var self = this;
          var nodes = document.getElementsByTagName('li');
        for (var i = 0; i < nodes.length; i++) {
           document.getElementsByTagName('li')[i].addEventListener('click', function() {
             //we only want to display an item once
             self.clickedList.pushIfDoesntExist(this.textContent)
              self.displayData();
           });
      }
    },
    displayData: function() {
      var textBox = document.getElementById('clickedElements');
        textBox.textContent = this.clickedList.join(', ');
    }
}

Testing.applyListeners();

Milk

Eggs

Bacon

Cheese

Don't Modify Array.prototype

Writing custom functions to built in prototypes is generally a bad idea. The basic reasoning behind this is that it is a bad practice to modify something you don't own, but check out this Stack Overflow Question for more info.

Instead of changing the prototype you could just write a global function:

function pushIfDoesntExist(array, item) {
  if (array.indexOf(item) === -1) {
    array.push(item);
    return true;
  }
  return false;
}

Weigh Out Pros and Cons before Abstracting

The function pushIfDoesNotExist is well named, so it can act as a comment for what the code is doing. Also, I can think of several of cases where it could be useful. BUT is it really worth writing a function for it? After all, you wouldn't write a function add1:

function add1(number) {
  return number + 1;
}

And that's a well named function with even more applications than pushIfDoesNotExist. What I'm trying to get at here is that abstracting a part of code is not always a good idea. There are some downsides to making a function for something:

The function delocalizes code. Someone who is reading through your code to better understand it needs to hunt down where pushIfDoesNotExist is defined to see how it works. (good documentation can help with this)
It adds a dependency. Functions that call pushIfDoesNotExist rely on pushIfDoesNotExist being defined properly. (unit tests can help with this)
It takes time to write/debug/unit-test/document/maintain/think-about pushIfDoesnNotExist.

Is the value added by the function pushIfDoesntExist really worth the cost? (especially since you only call it once)

Inline Module Constructor

If you want private member variables, or you don't like using this or self you can use an inline function to construct your Testing object.

var Testing = (function () {
  var clickedList = []; //hidden inside inline function scope

  function applyListeners() {
    document.getElementsByTagName("li").forEach(function (node) {
      if (clickedList.indexOf(elem.textContent) === -1) {
        clickecList.push(node.textContent);
        displayData();
      }
    });
  }

  function displayData() {
    document.getElementById("clickedElements").textContent = clickedList.join(", ");
  }

  return {
    applyListeners: applyListeners // public
  };
})();


boost::spreadsort for std::pair Boost 1.58 has new library spreadsort, so I tried it out.
I wrote simple sample for sorting vector of pairs of int:

#include <chrono>
#include <iomanip>

#include <boost/sort/sort.hpp>

// based on http://www.boost.org/doc/libs/develop/libs/sort/doc/html/sort/sort_hpp/string_sort.html

struct lessthan
{
  inline bool operator()(const std::pair<int, int> &x, const std::pair<int, int> &y) const
  {
    return x < y;
  }
};

struct bracket
{
  inline unsigned char operator()(const std::pair<int, int> &x, size_t offset) const
  {
    if (offset < sizeof(int))
      return get_char(x.first, offset);

    return get_char(x.second, offset - sizeof(int));
  }

private:
  inline unsigned char get_char(int x, size_t offset) const
  {
    static const boost::uint64_t base_mask = 0xff;

    const int bit_shift = 8 * (sizeof(int) - offset - 1);
    unsigned char result = (x & (base_mask << bit_shift)) >> bit_shift;
    if (offset == 0)
      return result ^ 128;

    return result;
  }
};

struct getsize
{
  inline size_t operator()(const std::pair<int, int> &) const
  {
    return 2 * sizeof(int);
  }
};

bool verify_sorted(const std::vector<std::pair<int, int>> & a)
{
  lessthan ls;
  for (size_t i = 0; i + 1 < a.size(); i++)
  {
    if (ls(a[i + 1], a[i]))
      return false;
  }

  return true;
}

int main()
{
  std::vector<std::pair<int, int>> r;

  srand(12345);
  for (size_t i = 0; i < 10000000; i++)
    r.push_back(std::make_pair(rand(), rand()));

  {
    std::vector<std::pair<int, int>> a = r;

    auto t_start = std::chrono::high_resolution_clock::now();
    std::sort(a.begin(), a.end());
    auto t_end = std::chrono::high_resolution_clock::now();

    std::cout << std::fixed << std::setprecision(2) << "CPU time used (std::sort): "
      << std::chrono::duration<double, std::milli>(t_end - t_start).count()
      << " ms\n";

    std::cout << (verify_sorted(a) ? "OK." : "FAIL!") << std::endl;
  }

  {
    std::vector<std::pair<int, int>> a = r;

    auto t_start = std::chrono::high_resolution_clock::now();
    boost::sort::spreadsort::string_sort(a.begin(), a.end(), bracket(), getsize(), lessthan());
    auto t_end = std::chrono::high_resolution_clock::now();

    std::cout << std::fixed << std::setprecision(2) << "CPU time used (boost::sort::spreadsort::string_sort): "
      << std::chrono::duration<double, std::milli>(t_end - t_start).count()
      << " ms\n";

    std::cout << (verify_sorted(a) ? "OK." : "FAIL!") << std::endl;
  }
}

It's faster then std::sort, but it's seems to me this implementation is to heavyweight and ugly for such a simple task.
Can anyone propose better one?

You spend a lot of lines of code trying to get the expressions lessthan(), getsize(), and bracket() to behave like plain old functions. Consider just using plain old functions, instead. For example:

static uint8_t get_char(int x, size_t offset)
{
    const int bit_shift = 8 * (sizeof(int) - offset - 1);
    uint8_t result = (x >> bit_shift) & 0xFF;
    if (offset == 0) result ^= 128;
    return result;
}

static uint8_t bracket(const std::pair<int,int>& x, size_t offset)
{
    if (offset < sizeof(int))
      return get_char(x.first, offset);
    return get_char(x.second, offset - sizeof(int));
}

...
string_sort(a.begin(), a.end(), bracket, getsize(), lessthan());

You could also productively replace the expression getsize() with

auto getsize = [](auto){ return 2*sizeof(int); };

...
string_sort(a.begin(), a.end(), bracket, getsize, lessthan());

Finally, lessthan is equivalent to std::less<std::pair<int,int>>, so you can just write

string_sort(a.begin(), a.end(), bracket, getsize, std::less<>());

That's the kind of shortening you were looking for, right? :)


As long as this input array is static in its depth and structure, the best and fastest technique (due to lowest overhead) is to use old-school loops.
Beyond that, I would:

Not declare variable that will not be used
Use square brace pushing syntax
Avoid excessively wide lines of code which demand the developer to horizontally scroll
Use words as variable names to make your code more readable

Code: (Demo)
$final = [];
foreach ($categorizedOptions as $categories) {
    foreach ($categories as $categoryId => $category) {
        foreach ($category['attribute'] as $attributeId => $attribute) {
            foreach ($attribute['option'] as $optionId => $option) {
                $final[] = [
                    'category' => $categoryId,
                    'categoryname' => $category['name'],
                    'attribute' => $attributeId,
                    'attributename' => $attribute['name'],
                    'option' => $optionId,
                    'optionname' => $option['name']
                ];
            }
        }
    }
}

Don't worry with trying to convert this code into some sexy one-liner.  Trying to do so with recursion or nested functions will make your code ugly and negatively impact performance.
Even trying to get fancy with "array destructuring" and a compact() call don't look worth the effort.  (Demo)


Is this violating the DRY principle? I feel that I am repeating myself a lot with this HTML/PHP code. Am I right, and is there a maintainable way of reducing the amount of repetition in the code?

mydomain/index.php:

<!DOCTYPE HTML>
<?php $root="/mywebdir"; ?>

<?php include("$root/inc/meta.php"); ?>
<meta name="description" content="Some description" />
<meta name="keywords" content="some,home,page,keywords" />
<title>Home - This Website</title>

<?php include("$root/inc/header.php"); ?>
<?php include("$root/inc/nav.php"); ?>

Home

Introduction.

Some content.

More content.

<?php include("$root/inc/footer.php"); ?>

mydomain/somedir/index.php:

<!DOCTYPE HTML>
<?php $root="/mywebdir"; ?>

<?php include("$root/inc/meta.php"); ?>
<meta name="description" content="Some description" />
<meta name="keywords" content="some,keywords,about,this,page" />
<title>Some Page - This Website</title>

<?php include("$root/inc/header.php"); ?>
<?php include("$root/inc/nav.php"); ?>

Some Page

Some content about this page.

<?php include("$root/inc/footer.php"); ?>

Consider using existing template systems to help you that support inheritance. Template inheritance isn't a strict requirement, but is a powerful feature that makes maintaining templates a lot easier. Twig is easy to use, and feature-rich.

If you want to go it your own way, you could simply implement the use of layout files:

layout_default.php

<!DOCTYPE HTML>
<?php $root="/mywebdir"; ?>

<?php include("$root/inc/meta.php"); ?>
<meta name="description" content="<?php echo $description ?>" />
<meta name="keywords" content="<?php echo keywords?>" />
<title><?php echo $title ?></title>

<?php include("$root/inc/header.php"); ?>
<?php include("$root/inc/nav.php"); ?>

<?php echo $contents ?>

<?php include("$root/inc/footer.php"); ?>

example page.php

<?php ob_start() ?>

 main body html

<?php
$title = "x";
$description = "x";
$keywords = "x";
$contents = ob_get_clean();
require 'layout_default.php'

This is a very crude implementation, simply enough to illustrate the principle.


Function queue for synchronous execution The purpose of this code is to allow to add functions to a queue, that will execute them sequentially.

With this in mind, it also allows to execute them with a defined delay, to prevent them from blocking the browser.

(function(window, undefined){
    'use strict';
    var queue = [];
    var running = false;
    var stop = false;
    var log = function(data){
        //some browsers may not have the .error method
        (window.console.error || window.console.log).call(window.console, data);
    };
    var runNext = function(){
        //I have to do like this, so I don't insert it back
        var fn = queue[0];
        setTimeout(function(){
            try
            {
                //sometimes, when it is to stop, it's already here
                if(!stop)
                {
                    queue.shift();//remove it from the queue
                    fn.fn();
                }
            }
            catch(e)
            {
                //simply log it, to give a chance to other functions
                log(e);
            }

            if(queue.length && !stop)
            {
                setTimeout(runNext, 1);
            }
            else
            {
                running = false;
            }
        }, fn.delay);
    };

    window.FnQueue = {
        add: function(fn, delay){
            if(!(fn instanceof Function))
            {
                log('FnQueue.add - fn must be a function');
                return window.FnQueue;
            }
            if(isNaN(delay) || delay < 1)
            {
                log('FnQueue.add - delay must be a number equal or higher than 1');
                return window.FnQueue;
            }
            queue.push({
                fn:fn,
                delay:delay
            });
            //starts the execution only if it wasn't running and stopped
            if(!running && !stop)
            {
                running = true;
                setTimeout(runNext, 1);
            }
            return window.FnQueue;
        },
        clear: function(){
            queue = [];
            return window.FnQueue;
        },
        stop: function(){
            stop = true;
            return window.FnQueue;
        },
        resume: function(){
            stop = false;
            if(!running && queue.length)
            {
                setTimeout(runNext, 1);
            }
            return window.FnQueue;
        },
        getQueue: function(){
            return queue;
        },
        isRunning: function(){
            return running && !stop;
        }
    };
})(Function('return this')());

What can I improve on my code to make it less spaghetti-ish and more distributed? What inefficient/unnecessary code can I improve?

With this in mind, it also allows to execute them with a defined delay, to prevent them from blocking the browser.

This is a common misconception of how the timers work. Currently executing code doesn't stop when a timer expires. That callback waits until the currently running code finishes.

You can think of JS as having 3 internal data structures:

The execution stack, where the currently running code is.
The callback queue, where all callbacks for async operations are stored
The callback ready queue, where all callbacks for competed async operations are stored.

When you call a timer function, that callback is stored in the callback queue until the timer expires. When it does, it moves to the ready queue, but it doesn't execute just yet. The current code in the stack will continue until it exhausts the stack. Only then will the engine pick from the ready queue and execute it. This is what they call the "event loop" which is basically code taking turns, one after the other.

So adding a delay isn't really needed. You can even put a delay of 0 and it won't block the UI.

What does block the UI is when operations in the stack take too long - regardless where they came from. If you happen to call a function that iterates through 10 billion items, that's the problem. Your stack won't exhaust fast enough to let other code run.

(window.console.error || window.console.log).call(window.console, data);

This is a problem, because some browsers don't even have a console object (I'm looking at you IE8). You'll probably get a "Accessing property error of undefined) when you run this piece of code. Check for console first to be safe.

try {
  //sometimes, when it is to stop, it's already here
  if(!stop){
    queue.shift();//remove it from the queue
    fn.fn();
  }
}

There are 2 problems in this piece of code. First is why are you even trying to use try-catch. Whenever I see a try-catch, it's an indicator that the developer is unsure of what will happen with the code. Solve the root problem of why the code can be unpredictable and make it predictable, before resorting to a try-catch.

The next is optimization. Engines like V8 optimize code heavily (and which is why Chrome is fast). However, code inside a try-catch doesn't get optimized because it is deemed unpredictable.

So what you can do is place all of your code in the try-catch inside a function and call that function in the try-catch instead. That way, the engine can optimize the function, while what doesn't get optimized is the call.

setTimeout(runNext, 1);

I assume your function queue isn't sensitive to when it gets called due to this. Anyways, the problem with this code is that it's calling setTimeout all the time. The problem here is that, as explained in the first section of the answer, you are scheduling when you call a timer.

A better solution for this is to schedule once, using setInterval. Each iteration will call the function next in line. Once the queue is done, you can clear the interval using clearInterval.

Function('return this')()

While this relatively looks safe (because the string isn't arbitrary), you will run into issues when using this piece of code in environments with very strict CSP rules. This includes mobile devices, browser extensions and some websites who lock down these things for security.

getQueue: function(){
  return queue;
},

I wouldn't want one to manipulate the queue outside if I were you. It could mess up your code. I suggest you return a copy of the queue, and not the queue array itself. Use slice to return a copy of the queue.

clear: function(){
  queue = [];
  return window.FnQueue;
},

A better way to clear an array without creating another array is to just set the length to zero.

If I understood the code correctly and following my suggestions, the following should be a simple replication:

;(function(ns){

  var queue = [];
  var isRunning = false;
  var runner = null;

  ns.add = function(fn){
    queue.push(fn);
    if(!isRunning) ns.resume();
    return ns;
  };

  ns.clear = function(){
    queue.length = 0;
    return ns;
  };

  ns.stop = function(){
    clearInterval(runner);
    runner = null;
    isRunning = false;
    return ns;
  };

  ns.resume = function(){
    if(!isRunning && !runner){
      isRunning = true;
      runner = setInterval(function(){
        // Here, I intentionally set the context to null. This makes the
        // callbacks have a `this` value of null, instead of `window`.
        queue.shift().call(null);
        if(!queue.length) ns.stop();
      }, 0);
    }
    return ns;
  };

  ns.getQueue = function(){
    return queue.slice(0);
  };

  ns.isRunning = function(){
    return isRunning;
  };

}(this.FnQueue = this.FnQueue || {}));

// Sample usage

alert('Let us start. This is your first alert in the stack');

FnQueue.add(function(){
  alert('First scheduled item');
});

alert('This should fire second, we are still in the stack');

FnQueue.add(function(){
  alert('Second scheduled item');
});

FnQueue.add(function(){
  alert('Third scheduled item');
});

FnQueue.add(function(){
  alert('Fourth scheduled item');
});

alert('After this alert, the stack is empty. All callbacks should fire in sequence');


Building functions.php Lets say I build functions.php and I have 3 functions inside of it that I use. Lets say that I build a 4th function, but what I really want to do is simply put the instructions of function 4 into function 1 for example. Is there anything wrong with this?

Consider my example here, where I have included a function inside of a function. I am aware that the variables inside of a function are local, and that you cannot call a variable inside a function outside of it, however I have tested this with a simple echo test and it seems to work. Is there anything wrong with doing this? If so, can you explain how you might go about doing this generally? You do not have to be specific.

functions.php

function validate_registration($password, $confirmpassword, $email) {
    if ((empty($email)) || (empty($password)) || (empty($confirmpassword))) {
        registration_form();
        echo "Please enter the required information:";
    }
    elseif ((filter_var($email, FILTER_VALIDATE_EMAIL)) && ($password == $confirmpassword)) {
        input_registration($email, $password);
    }
    elseif ((filter_var($email, FILTER_VALIDATE_EMAIL)) && ($password !== $confirmpassword)) {
        registration_form();
        echo "Your password does not match!";
    }
    else {
        registration_form();
        echo "You have not entered a valid email address!";
    }

}

the input_registration function:

function input_registration ($email, $password) {
$email_clean = htmlspecialchars($email);
$password_clean = htmlspecialchars($password);

$hash = md5($password_clean);

$db = new PDO("mysql:host=localhost;dbname=users", "root", "")

$statement = $db->prepare("INSERT into userinfo(email, hash) VALUES (:email, :hash)")

//continue with inputting info into database

It is absolutely ok to call functions from other functions. There is nothing wrong with that. However, there's room for improvements in your code.

function.php:

There is no need to put simple expressions within parenthesis in your conditions.
The order of your conditions is irritating (negative, positive, negative, negative). Group them properly. That also helps to simplify the conditions.

function validate_registration($password, $confirmpassword, $email)
{
    if (empty($email) || empty($password) || empty($confirmpassword)) {
        registration_form();
        echo "Please enter the required information:";
    } elseif (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
        registration_form();
        echo "You have not entered a valid email address!";
    }  elseif ($password !== $confirmpassword) {
        registration_form();
        echo "Your password does not match!";
    } else {
        input_registration($email, $password);
    }
}

input_registration:

Escape values on output using the right escaping function.  Since you use prepared statements, no extra escaping is needed for the database.
Create the database connection in central place, so you don't have to change several files and functions, if the database (or your password) changes.

function input_registration($db, $email, $password)
{
    $hash = md5($password); // Consider using a better hash algorithm
    $statement = $db->prepare("INSERT INTO userinfo(email, hash) VALUES (:email, :hash)");

    // Continue with inputting info into database
}

Of course you have to change validate_registration to support the injection of the database dependency.


Fizz Buzz in Haskell Pretty straightforward, I implemented the popular fizz buzz challenge in Haskell.

import qualified Data.Map as M

factorWords = M.fromList [(3, "fizz"), (5, "buzz")]

factors :: Int -> [Int]
factors x = filter (\y -> x `mod` y == 0) factorKeys
    where factorKeys = M.keys factorWords

fizzbuzz :: Int -> String
fizzbuzz x = if null fs then show x
                        else concatMap ((M.!) factorWords) fs
    where fs = factors x

main = mapM_ (putStrLn . fizzbuzz) [1..100]

I'm guessing people are going tell me that using Data.Map and concatMap is overkill for a program of this size and complexity, but it seemed the most natural way to do it in Haskell for whatever reason. Any advice, particularly regarding my use of data structures?

*ahem* Data.Map is an overkill for this program. It needs the containers package. Also, if a number is divisible by \$k\$ of your \$K\$ divisors, your algorithm is \$\mathcal O(K + k \log K)\$ (*), so worst case (\$K = k \$) you will have \$\mathcal O(K \log K) \$.

This is due to the definition of factors. Compare your factors with the following variant:

factors' :: Int -> [(Int, String)]
factors' x = filter (\(y,_) -> x `mod` y == 0) factorPairs
  where factorPairs = M.toAscList factorWords

It's almost the same, and has the same algorithmic complexity as your previous factors, namely \$\mathcal O(K)\$. But this time, you're getting more information from a single function: you can define both factors and fizzbuzz in terms of factors', without using the original map/list anymore:

factors :: Int -> [Int]
factors = map fst . factors'

fizzbuzz :: Int -> String
fizzbuzz x = case concatMap snd (factors' x) of
               [] -> show x
               xs -> xs
-- feel free to use your "where style" here instead

As you can see, a list of pairs is enough to capture everything, since you're going to traverse all of them either way. It also leads to a complexity of \$\mathcal O(K)\$ (*). If you're concerned about the order of elements, a single Data.List.sort at the original definition can help.

However, even this might be an overkill if you never intend to increase the number of factors. If all you want to do is to solve fizzbuzz, you can drop down to a guard solution:

fizzbuzz :: Int -> String
fizzbuzz x
  | isDivisibleBy 15 = "FizzBuzz"
  | isDivisibleBy  5 =     "Buzz"
  | isDivisibleBy  3 =     "Fizz"
  | otherwise        =     show x
 where isDivisibleBy n = x `rem` n == 0

Which beats all other discussed solutions in terms of complexity and clarity.

(*) Only counting filter and lookup, not actual concatenation.


Class to unit-test functions, with rudimentary support for testing multi-parameter functions Recently I read about test-driven-design and decided to try to create a simple unit testing class to begin using this approach. I have not been programming in Python for very long so forgive my ignorance of the language.

I would like to discuss better approaches to handling multi-parameter test functions. Right now I accept a tuple (of varying length) with one or more function inputs.

I find the tuple input and "length" checks (lines 10-13) to be clunky. Is there a better way to handle this? I tried using a spread operator to distribute the arguments into the test function but this fails for list inputs (it spreads the list input itself rather than inserting it as the argument).

Like I said this is very basic and I'm sure there are modules out there I can import to do a much more thorough job. I learn best when I try something myself so while I appreciate any suggestions for external modules I'd like to focus on improving this class itself.

class Unit_Tester:
  def __init__(self, function, tests):
    self.function_to_test = function
    self.test_list = tests

  def unit_test(self, function_to_test, test_input, expected_output):
      try:
          number_of_inputs = len(test_input)
          if number_of_inputs == 1: function_output = function_to_test(test_input)
          elif number_of_inputs == 2: function_output = function_to_test(test_input[0], test_input[1])
          elif number_of_inputs == 3: function_output = function_to_test(test_input[0], test_input[1], test_input[2])
          elif type(test_input) == list: function_output = function_to_test(test_input)
      except Exception as error:
          print("Error occurred with test input: [{0}] value: {1}\nError Message: {2}\nCorrect the error and try again.\n"
          .format(type(test_input), test_input, error))
      else:
        try:
            assert function_output == expected_output
            print(self.unit_test_response(True, test_input, function_output, expected_output))
        except AssertionError:
            print(self.unit_test_response(False, test_input, function_output, expected_output))

  def unit_test_response(self, correct, test_input, function_output, expected_output):
      score = "Test Passed" if correct else "Test Failed"
      return "{0}\nInput: {1}\nExpected Output: {2}\nFunction Output: {3}\n".format(score, test_input, expected_output, function_output)

  def run_unit_tests(self, function_to_test, test_list):
      for test_tuple in test_list:
          test_input, expected_output = test_tuple
          self.unit_test(function_to_test, test_input, expected_output)

  def run(self):
    self.run_unit_tests(self.function_to_test, self.test_list)

Since indentation is crucial in Python, you should consistently use four spaces, as specified in PEP 8, the official Python style guide.

Also, don't mix naming styles.  Based on PEP 8, the class should be named UnitTester.

Your test harness neglects to support…

calling functions with no parameters, or with more than 3 parameters.  This is easily solved using the * operator:

function_output = function_to_test(*test_input)

calling functions with keyword parameters.  You could accomplish this using the ** operator.
testing that a function raises an exception as expected.  That is left as an exercise for you.
testing whether a function performs expected side-effects, such as printing certain output.  This is quite tricky to implement, but it's something that doctest can easily handle.

def aloha():
    """
    >>> aloha()
    Hello
    'Goodbye'
    """
    print('Hello')
    return "Goodbye"

This code is an abuse of assertions:

    try:
        assert function_output == expected_output
        print(self.unit_test_response(True, test_input, function_output, expected_output))
    except AssertionError:
        print(self.unit_test_response(False, test_input, function_output, expected_output))

Assertions are a way for you, as a programmer, to state that a certain condition must logically be true, not that you hope for a condition to be true.

Furthermore, assertions can be disabled, in which case every test would succeed!

You should replace that code with:

      result = (function_output == expected_output)
      print(self.unit_test_response(result, test_input, function_output, expected_output))

My biggest complaint about this unit-testing class, though, is that it is unclear how it is meant to be used.  You didn't describe the intended usage in the question, and you didn't write docstrings like you should have.  I'm guessing that this is your intended usage:

Unit_Tester(max, [
    ([3, 1, 4], 4),
    ([-3, -1, -4], -1),
]).run()

But why does the run_unit_tests(…) method exist?  Is this a "valid" way to use your harness?

Unit_Tester(None, None).run_unit_tests(max, [
    ([3, 1, 4], 4),
    ([-3, -1, -4], -1),
])

If it is, then I recommend getting rid of the tests parameter in the constructor.  If it isn't, then I recommend merging run_unit_tests() into run(), so that misuse is impossible.

Alternatively, name your "private" methods with a leading underscore (e.g. _unit_test_response(…) to indicate that a method should not be called directly.


Null object design pattern in a linked list I'm trying to create a doubly linked list using the null object design pattern. I've implemented four classes: a node abstract class, nullnode and datanode classes for the null node object and the linkedlist class, for the linked list implementation. I'm not sure if I've done this correctly.

Can anyone tell me if I am correctly using the null object design pattern and if not, how I can fix it?

/* Node abstract class */
class node{
public:

node();

    node(int el) { element = el; }

    /* Return pointer to next node */
    node* getNext() { return next; }

    /* Set pointer to next node */
    void setNext(node* n) { next = n; }

    /* Return pointer to prev node */
    node* getPrev(){ return prev; }

    /* Set pointer to prev node */
    void setPrev(node* n) { prev = n; }

    /* Return element stored in node */
    int getElement() { return element; }

    /* Set element stored in node */
    void setElement(int e) { element = e; }

    node* self();

private:
    // pointer to next node
    node* next;

    // pointer to prev node
    node* prev;

    // element stored in node
    int element;
};

/* Null node class */
class NullNode : public node{
public:

    /* Return pointer to next node */
    node* getNext() { return NULL; }

    /* Set pointer to next node */
    void setNext(node* n){ /* Do Nothing */ }

    /* Return pointer to prev node */
    node* getPrev() { return NULL; }

    /* Set pointer to prev node */
    void setPrev(node* n){ /* Do Nothing */ }

    /* Return element stored in node */
    int getElement() { return -1; }

    /* Set element stored in node */
    void setElement(int e){ /* Do Nothing */ }

    node* self(){ return NULL; }

};

/* datanode class */
class dataNode : public node{

public:

    dataNode(int ele) { node::node(); }

    // Return pointer to next node
    node* getNext() { return node::getNext(); }

    // Set pointer to next node
    void setNext(node* n){ node::setNext(n); }

    // Return pointer to prev node
    node* getPrev() { return node::getPrev(); }

    // Set pointer to prev node
    void setPrev(node* n){ node::setPrev(n); }

    // Return element stored in node
    int getElement() { return node::getElement(); }

    // Set element stored in node
    void setElement(int e){ node::setElement(e); }

    node* self(){ return this; }

};

/* linked list class */
class linkedlist{
public:

    linkedlist(){
        container.setNext(&container);
        container.setPrev(&container);
    }

    ~linkedlist();

    // Insert an element at the beginning of the list
    dataNode* insertHead(int element);

    // Return pointer to first node in list
    dataNode* getHead() { return (container.getNext())->self(); }

    // Return pointer to last node in list
    dataNode* getTail() { return (container.getPrev())->self(); }

    void insertBefore(dataNode* n, int e);

    void insertAfter(dataNode* n, int e);

// Remove specified node
void remove(dataNode* n);

private:
    // Pointer to first node in list
    NullNode container;
};

The question's classes are node, NullNode, dataNode, and linkedlist. Notice that the naming scheme appears not to be case-aware. It is best to fix this. Let's use camel case and capitalize the first letter. So we'll use Node, NullNode, DataNode, and LinkedList.

A problem is the type of LinkedList::container is NullNode. This means that LinkedList cannot contain DataNode objects!

DataNode -> Node  (-> means implies)

NullNode -> Node

Also, the null object design pattern is not properly used for linked lists. For example, NullNode::setNode() does not set the node. What does this mean? If the caller wants to store a value in a LinkedList object and it only contains a NullNode should the method do nothing?

What if the result of NullNode::getNext() is invoked? For instance,

NullNode x = ...;
NullNode w = x.getNext();
w.getNext(); // Grr, error!

In terms of rewriting this, I'm not sure abstracting nodes (with the null object pattern) is useful for doubly (or singly) linked lists. My gut leads me to believe that using the null object pattern for abstracting the data or elements of the list may work well.

class Data
{
    // interface for getter and setter
};
class BlankData : public Data
{
    // returns 0 for summing, 1 for multiplying, etc.
};
class IntegerData : public Data
{
    // returns value
};
class Node
{
    // typical pointer handling code
    Data element;
};


Generic absolute value function I wanted to write a generic abs function that would correctly work for every type. Basically, I wanted to use the following algorithm:

If the type is a built-in integer, use std::abs from <cstdlib>.
If the type is a built-in floating-point, use std::abs from <cmath>.
If the type is a user-defined type with a namespace-level abs, use it.
Otherwise, call a generic abs algorithm.

Here is what I came up with:

#include <cmath>
#include <cstdlib>

namespace math
{
    namespace detail
    {
        // generic abs algorithm
        template<typename T>
        constexpr auto abs(const T& value)
            -> T
        {
            return (T{} < value) ? value : -value;
        }
    }

    template<typename T>
    constexpr auto abs(const T& value)
        -> T
    {
        using std::abs;
        using detail::abs;
        return abs(value);
    }
}

The idea is to create a generic detail::abs algorithm, then to create another abs function that will choose the function to call thanks to the argument-dependant lookup. Here is what I tried to take into account:

While the generic algorithm also works for built-in integral and floating point types, std::abs may produce optimized code for these types. Using std::abs when possible will probably generate an optimized executable. That said, std::abs lacks constexpr, which is a desirable feature, and the compiler might recognize a absolute value-like construct and optimize it away...
Some types have a namespace-level abs that does not behave like the generic algorithm. Therefore, we have to call this namespace-level function if it exist.
Some types may be huge. Therefore, I chose to take the parameter by const& since some namespace-level abs may also take their parameter by const&.
Some types only provide operator< to represent the ordering, but not the other relational operators. Therefore, calling operator< in the generic algorithm is more likely to work.
I chose to use T{} instead of 0 for the comparison in the generic algorithm in order to be able to represent the default value for any given type. A type is not guaranteed to be comparable to an integer.

Here is a test case to demonstrate what the function can achieve (you can also test it online):

namespace eggs
{
    struct Foo
    {
        Foo(int val):
            val(val)
        {}

        int val;
    };

    Foo abs(Foo foo)
    {
        return { std::abs(foo.val) };
    }
}

struct Bar
{
    Bar(int val=0):
        val(val)
    {}

    Bar operator-() const
    {
        return { -val };
    }

    int val;
};

bool operator<(const Bar& lhs, const Bar& rhs)
{
    return lhs.val < rhs.val;
}

int main()
{
    using namespace std::literals;

    std::cout << math::abs(-5) << '\n';
    std::cout << math::abs(-5.3f) << '\n';
    std::cout << math::abs(-5i+2.0) << '\n';

    eggs::Foo foo = { -8 };
    std::cout << math::abs(foo).val << '\n';

    Bar bar = { -9 };
    std::cout << math::abs(bar).val << '\n';
}

What do you think of such a function? Did I miss any obscure error? Do you see anything that could be improved (in the implementation, I don't care about the test case)?

I believe you have a bug in your generic implementation, especially in relation to floating-point style classes that have signed-zero values:

your code:

return (T{} < value) ? value : -value;

would be better as a T{} <= value (or rewritten as (T{} > value) ? -value : value;).

Your current logic will return -0.0 for an input value of 0.0, and that's not appropriate for an abs() function.

Additionally, I don't know how you would really test these things, because, if I am not mistaken, in floating-point comparisons with signed-zero values, -0.0 == 0.0 yet I would expect that abs(-0.0) would return 0.0.

How you resolve this issue though, I don't know.


Multi threaded circular buffer I've written a circular buffer with multithreaded write and single read ability:

public class RingBuffer<E> extends AbstractList<E> implements RandomAccess {

    class Pointer {
        int p = 0;

        public int getPointer(){
            return this.p;
        }

        public void increasePointer(int p) {
            this.p = p;
        }
    }

    private final int n;
    private final List<E> rb;

    private int size = 0;
    private Pointer ptr_w = new Pointer();
    private Pointer ptr_r = new Pointer();

    public RingBuffer(int n) {
        this.n = n + 1;
        this.rb = new ArrayList<E>(Collections.nCopies(this.n, (E) null));
    }

    public int size() {
        return this.size;
    }

    private int wrap(int i) {
        int m = i % this.n;
        return (m < 0) ? m += this.n : m;
    }

    @Override
    public E get(int i) {
        return (this.size == 0) ? null : this.rb.get(i);
    }

    @Override
    public E remove(int i) {
        E e = this.get(this.ptr_r.getPointer());
        if(e != null) {
            this.rb.set(this.ptr_r.getPointer(), null);
            setPointer(false, this.ptr_r);
        }
        System.out.println("Read: " + this.ptr_r.getPointer() + " " + size());
        return e;
    }

    @Override
    public synchronized boolean add(E e) {
        if (size() == this.n - 1)
            throw new IllegalStateException("Cannot add element. Ringbuffer is filled.");
        this.rb.set(this.ptr_w.getPointer(), e);
        setPointer(true, this.ptr_w);
        System.out.println("Write: " + this.ptr_w.getPointer() + " " + size());
        return true;
    }

    private synchronized void setPointer(boolean a, Pointer ptr) {
        this.size = (a) ? this.size + 1 : this.size - 1;
        ptr.increasePointer(wrap(ptr.getPointer() + 1));
    }
}

I would like to know what I could improve and what you think about the code. I'm new to multithreading and would love to get some feedback.

I highly recommend Java Concurrency in Practice by Brian Goetz et al. This will give you a solid foundation for building multi-threaded Java applications.

The biggest problem with RingBuffer is that it is not thread-safe. You must synchronize all read and write operations to be certain to see the latest values for fields. This doesn't mean you have to lock the structure the entire time.

This is explained far better in the book than I can here. The short of it is that without synchronization via memory barriers, the JVM is free to cache values read in one thread and miss writes happening in another.

Thread A calls get and sees that the buffer is empty.
Thread B calls add which updates the pointer.
Thread A calls get again, but without a memory barrier the cached pointer value tells it that the queue is still empty.

You'll need to add synchronization to guard any access of the pointers and size. Sometimes you can merely add synchronized to the method, but this usually hurts performance. You want to shrink your synchronized blocks to the bare minimum; prefer using synchronized (<lock>) blocks over synchronized methods.

Here are some other suggestions unrelated to concurrency:

RingBuffer composes an ArrayList so it should not extend AbstractList itself. It shouldn't even implement the basic List interface nor RandomAccess because it doesn't truly support the get operation. It should implement Queue instead.
As written, get allows reading outside the buffer. Are you sure you want to support this? See (1) above.
You don't need a class to hold an integer pointer given that it is internal to RingBuffer. A simple int is sufficient.
The modulo operator % should handle negative values already (test this!) so wrap could be simplified.


Check multiple subdirectories and delete if file exists Long story short, I have over 180K files that I need to delete. I have all of the file names that need to be deleted in a TXT file, however the files that need deleting are scattered in over 400 folders within this one directory.

I'm attempting to design a script that reads a .TXT file, checks if the file exists in any of the sub directories and deletes if found.

I'm quite knew to PowerShell / Object-oriented scripts so need some assistance.

$WorkingDir = 'D:\TEST\'
$txtFile = Get-Content "D:\TEST\Files.txt"

#foreach file in the .TXT
foreach ($file in $txtFile){
    # foreach file in the folders
    foreach($dir in (Get-ChildItem $WorkingDir -Recurse)){
        # if the file name is in directory listed, then delete?
            If (Test-Path $file){
                Remove-Item $file
                  }
             }
      }

Example of data in TXT file:

215865.wav
215864.wav
215875.wav
215887.wav
215885.wav
215884.wav
215902.wav
215906.wav

Attempted to run script above on some test data so it didn't have to scan through hundreds of folders...runs successfully but nothing gets deleted.

Your approach is very inefficient

the array $txtfile is already in memory
iterating it and per entry searching recursivly the directories (the var $dir in fact also includes files) creates a lot of unneccessary overhead.

$WorkingDir = 'D:\TEST\'
$txtFile = Get-Content "D:\TEST\Files.txt"

Get-ChildItem $WorkingDir -Recurse|ForEach-Object{
    If ($txtFile -contains $_.Name){
        Remove-Item $_.FullName -WhatIf
    }
}

If the output looks OK, remove the trailing -WhatIf


Tasks based on variable definition I am creating a playbook that takes a variable ver at runtime and then runs tasks based on that variable. If that ver is defined, it takes the version number and installs a version according to that. If the ver is not defined, it defaults to a default version.

My playbook for now looks like:

- name: Download Maven from remote repository.
  unarchive:
    src: "{{ baseurl }}/{{ versions[ver] }}"
    dest: /usr/local/
    remote_src: yes
  become: yes
  when: ver is defined

- name: Rename maven folder.
  command: mv /usr/local/apache-maven-{{ver}} /usr/local/maven-{{ver}}
  become: yes
  when: ver is defined

- name: Create symbolic link to maven folder.
  file:
    src: "/usr/local/maven-{{ver}}"
    dest: "/usr/local/maven"
    state: link
  become: yes
  when: ver is defined

- name: Download Default Maven from remote repository.
  unarchive:
    src: "{{ baseurl }}/{{ versions[default_version] }}"
    dest: /usr/local/
    remote_src: yes
    list_files: yes
  become: yes
  when: ver is not defined

- name: Rename maven folder.
  command: mv /usr/local/apache-maven-{{default_version}} /usr/local/maven-{{default_version}}
  become: yes
  when: ver is not defined

- name: Create symbolic link to maven folder.
  file:
    src: "/usr/local/maven-{{default_version}}"
    dest: "/usr/local/maven"
    state: link
  become: yes
  when: ver is not defined

As you can see, based on whether a variable is defined or not, I have to write whole new tasks for it.

The above works but I think it's not optimized as I'm writing too much code to get this to work. One idea that I have is to create have main.yaml invoke another task file when ver is defined and a separate task file when ver is not defined.

Is there another way to optimize this? I have many other tasks that deal with version number.

It's a duplicate of Ansible - Use default if a variable is not defined

version: "{{ ver | default('default_version') }}"

To use default_version in case ver is empty

- set_fact:
    version: "{{ default_version }}"
  when: ver == ""

or (Ansible Galaxy warning-free option)

  when: ver | length > 0


Generic makefile for C++ projects Looking at back at the C++ projects I've developed, the Makefiles are a mess.  To tell you the truth, I copied one a long time ago and basically have been cutting and pasting it haphazardly ever since.  I've decided to change that so I've been reading more about how Make actually works and some best practices for using it going forth.  As most of the programs I write are not that complicated I've come up with a generic Makefile I can cut and paste ^W^W^W deploy going forth.  I'd like some feedback on it.

Any input (except "you should be using cmake or $(OTHER_TRENDY_NEW_TOOL) instead") is welcome but some specific questions that come to mind are:

Have I forgotten to add something?
Conversely, have I added too much?
Is it worth making this a POSIX makefile.  Currently I've only tried
it with GNU make on Linux but portability is good.  What about BSD or
Windows make? Or should I just rely on gmake being available pretty
much everywhere?
Do I have all the recommended compiler/linker flags for C++?  I would
like to be able to choose between g++ and clang atleast so options
should be portable between both though if there is something good
that only one supports I can special case it.  (Can I?)
I'm assuming Visual Studio/Windows in general is a whole separate can
of worms so I've ignored it for now but if you know how this Makefile
could be adapted for Windows development I'd be happy to know.

Thanks in advance.

    PROGRAM=someprogram
    SRCDIR:=src
    INCDIR:=include
    DEPDIR:=deps
    BUILDDIR:=build
    DESTDIR?=
    PREFIX?=/usr/local
    BINDIR?=bin

    SRC:=$(wildcard $(SRCDIR)/*.cc)
    OBJECTS:=$(patsubst $(SRCDIR)/%.cc,$(BUILDDIR)/%.o,$(SRC))
    DEPFILES:=$(patsubst $(SRCDIR)/%.cc,$(DEPDIR)/%.d,$(SRC))

    CXX?=/usr/bin/g++
    STRIP?=/usr/bin/strip
    INSTALL?=/usr/bin/install
    VALGRIND?=/usr/bin/valgrind

    CPPFLAGS:=-I$(INCDIR)
    CXXFLAGS:=-std=c++17 -Wall -Wextra -Wpedantic -flto ${CXXFLAGS}
    LDFLAGS:=-ffunction-sections -fdata-sections -Wl,-gc-sections $(LDFLAGS)
    DEPFLAGS=-MT $@ -MMD -MP -MF $(DEPDIR)/$*.Td

    COMPILE.cc=$(CXX) $(DEPFLAGS) $(CXXFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c
    LINK.cc=$(CXX) $(CXXFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH)
    POSTCOMPILE=mv -f $(DEPDIR)/$*.Td $(DEPDIR)/$*.d && touch $@

    all: debug

    $(BUILDDIR)/%.o: $(SRCDIR)/%.cc $(DEPDIR)/%.d | $(DEPDIR) $(BUILDDIR)
        $(COMPILE.cc) $(OUTPUT_OPTION) $<
        $(POSTCOMPILE)

    $(BUILDDIR)/%.o: $(SRCDIR)/%.s | $(BUILDDIR)
        $(COMPILE.s) $(OUTPUT_OPTION) $<

    $(BUILDDIR): ; @mkdir -p $@

    $(DEPDIR): ; @mkdir -p $@

    $(DEPFILES):

    $(PROGRAM): $(OBJECTS)
        $(LINK.cc) $(OUTPUT_OPTION) $^

    debug: CPPFLAGS += -DDEBUG
    debug: CXXFLAGS += -g3
    debug: $(PROGRAM)

    release: CXXFLAGS += -O2
    release: distclean $(PROGRAM)
        $(STRIP) --strip-all  -R .comment -R .note $(PROGRAM)

    memcheck: debug
        $(VALGRIND) --suppressions=valgrind.suppressions --quiet --verbose --leak-check=full --show-leak-kinds=all --track-origins=yes --log-file=valgrind.log ./$(PROGRAM)

    install: release
        $(INSTALL) -m755 -D -d $(DESTDIR)$(PREFIX)/$(BINDIR)
        $(INSTALL) -m755 $< $(DESTDIR)$(PREFIX)/$(BINDIR/$(PROGRAM)

    clean:
        -rm -rf $(BUILDDIR)
        -rm -rf $(DEPDIR)
        -rm valgrind.log

    distclean: clean
        -rm $(PROGRAM)

    .PHONY: all debug release memcheck install clean distclean

    .DELETE_ON_ERROR:

    -include $(wildcard $(DEPFILES))

```

It's reasonable to require GNU Make - it's available on all platforms that have their own Make (as far as I know), and trying to cope with the vagaries of all vendors' Make implementations is an exercise in futility.  So I think you've taken the right approach here.

You're working against Make by putting build products into subdirectories.  It's easier to build them into the working directory, and the sources can be found using VPATH, then there's no need to copy all the built-in rules (not so bad when all your source files are C++, but when you need to add a few assembler and C files, then the maintenance starts to grow).
Creating separate debug and release binaries in the same build tree is problematic - it can mean a total rebuild when switching from one kind to another.  It's more usual to have separate build directories for the two, so you can incrementally build either at any time (of course, they can share the same source files, using VPATH, and can share most of the Makefile the same way).
That would look something like this:
debug/Makefile
CXXFLAGS += -g3 -DDEBUG
VPATH = ../src:../include
include ../Makefile

release/Makefile
CXXFLAGS += -O2
VPATH = ../src:../include
include ../Makefile

Then, building the release version doesn't affect the objects used to build the debug version, and vice versa.

This target is problematic for a parallel build:

release: distclean $(PROGRAM)

We'll lose some of the files as the distclean isn't sequenced with respect to $(PROGRAM).
I don't like the explicit strip invocation in the release target: since install depends on release, this makes it impossible to build debug-symbol Debian packages the usual way.  Just let the packager do the stripping.

I see no value in redefining COMPILE.cc and LINK.cc: CXXFLAGS and LDFLAGS are provided specifically for you to add your own flags to these lines - just use them.
Speaking of which, we normally use += to add to the flags:
CPPFLAGS += -I$(INCDIR)
CXXFLAGS += -std=c++17 -Wall -Wextra -Wpedantic
CXXFLAGS += -flto

LDFLAGS += -ffunction-sections -fdata-sections
LDFLAGS += -Wl,-gc-sections

DEPFLAGS = -MT $@ -MMD -MP -MF $(DEPDIR)/$*.Td
CPPFLAGS += $(DEPFLAGS)

$(RM) is provided as a more portable alternative to rm -f; we could use that in a few places.
.DELETE_ON_ERROR is often missed - kudos for remembering that.
DESTDIR?= is a no-op - undefined Make variables already expand to nothing.  Just omit this line.  It's good that you're correctly allowing this to be set by packaging systems etc.


Calculating total number of calories burned The following question was taken from Absolute Java 5th ed. by Walter Savitch:

  One way to measure the amount of energy that is expended during
  exercise is to use metabolic equivalents (MET). Here are some METS for
  various activities:

  Running 6 MPH: 10 METS
  Basketball: 8 METS
  Sleeping: 1 MET

  The number of calories burned per minute may be estimated using the
  following formula: Calories/Minute= 0.0175 * MET* Weight in
  kilograms.

  Write a program that calculates and outputs the total number of calories burned for a 150-pound person who runs 6 MPH for 30
  minutes, plays basketball for 30 minutes, and then sleeps for 6
  hours. One kilogram is equal to 2.2 pounds.

This is the code that I have written:

public class Question1 {

    final static int RUNNING6MPH = 10;
    final static int BASKETBALL = 8;
    final static int SLEEPING = 1;

    private static double value = 0.0175;
    private static double totalCaloriesBurned = 0;

    public static void main(String[] args) {

        totalCaloriesBurned = caloriesPerMinute(RUNNING6MPH, 150, 30)
                + caloriesPerMinute(BASKETBALL, 150, 30)
                + caloriesPerMinute(SLEEPING, 150, hoursToMinutes(6));
        System.out.println(totalCaloriesBurned);
    }

    public static double caloriesPerMinute(int met, double weight, double time) {
        return value * met * poundToKilogram(weight) * time;
    }

    public static double poundToKilogram(double pound) {
        return pound / 2.2;
    }

    public static double hoursToMinutes(double hours) {
        return hours * 60;
    }
}

First, not the most important point but first to grab my attention, prefer static final instead of final static in the name of least surprise.

Make everything private that can be made private, as @maartinus said.

This is a constant so make it final also:

private static double value = 0.0175;

Also name value is too generic, use the term from the field if available, you can name it CALORIES_PER_MET_PER_KG_PER_MINUTE, or at least SCALING_FACTOR.

This is not used anywhere other than main therefore should be a local variable there:

private static double totalCaloriesBurned = 0;

Then you do not need to initialize it to an unused value.

In main unnamed constant 150 is used repeatedly. You should name it weight or even better weightInPounds.

In:

public static double caloriesPerMinute(int met, double weight, double time)

caloriesPerMinute name is wrong, as it does not compute a per minute value. It could be renamed calories or caloriesBurned.

double weight, double time can be named weightInPounds and timeInMinutes to remind the user to do necessary conversion beforehand.

After renamings I suggested above the formula: value * met * poundToKilogram(weight) * time reads:

CALORIES_PER_MET_PER_KG_PER_MINUTE * met * poundToKilogram(weightInPounds) * timeInMinutes

So that it is easier to verify you did not mess up the units.


Did you twist my words? Challenge
Given two strings, determine if the second string is a rotation of the first.
Specifications

The first argument is a path to a file.
The file contains multiple lines.
Each line is a test case represented by two comma separated strings.
For each test case print out True/False if the second string is a rotation of the first.

Sample Input

Code,deCo
Review,wRevie
testcase,casetoast

Sample Output

True
True
False

Source
My solution
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define LINE_LENGTH 64

char* str_mul(char* str, int times) {
    char *string_multiplied = malloc(sizeof(str) * times + 1);
    for (int i = 1; i <= times; i++) {
        strcat(string_multiplied, str);
    }
    return string_multiplied;
}

bool is_rotated(char *original, char *test_case) {
    int original_length = strlen(original);
    char *rotation_superset = str_mul(original, 2);
    bool is_substring = strstr(rotation_superset, test_case) != NULL;
    return original_length == strlen(test_case) && is_substring;
}

char* parse_and_evaluate(char *line) {
    const char tokens[] = ",\n\r";
    char *original = strtok(line, tokens);
    char *test_case = strtok(NULL, tokens);
    return is_rotated(original, test_case) ? "True" : "False";
}

int main(int argc, char *args[]) {
    if (argc < 2) {
        fprintf(stderr, "File path not provided. Exiting...\n");
        return 1;
    }

    if (argc > 2) {
        puts("Excessive arguments, only the first will be considered.");
    }

    FILE *file = fopen(args[1], "r");
    if (file == NULL) {
        perror("Error");
        return 1;
    }

    char line[LINE_LENGTH];
    while (fgets(line, LINE_LENGTH, file)) {
        printf("%s\n", parse_and_evaluate(line));
    }

    fclose(file);
}

You're bleeding memory for every line in the file.  This is probably ok for the challenge, however it's a good idea to get into the habit of cleaning up after yourself.

The call to malloc in str_mul needs to have a corresponding free call somewhere to release the memory.  Looking at your program in it's current state, the easiest way to do that is probably in the is_rotated method like so:

bool is_rotated(char *original, char *test_case) {
    int original_length = strlen(original);
    char *rotation_superset = str_mul(original, 2);
    bool is_substring = strstr(rotation_superset, test_case) != NULL;
    free(rotation_superset);
    return original_length == strlen(test_case) && is_substring;
}

I'd be tempted to actually move the malloc into the same method and pass the buffer into the str_mul method so that the allocation and release are at the same level.  This would also allow you to simply declare a local on the stack in is_rotated, rather than needing to use malloc:

void str_mul(char* str, char *string_multiplied, int times) {
    string_multiplied[0] = '\0';
    for (int i = 1; i <= times; i++) {
        strcat(string_multiplied, str);
    }
}

bool is_rotated(char *original, char *test_case) {
    int original_length = strlen(original);
    char rotation_superset[LINE_LENGTH*2];
    str_mul(original, rotation_superset, 2);
    bool is_substring = strstr(rotation_superset, test_case) != NULL;
    return original_length == strlen(test_case) && is_substring;
}

malloc Vs calloc

malloc isn't guaranteed to return zero initialized memory.  Since you're using strcat to append to the buffer, you should really ensure that the memory is a null terminated array.  You can either do that by initially null terminating it (as in my example above), or by using calloc instead.