Send multiple broadcast to multiple listeners I want to send multiple broadcast to multiple listeners in go.

I can't send data through a channel, because only one listener will get it, except for the close signal (second "multiple" of my title).

However once a channel is closed, I can't send any other signal (first "multiple" of my title).

Hence I wrote the following code (explanation at the end of this post):

package main

import "sync"

// Broadcast allows to send a signal to all listeners
type Broadcast struct {
    lock sync.RWMutex
    ch   chan struct{}
}

// NewBroadcast creates a new broadcast
func NewBroadcast() *Broadcast {
    return &Broadcast{
        lock: sync.RWMutex{},
        ch:   make(chan struct{}),
    }
}

// Receive a channel on which the next (close) signal will be sent
func (b *Broadcast) Receive() <-chan struct{} {
    b.lock.RLock()
    defer b.lock.RUnlock()
    return b.ch
}

// Send a signal to all listeners
func (b *Broadcast) Send() {
    b.lock.Lock()
    defer b.lock.Unlock()
    close(b.ch)
    b.ch = make(chan struct{})
}

Usage example:

func main() {

    b := NewBroadcast()

    done := make(chan bool)
    quit := make(chan struct{})

    go func() {
        <-b.Receive()
        done <- true
    }()

    go func() {
        <-b.Receive()
        <-b.Receive()
        <-b.Receive()
        <-b.Receive()
        done <- true
    }()

    go func() {
        <-done
        <-done
        close(done)
        close(quit)
    }()

    func() {
        for {
            select {
            case <-quit:
                return
            case <-time.After(1 * time.Millisecond):
                b.Send()
            }
        }
    }()

}

I simply close the "current" channel to send a broadcast to all listeners and I create a new channel for the next Send. A ReadWriteMutex is added to prevent concurrency issues.

I would appreciate any comment regarding the code, the style and anything else!

Concept
The basic concept of having a channel that you hand out to all receivers, and then close that channel as a signal, is a good one. The way you use that concept in your example usage, is a bit contrived, and I don't think it represents a real use-case, but it's good enough for an idea of how it could be used.
Communication
In Go, the best practice is to use "communication" over "locks". This is documented in the sync package as:

Other than the Once and WaitGroup types, most are intended for use by low-level library routines. Higher-level synchronization is better done via channels and communication.

In your code, what does this mean? It means the Mutex.Lock() code is not great, and that you should find a way to accomplish this with communication instead.
Further, there's no need for a RWMutex in your code, a simpler Mutex would be better...
For "Communication" in go, you need a go routine to communicate with. The go-routine is linked to the Broadcast instance, and it's the only routine that knows the Broadcast state, and the only go-routine that can change that state.
Now, you just need to communicate state changes to/from that go-routine using channels.
In code, it would look like:
// Broadcast allows to send a signal to all listeners
type Broadcast struct {
    poke      chan bool
    receivers chan chan struct{}
    done      chan bool
}

// NewBroadcast creates a new broadcast
func NewBroadcast() *Broadcast {
    bc := &Broadcast{
        poke:      make(chan bool),
        receivers: make(chan chan struct{}),
        done:      make(chan bool),
    }
    go bc.listen()
    return bc
}

func (b *Broadcast) listen() {

    defer close(b.done)

    notify := make(chan struct{})
    for {
        select {
        case ok := <-b.poke:
            if !ok {
                return
            }
            // all current receivers get a closed channel
            close(notify)
            // set up next batch of receivers.
            notify = make(chan struct{})
        case b.receivers <- notify:
            // great. A Receiver has our channel
        }
    }
}

// Close makes Broadcast implement the Closer interface
func (b *Broadcast) Close() error {
    select {
    case b.poke <- false:
    case <-b.done:
    }
    return nil
}

// Receive a channel on which the next (close) signal will be sent
func (b *Broadcast) Receive() <-chan struct{} {
    select {
    case r := <-b.receivers:
        return r
    case <-b.done:
        // FIXME - should probably return an error.
        return nil
    }
}

// Send a signal to all current Receivers
func (b *Broadcast) Send() {
    select {
    case b.poke <- true:
    case <-b.done:
        // FIXME - attempt to notify on a closed broadcaster... oops. Should probably return an error
    }
}

What's important in the above code? There is no sync usage, and there's a go-routine listen() for the Broadcast that manages the state. It listens for new Receivers and it listens for notification events. It is single-threaded, and as a consequence, it does not need any locking.
State changes are communicated via the receiver, poke and done channels.
Note that it now manages resources beyond the simple struct that it was, it's now got a go-routine you don't want to leak, so it should implement the Close() method, and you would do something like:
broadcaster := NewBroadcast()
defer broadcaster.Close()

It's very important to note that all the channels are unbuffered. This allows for a clean and reliable communication path, you can't have stale receivers, etc.


UPenn CIS 194 Homework 3: Code golf I am working through UPenn CIS 194: Introduction to Haskell (Spring 2013). Since I am not able to take the course for real I am asking for CR (feedback) as it could be from teacher in that course.

HW3 - Code golf - Full description

module Golf where

import Data.List

-- ["apple","orange","plum"] example
-- ... [("apple",0),("orange",1),("plum",2)] added indexes
-- ... [("apple",0),("orange",1),("plum",2)] filtered by predicate: index `mod` 1 == 0
-- [["apple","orange","plum"]] ++ ... [("orange",0),("plum",1)] added indexes
-- [["apple","orange","plum"]] ++ ... [("orange",0)] filtered by predicate: index `mod` 2 == 0
-- [["apple","orange","plum"]] ++ [["orange"]] ++ ... [[("plum",0)]] added index
-- [["apple","orange","plum"]] ++ [["orange"]] ++ ... [[("plum",0)]] filtered by predicate: 0 `mod` 3 == 0
-- [["apple","orange","plum"]] ++ [["orange"]] ++ [["plum"]]
-- [["apple","orange","plum"],["orange"],["plum"]]
skips :: [a] -> [[a]]
skips = skips' 1

skips' :: Integer -> [a] -> [[a]]
skips' _ [] = []
skips' n root@(_:xs) = [fst $ unzip $ filter devidedByIndex $ zip root [0..]] ++ skips' (n + 1) xs
  where
    devidedByIndex x = (snd x) `mod` n == 0

-- [1,2,9,3]
-- 2 > 1 && 2 > 9 = False
-- localMaxima [2,9,3]
-- 9 > 2 && 9 > 3 = [9] ++ localMaxima [3]
-- [9] ++ []
-- [9]
localMaxima :: [Integer] -> [Integer]
localMaxima [] = []
localMaxima (x:[]) = []
localMaxima (x:y:[]) = []
localMaxima (x:y:z:xz)
  | y > x && y > z = [y] ++ localMaxima (z:xz)
  | otherwise      = localMaxima (y:z:xz)

-- putStr (histogram [1,2,2,3,3,3,4,4,4,4,5,5,5,5,5,6,6,6,6,7,7,7,8,8,9])
--      *
--     ***
--    *****
--   *******
--  *********
-- ==========
-- 0123456789
histogram :: [Integer] -> String
histogram m = transform (maximum n) n
  where
    n = countElements m

transform :: Int -> [Int] -> String
transform 0 _  = "==========\n0123456789\n"
transform i n  = intercalate "" ((map transform' n) ++ ["\n"]) ++ transform (i - 1) n
  where
    transform' x
      | x >= i    = "*"
      | otherwise = " "

-- count how many times each number appears in array
countElements :: [Integer] -> [Int]
countElements n = map count [0..9]
  where
    count x = length (elemIndices x n)

go

First just a blanket statement about names. Since this is
a golfing challenge you should pick short names for
varibles, helper functions, etc. Of course, that makes
things less readable, but there are some conventions that
Haskellers follow:

go         - names a local helper function
xs, ys, zs - a list
(x:xs)     - xs is the tail of a list whose head is x
x'         - a value related to x

Some people complain about the heavy use of single letter
variable names in Haskell, but when used judiciously I think
it actually aids readability.

skips

You can perform filter, fst and unzip all in a single
list comprehension like this:

skips' n root@(_:xs) = [ x | (x,k) <- zip root [0..], mod k n == 0] ++ ...

Also, note that you define devidedByIndex, but you only call it once.
In a situation like that you can always inline the definition where it is used,
and that's what we've done here by putting the mod k n ... expression
directly into the list comprehension.

localMaximum

You have three cases which return the same value, so just put the
last case first and use a default pattern for the others:

localMaxima (x:y:z:xz) = ...
localMaximum _ = []

Next, because we're golfing, you can avoid the otherwise clause and
duplicating the recursion call like this:

localMaxima (x:y:z:zs) = (if x < y && y > z then [y] else []) ++ localMaximum ...

histogram

There are two sub-problems:

Computing the counts
Building the picture.

Let's start with #2, and say we have the counts in a list ns, i.e. the counts
for the example is [0,1,2,3,4,5,4,3,2].

A quick way to create the row for the k-th level is:

row k ns = [ if n >= k then '*' else ' ' | n <- ns ]

and then we just need to stack those lines together:

(row 9 ns) ++ "\n" ++ (row 8 ns) ++ "\n" ++ ... (row 1 ns) ++ "\n"

Perhaps this gives you some ideas on how to improve the code.

In your computeElements, again note that your helper count is only used once,
so for golfing purposes you can inline it:

countElements n = map (\x -> length (elemIndices x n)) [0..9]

Also, compare:

elementIndices x n          - 16 non-space chars
[ y | y <- n, y == x ]      - 13 non-space chars

Since you only interested in the length both will work.

Finally, whenever you have map (\x -> ...) [...] it's the same as
a list comprehension:

map (\x -> .A.) [.B.]  = [ .A. | x <- [.B.]]

and the list comprehension is shorter by a few characters.

import ...

The exercise encourages you to look for standard library functions to help
you reduce your code size.

Here are some library functions which might be applicable to these problems:

tails :: [a] -> [[a]]
sort :: Ord a => [a] -> [a]
group :: Eq a => [a] -> [[a]]
unlines :: [String] -> String


Producer - Consumer interaction I have an implementation of Producer-Consumer multi-threaded interaction. It works, but I feel that during the execution wait states happen too often between Consumer thread and Producer.  In my case, Consumer accesses a queue at random intervals and takes data from it.  Now, the producer thread runs during the whole process life time.  The producer thread works as caching machine.  It checks in a loop if the size of queue is smaller than the maximum cache size allowed, and if that's the case, it keeps pushing new data into that cache.

My concern is that when the consumer tries to access the queue, the last is still locked by the producer thread and the consumer should wait.  I would like to get a feedback if what I am doing is optimal in terms of interaction flow between producer and consumer.

   //Called by consumer(main thead) to retrieve data from cache
 uint8_t* Worker::GetFrame() {

  boost::unique_lock<boost::mutex> lk(_frameCacheMutex);
  //If the cache is empty:
  while (0 == _frames_cache.size()) {
           //tell Producer to push data into cache
         _fill_cache_wait_cond.notify_one();
          //wait for the data to arrive(will be signaled by worker thread)
         _get_frame_wait_cond.wait(lk);

  }
  uint8_t * fr = _frames_cache.front();
  _frames_cache.pop();
  // notify worker thread to continue caching
  _fill_cache_wait_cond.notify_one();

  return fr;
}

Producer thread:

  void Worker::operator () () {
   //Some init here..
    while (isRunning) {
       boost::unique_lock<boost::mutex> lk(_frameCacheMutex);

       /// Here create the data for cache...

       _frames_cache.push(data);

       /// Notify waiting main thread to take data
       _get_frame_wait_cond.notify_one();

       /// If the cache is full ,wait
       while (_frames_cache.size() == cacheMaxSize ){

             _fill_cache_wait_cond.wait(lk);

       }

    }

}

Definitely not optimal as you are holding locks longer than you need too.

   boost::unique_lock<boost::mutex> lk(_frameCacheMutex);

   /// Here create the data for cache...

   // You are creating data here.
   // But you should not be interacting with the cache here.
   // So why are you holding a lock?

   // You should only hold a lock while you are trying to update a shared resource.

   _frames_cache.push(data);

So I would expect it to look like this:

   /// Here create the data for cache...

   boost::unique_lock<boost::mutex> lk(_frameCacheMutex);
   _frames_cache.push(data);

As a side note.
Using _ as a prefix on identifiers in C++ user applications is frowned upon (as in _frames_cache). Do you know the exact rules on when identifiers are reserved? You did not break them; but I would still advise against it because you don't know the exact rules.

This looks wrong:

while (0 == _frames_cache.size()) {
       //tell Producer to push data into cache
     _fill_cache_wait_cond.notify_one();
      //wait for the data to arrive(will be signaled by worker thread)
     _get_frame_wait_cond.wait(lk);

}

It is in a while loop because the thread was woken early but another thread has grabbed the work. So I would not expect you notify_one at this point. Also You are notifying the producer when the cache is empty. I would rather notify the producer as soon as there is any space available. So each time you grab something notify the producer (because if you take something then there is space available.

  while (0 == _frames_cache.size()) {
      //wait for the data to arrive(will be signaled by worker thread)
     _get_frame_wait_cond.wait(lk);
  }
  //tell Producer to push data into cache
  // We are about to take one item so there will be space available,
  _fill_cache_wait_cond.notify_one();

Your usage of wait() is a bit cumbersome. With the new lambda expressions you can express the wait and loop in a single expression.

  _get_frame_wait_cond.wait(lk, [this](){return 0 != _frames_cache.size();});

You are using Yoda Conditionals.

  while( 0 == _frames_cache.size() ) {}

This is a bit dated. And does not provide any real protection but also imposes a mental tax on the reader of your code as it is not the natural way to express the condition.

You get much better protection by making the compiler detect the problems you are looking for.

 // You are trying to solve this problem
 while( _frames_cache.size() = 0 ) {}
                       //   ^^^  accidental assignment.

Turn you compiler warning level up (it should already be at the highest level). And treat all warnings as errors (as they are logical errors in your thinking of how the code works). Do that and this is no loger an issue as it is a compiler error detected for you and the reader of your code gets readable code.

Your indentation and blank lines in your code make it look real messy. Tidy it up.

Some (most) of your comments are usless and parat what the code says.

  // notify worker thread to continue caching
  _fill_cache_wait_cond.notify_one();

When you write a comment it should be a description of WHY or an explanation of some complex algorithm that you are trying to implement. Writing a comment like the above one is actually worse than not writing a comment because now you have to maintain the comment to be the same as the code (and I can already read the code and see what it is doing I don't need the comment to say the same thing).

You also seem to be using two different naming conventions for your identifiers:

 _frames_cache         http://en.wikipedia.org/wiki/Snake_case
 _frameCacheMutex      http://en.wikipedia.org/wiki/CamelCase

Personally I prefer CamelCase but am not that ground in. Follow the standards of the place you work (but pick one).

I use CamelCase. But a leading uppercase character indicates a User Type while a leading lower case character indicates a variable/method/function name (something I can take the address of). I don't use special prefixes to mark members (I just name the members very well so there usage is obvious).

Cleaned up code:

Consumer

// Consumer: Get one item from the cache.
//           If the cache is empty then wait until
//           data is available.
uint8_t* Worker::GetFrame() {

    std::unique_lock<std::mutex> lk(frameCacheMutex);
    getFrameWaitCond.wait(lk, [this](){return framesCache.size() != 0;});

    uint8_t* fr = framesCache.front();
    framesCache.pop();

    fillCacheWaitCond.notify_one();
    return fr;
}

Producer

// Producer: run in infinite loop
//           Generate the data.
//           Get the lock and wait for space to be available before
//           continuing. This way you pre generate the data and
//           can push it as soon as there is space.
void Worker::operator () () {

    while (isRunning) {

       /// Here create the data for cache...

       std::unique_lock<std::mutex> lk(frameCacheMutex);
       fillCacheWaitCond.wait(lk, [this]{return framesCache.size() != cacheMaxSize;});

       framesCache.push(data);

       getFrameWaitCond.notify_one();
    }
}


Construct the tree from a list of edges I was using the following code to construct the tree from a list of nodes: [1, 2] means that there is a bi-directional link between 1 and 2

def createTreeFromEdges(edges):
    tree = {}
    for i in edges:
        v1, v2 = i
        if v1 in tree:
            tree[v1].append(v2)
        else:
            tree[v1] = [v2]

        if v2 in tree:
            tree[v2].append(v1)
        else:
            tree[v2] = [v1]

    return tree

and it works in the following fashion:

edges = [[2, 1], [3, 2], [4, 2], [5, 1], [6, 1], [7, 1], [8, 7]]
print createTreeFromEdges(edges)

This is cool, but for a python it looks like too much code. Is there a way to make it shorter?

The following is very Pythonic, but a little too terse if you are not used to it:

def createTreeFromEdges(edges):
    tree = {}
    for v1, v2 in edges:
        tree.setdefault(v1, []).append(v2)
        tree.setdefault(v2, []).append(v1)
    return tree

Basically, setdefault does the following:

def setdefault(self, key, default=None):
    if key not in self:
        self[key] = default
    return self[key]


Console application to search for files that contain a match for a regex I created a small console application to recursively search a folder and all subfolders for files that contain a match for a regex.

The command line arguments are the path you're looking to search followed by the regex (in .NET regex format).

I break this up into two threads: the main thread loops through the files and puts file data (their content, path, and name) on a concurrent queue. The second thread processes the queue, looking for regex matches.

Would using a Parallel.ForEach loop be better here? The main issue I had with this is that the queue is being modified while we're looping over it, but a Parallel.ForEach loop assumes that you already have a complete collection. The two possibilities I came up with this:

Just read the content of the files, and then do a Parallel.ForEach loop to do the regex.
Process it on the background thread like I'm doing until we're done reading the file contents, then do a Parallel.ForEach loop over any remaining items once we're done.

What I have here works but is slower than I'd like. I'd definitely like feedback on the multithreading I'm doing here, especially if one of the possibilities I mentioned above would be better than what I'm doing (or if there's another possibility that I don't mention above).

namespace SearchByRegex
{
class Program
{
    // I should probably just move this out to its own file
    private class FilePair
    {
        public string Content { get; set; }

        public string FilePath { get; set; }

        public string FileName { get; set; }
    }

    private static ConcurrentQueue<FilePair> fileInformation = new ConcurrentQueue<FilePair>();

    static void Main(string[] args)
    {
        Console.WriteLine("Working...");

        // Command lines are a path followed by the regular expression
        var regex = new Regex(args[1], RegexOptions.Multiline);

        // Process
        Thread thread = new Thread(PrintInformationOnMatches);
        thread.Start(regex);

        SearchDirectory(args[0]);

        while (fileInformation.Any())
        {
            Thread.Sleep(1000);
        }

        thread.Abort();

        Console.WriteLine("Press any key to exit");
        Console.ReadLine();
    }

    /// <summary>
    /// Print information on all files whose content matches the <paramref name="regex"/>
    /// </summary>
    /// <param name="regex">Regex we're looking for</param>
    private static void PrintInformationOnMatches(object regex)
    {
        var castRegex = (Regex)regex;
        while (true)
        {
            // Process the entire queue
            while (fileInformation.Any())
            {
                FilePair pair;
                fileInformation.TryDequeue(out pair);

                if (castRegex.IsMatch(pair.Content))
                {
                    Console.WriteLine($"File name: {pair.FileName}. Path: {pair.FilePath}.");
                }
            }

            // Wait for more stuff on the queue
            Thread.Sleep(1000);
        }
    }

    /// <summary>
    /// Get the contents of the file and put them on the queue
    /// </summary>
    /// <param name="path">Path we are recursively searching</param>
    private static void SearchDirectory(string path)
    {
        string[] files = Directory.GetFiles(path);

        //Console.WriteLine("Path: " + path);
        foreach (string file in files)
        {
            fileInformation.Enqueue(new FilePair { Content = File.ReadAllText(file), FilePath = path, FileName = file });
        }

        foreach (string directory in Directory.GetDirectories(path))
        {
            SearchDirectory(directory);
        }
    }
}
}

private static void SearchDirectory(string path)

This method does not need to be recursive. You could use the Directory.EnumerateFiles method

Directory.EnumerateFiles Method (String, String, SearchOption)

where

  One of the (SearchOption) enumeration values that specifies whether the search operation should include only the current directory or should include all subdirectories.

I don't know how your regex looks like but for simple pattern with wildcards you can use the second parameter

  The search string to match against the names of files in path. This parameter can contain a combination of valid literal path and wildcard (* and ?) characters (see Remarks), but doesn't support regular expressions.

As far as the multithreading is concered you should definitely use the new TPL (Parallel.ForEach) instead of creating your own threads.


Writing a faster mutable trie I am implementing a mutable trie. I tested and it produces correct results. However, it is very slow. I benchmark it against plain old Data.Map, which is more than twice as fast.

Full source

So what am I doing wrong? I am expecting a fundamental flaw in the way I write mutable code here and not some micro performance tricks. I assume STVector is fast enough.

module Trie(Trie, empty, insert, member) where

import Data.Char (ord, toUpper)
import Data.Maybe (isJust)
import Control.Monad.ST
import qualified Data.Vector.Mutable as V

-- In a real world scenario, we would probably want to our mutable trie to base
-- on PrimMonad and PrimState so that it can work within both IO and ST. Also,
-- our trie only accepts String keys composed of capitalized English alphabets
-- ([A-Z]+). Values, though, can be of any type. Finally, there should have
-- been a function to retrieve the value given a key, but we omitted it because
-- of laziness (pun intended).
data Trie s a = Trie {
    trieValue :: Maybe a ,
    trieChildren :: V.STVector s (Maybe (Trie s a))
}

toIndex :: Char -> Int
toIndex c = (ord (toUpper c) - ord 'A') `mod` 26

empty :: ST s (Trie s a)
empty = emptyWith Nothing

newChildren :: ST s (V.STVector s (Maybe (Trie s a)))
newChildren = V.replicate 26 Nothing

emptyWith :: Maybe a -> ST s (Trie s a)
emptyWith x = newChildren >>= return . Trie x

insert :: Trie s a -> String -> a -> ST s (Trie s a)
insert = insert' . Just

insert' :: Maybe (Trie s a) -> String -> a -> ST s (Trie s a)
insert' Nothing cs z = do
    node <- empty
    insert node cs z
insert' (Just root@(Trie _ ys)) [c] z = do
    node <- emptyWith (Just z)
    V.write ys (toIndex c) (Just node)
    return root
insert' (Just root@(Trie _ ys)) (c:cs) z = do
    let i = toIndex c
    y <- V.read ys i
    insert' y cs z >>= V.write ys i . Just
    return root

member :: Trie s a -> String -> ST s Bool
member = member' . Just

member' :: Maybe (Trie s a) -> String -> ST s Bool
member' Nothing _ = return False
member' (Just (Trie x _)) [] = return (isJust x)
member' (Just (Trie _ ys)) (c:cs) =
    V.read ys (toIndex c) >>= flip member' cs

The big thing I can see is that you don't seem to know why to use a mutable structure in Haskell. They're not automatically faster. In fact, they have some GC-related overhead that exceeds that of immutable structures in GHC. Mutable structures in GHC are only a win if they can actually reduce the amount of allocation, and this implementation looks like it allocates a lot.

When you consider using a mutable structure, you need to examine how mutability will reduce total allocation. If it doesn't reduce allocation (and usually by an asymptotic factor) it's unlikely that mutability alone is going to help performance.


How to make this special binary-search algorithm more rusty? I am solving one binary search problem in LeetCode.
I have accomplished such code:
struct Solution;
impl Solution {
    pub fn search(nums: Vec<i32>, target: i32) -> i32 {
        if nums.is_empty() {
            return -1;
        }
        if nums.len() == 1 {
            return if nums[0] == target { 0 } else { -1 };
        }
        let mut l = 0;
        let mut r = nums.len() - 1;

        while l <= r {
            let mid = (l + r) / 2 as usize;
            if nums[mid] == target {
                return mid as i32;
            }
            if nums[0] <= nums[mid] {
                if nums[0] <= target && target < nums[mid] {
                    r = mid - 1;
                } else {
                    l = mid + 1;
                }
            } else {
                if nums[mid] < target && target <= nums[n - 1] {
                    l = mid + 1;
                } else {
                    r = mid - 1;
                }
            }
        }
        return -1;
    }
}

I have searched the standard library and found that it has an API: binary_search_by.  But since this is not a traditional binary search algorithm, I can't directly call the API.  I have also googled to try to find a more elegant Rust code but found nothing special.  Is there any idea to make this code rusty?

As always, the interface provided by LeetCode is atrocious.  In order to make your code Rusty, I recommend writing your own interface first and then call it within the implementation of LeetCode's interface.  Here's why.
The full interface of the search function, imposed by LeetCode, is
impl Solution {
    pub fn search(nums: Vec<i32>, target: i32) -> i32 {
        // ...
    }
}

Unfortunately, every single part — no exaggeration here — of the interface can be improved.

struct Solution is completely unnecessary in Rust.  It is much more idiomatic to define search as a free function or a trait method.

The name search does not indicate the assumptions on nums clearly.  I prefer something along the lines of rotated_binary_search.

Since the function only reads the contents of nums, there is no need to take ownership of the argument.  Moreover, the function can be made more generic by taking a slice instead of a Vec — see Why is it discouraged to accept a reference to a String (&String), Vec (&Vec), or Box (&Box) as a function argument?.

The i32s in the types of nums and target aren't unidiomatic per se, but since the same algorithm applies to any type with a total order, a generic type T: Ord is even better.

The return type i32 is problematic for two reasons:

Rust uses usize for indexes, not i32, so returning i32 introduces gratuitous type conversions.

The absence of the target value is signaled by -1, which is error-prone, as the caller may easily forget to check the return value against -1.

Instead, Option<usize> should be used so as to take advantage of the type system.

Here's the improved interface:
pub fn rotated_binary_search<T: Ord>(nums: &[T], target: &T) -> Option<usize> {
    // ...
}

The implementation is of much better quality than the interface.  The last line return -1; can be simplified to -1 (without the semicolon), as the last expression in a function is implicitly the return value.  Then, we can adjust the returns to the new interface — return Some(index) and return None surely read better than -1, right? :)
Now, let's look at the implementation of binary_search (and binary_search_for) in the standard library for inspiration: (ll. 2077–2176, documentation and attributes omitted for brevity)
pub fn binary_search(&self, x: &T) -> Result<usize, usize>
where
    T: Ord,
{
    self.binary_search_by(|p| p.cmp(x))
}

pub fn binary_search_by<'a, F>(&'a self, mut f: F) -> Result<usize, usize>
where
    F: FnMut(&'a T) -> Ordering,
{
    let s = self;
    let mut size = s.len();
    if size == 0 {
        return Err(0);
    }
    let mut base = 0usize;
    while size > 1 {
        let half = size / 2;
        let mid = base + half;
        // SAFETY: the call is made safe by the following inconstants:
        // - `mid >= 0`: by definition
        // - `mid < size`: `mid = size / 2 + size / 4 + size / 8 ...`
        let cmp = f(unsafe { s.get_unchecked(mid) });
        base = if cmp == Greater { base } else { mid };
        size -= half;
    }
    // SAFETY: base is always in [0, size) because base <= mid.
    let cmp = f(unsafe { s.get_unchecked(base) });
    if cmp == Equal { Ok(base) } else { Err(base + (cmp == Less) as usize) }
}

The major difference is the use of a (base, size) pair instead of a (low, high) pair, which greatly simplifies the control flow.  You can also use unsafe to eliminate bound checks, but I wouldn't recommend that except when performance is absolutely critical.


How to enumerate the internal nodes and leaves of a tree more elegantly? Might there be a simpler way to write this code?  I'd like to learn to write code more efficiently.

def leaves_and_internals(self):
    """(BTNode) -> ([object], [object])
    Return a pair of lists: (list of all the items stored in the leaves of
    the subtree rooted at this node, list of all the items stored in the
    internal nodes of the subtree rooted at this node).
    """
    leaves = []
    internal = []

    if self.left is None and self.right is None:
        leaves.append(self.item)
    else:
        internal.append(self.item)
    if self.left:
        subleaf, subinternal = self.left.leaves_and_internals()
        leaves.extend(subleaf)
        internal.extend(subinternal)
    if self.right:
        subleaf, subinternal = self.right.leaves_and_internals()
        leaves.extend(subleaf)
        internal.extend(subinternal)

    return (leaves, internal)

Here's a more efficient version that avoids copying list elements.  Instead, it creates just two empty lists and appends to them during the traversal.

def leaves_and_internals(self):
    """(BTNode) -> ([object], [object])
    Return a pair of lists: (list of all the items stored in the leaves of
    the subtree rooted at this node, list of all the items stored in the
    internal nodes of the subtree rooted at this node), according to a
    pre-order traversal.
    """
    def helper(self, leaves, internal):
        if self.left is None and self.right is None:
            leaves.append(self.item)
        else:
            internal.append(self.item)
            for child in [self.left, self.right]:
                if child is not None:
                    helper(child, leaves, internal)
        return (leaves, internal)
    return helper(self, [], [])


Define local variable in function? I have an object like this

initialize : {
    method : function() {
        this.__. //--> do another thing
        ... //--> so many
        this.__.params. //--> do another thing
        ... //--> so many
    }
}

and the method of this object call by another function.  As you can see, we have many lines that refer to this.__ and this.__. params, and for preventing this, I declare local variables T and P like below

initialize : {
    method : function() {
        var T = this.__, P = T.params;
        T. //--> do another thing
        ... //--> so many
        P. //--> do another thing
        ... //--> so many
    }
}

as I mentioned before the method calls by another function called caller.

Is it possible to not declare locale variables in the method function and instead define them in scope of the caller function and pass it to the method ?

Not much to review ;)

As @Dave mentioned, __ is a bit of an odd named, for sure it does not give away what it does
You might consider storing __.params separately in this.parameters or this.options

I would need more code to give deeper, insightful comments ;)


Is this escape unescape function pair correct? Is this example of an escape - unescape function correct?

ESCAPE = '/'

def escape(s):
    rv = s
    rv = rv.replace(ESCAPE, ESCAPE + ESCAPE)
    rv = rv.replace('+', ESCAPE + 'PLUS')
    rv = rv.replace('-', ESCAPE + 'MINUS')
    return rv

def unescape(s):
    rv = s
    rv = rv.replace(ESCAPE + 'MINUS', '-')
    rv = rv.replace(ESCAPE + 'PLUS', '+')
    rv = rv.replace(ESCAPE + ESCAPE, ESCAPE)
    return rv

Performing string substitutions using multiple passes is almost always a bad idea, not only for performance, but for correctness.  In this case, there is indeed a bug: unescape('//PLUS/PLUS') should produce '/PLUS+', but instead produces '/++'.

Therefore, you have to parse the input string in one pass:

import re

def unescape(s):
    def replacement(match):
        return {
            '/': '/',
            'PLUS': '+',
            'MINUS': '-',
        }[match.group(1)]
    return re.sub('/(/|PLUS|MINUS)', replacement, s)

In your original code and in the implementation above, there is a lot of repetition.  If there are more symbols being escaped than just + and -, then it makes sense to specify the mapping just once.

ESCAPE_CHAR = '/'
ESCAPE = {
    ESCAPE_CHAR: ESCAPE_CHAR,
    '+': 'PLUS',
    '-': 'MINUS',
}
ESCAPE_RE = re.compile('|'.join(map(re.escape, ESCAPE.keys())))

UNESCAPE = dict((v, k) for (k, v) in ESCAPE.items())
UNESCAPE_RE = re.compile(
    re.escape(ESCAPE_CHAR) + '(' +
        '|'.join(map(re.escape, UNESCAPE.keys())) +
    ')')

def escape(s):
    return ESCAPE_RE.sub(lambda match: ESCAPE_CHAR + ESCAPE[match.group(0)], s)

def unescape(s):
    return UNESCAPE_RE.sub(lambda match: UNESCAPE[match.group(1)], s)


Java splitting too large text file enhance performance I have a too large data.txt file containing data as follows:

a = line0
line1
line2
€
b = line0
line1
line2
€
...
z = line0
line1

I am still beginner with java, I wrote this code to split the data.txt into multiple .txt files named a.txt , b.txt etc... it splits the data.txt at each €\n character.

It works perfectly, but it is so slow, I noticed, as if it reads everything at once and makes its mission, then at once the result .txt files are created and showed  at once in the destination folder.

How could I make my code faster, for example when it is finished with the first part then it should create the result a.txt file immediatly then the second, third and so on...
Thank in advance

import java.util.*;
import java.io.File;
import java.io.IOException;
import java.io.*;

public class Main
{
    public static void main(String[] args)
    {
        File file = new File("/path/path2/file.txt");

        try{
            Scanner scanner = new Scanner(file);
            String str = "";

            while(scanner.hasNext()){

                str+=scanner.nextLine()+"\n";

            }

            String charac = "€";
            String end = "end of file";

            for(int i = 0; i < (str.split(charac).length)-1; i++){

                String name = str.split(charac)[i].split(" = ")[0];

                String out= "/path/path2/path3/Folder/"+name+".txt";
                FileWriter fw = new FileWriter(out);
                BufferedWriter bw = new BufferedWriter(fw);

                bw.write(str.split(charac)[i]+"\n"+end);
                bw.close();
            }

            System.out.println("Splitting is finished");

        }
        catch(Exception e){}
    }
}

As said in the comment, you program is slow because you load the full file in memory. To change that you should keep only your first loop where you read but also write and split into files.

while(scanner.hasNext()){
    str+=scanner.nextLine()+"\n";
    if ( mustSplit(str) ) {
        writeToAnotherFile(str);
        str = "";
    }
}


Replace values in R data frame according to values in other data frame I would like to change row names values from a first data frame if these rows names are present in another data frame, and change it to a corresponding value (defined in the second data frame). I could do it with a for loop, but I wonder what would be some other more suitable ways to do it in R.

Here is a toy dataset (my real dataset has 10^5 rows):

df <- data.frame("sample_1" = rep(0,3), "sample_2" = rep(0,3), row.names = paste0('gene_', seq(1, 3)), stringsAsFactors = FALSE)
annotation <- data.frame("gene" =  paste0('gene_', seq(1, 2)), 'name' = paste0('name_', seq(1, 2)), stringsAsFactors = FALSE)
df$gene <- rownames(df)

Initial data frames:

df
       sample_1 sample_2   gene
gene_1        0        0 gene_1
gene_2        0        0 gene_2
gene_3        0        0 gene_3

annotation
    gene   name
1 gene_1 name_1
2 gene_2 name_2

Solution found:

for (x in rownames(df)){
  if (x %in% annotation$gene){
    df[x,]$gene <- annotation$name[which(annotation$gene == x)]
  }
}
rownames(df) <- df$gene
df$gene <- NULL

Resulting data frame:

df
       sample_1 sample_2
name_1        0        0
name_2        0        0
gene_3        0        0

Code Review

One change I would make to your solution is to directly modify the row names as below (instead of modifying gene and then setting the row names equal to gene).

for (i in 1:nrow(df)){
  if (rownames(df)[i] %in% annotation$gene) {

    rownames(df)[i] = annotation$name[which(annotation$gene == rownames(df)[i])]

  }
}
df$gene <- NULL

df
#        sample_1 sample_2
# name_1        0        0
# name_2        0        0
# gene_3        0        0

This solution is similar to the original, but iterates over row numbers (1-3 in this case) instead of row names. This allows direct modification of the row names and makes the code slightly more concise. This version doesn't use gene at all.

That's how I'd modify the existing solution, but overall I wouldn't use a for loop. for loops can be slow since they iterate over every element (and your data set has 10^5 rows). Below is another approach using the dplyr library.

Another Approach

library(dplyr)
library(tibble)

df <- left_join(df, annotation, by = "gene") %>%        # Join "annotation" and "df"
mutate(gene = if_else(is.na(name), gene, name)) %>%     # Convert "gene" to "name" when "name" is valid
column_to_rownames(var = "gene") %>%                    # Set row names to "gene"
select(-name)                                           # Remove "name"

df
#        sample_1 sample_2
# name_1        0        0
# name_2        0        0
# gene_3        0        0

This syntax may look unfamiliar but it's doing 3 main things:
  - left-joining df and annotation by the gene field. It has to be a left-join so all rows of df are preserved!
  - changing the value of gene to name only when name is non-null
- setting the row names equal to gene

The pipe operator %>% passes the data through these operations without needing to repeatedly type df <-. These are all vectorized operations and so are faster than a for loop, especially for large data sets.

dplyr syntax can be intimidating at first, but it's an extremely popular (and I believe intuitive) paradigm to manipulate data. Here is a handy intro on dplyr syntax.


If any two items in a list is equal to a given number I just subscribed to the daily coding problems and received my first today.

The problem is:

  Given a list of numbers and a number k, return whether any two numbers from the list add up to k. For example, given [10, 15, 3, 7] and k of 17, return true since 10 + 7 is 17.

This is what I came up with:

def anyequalto(x, y):
    for i in x:
        if y - i in x:
            return True

anyequalto([10, 15, 3, 7], 17)

I was wondering if anyone had any notes or alternatives as I'm very new to Python and programming.

There's a failing case you might have missed.  If the target number is exactly twice the value of one of the entries, then you'll wrongly return true.

Test case:

anyequalto([5], 10)

Besides that, try to think of a better name for the function.  Perhaps contains_pair_totalling() as a start?


Method for calculating shipping cost I'm trying to optimize the following method. Any suggestions on how this can be done better? The shipping cost is based on weight and the total cost of products. Following are the conditions for calculating cost.

When order is under $100, and all items under 10 lb, then shipping is $5 flat.
When order is $100 or more, and each individual item is under 10 lb, then shipping is free.
Items over 10 lb always cost $20 each to ship.

public function addShippingCost($subtotal)
        {
            $shipCost = 0;
            $totalWeight = 0;
            $itemUnder10lb = true;
            foreach ($this->products as $product) {
                if($product->getWeight() > 10){
                    $shipCost += $this::RATE_HEAVY;
                    $itemUnder10lb = false;
                }
                $totalWeight += $product->getWeight();
            }
            //When order is under $100, and all items under 10 lb, then shipping is $5 flat
            //When order is $100 or more, and each individual item is under 10 lb, then shipping is free
            //Items over 10 lb always cost $20 each to ship

            if($subtotal > 100 && $itemUnder10lb)
                return $this::RATE_FREE;
            elseif($subtotal < 100 && $totalWeight < 10)
                return $this::RATE_FLAT;
            elseif($itemUnder10lb == false)
                return shipCost;
        }

What if you have an order, that is

> 10lb in total weight
< $100 in subtotal
has no product with > 10lb?

Your method would not return anything in that case.

Here's my patched solution:

public function addShippingCost($subtotal) {
    $shipCost = 0;
    $weightSum = 0;
    $allItemsUnder10lb = true;

    foreach ($this->products as $product) {
        if($product->getWeight() > 10){
            $shipCost += $this::RATE_HEAVY;
            $allItemsUnder10lb = false;
        }
        $weightSum += $product->getWeight();
    }

    if ($subtotal < 100 && $weightSum < 10) {
        $shipCost = $this::RATE_FLAT;
    }

    if ($subtotal >= 100 && $allItemsUnder10lb) {
        $shipCost = $this::RATE_FREE;
    }

    return $shipCost;
}


Java String Reverse Algorithm which is the most effcient approch? I have the following two approaches for writing a custom string reverse algorithm and I was hoping someone could tell me which is the most efficient / preferable option and explain why. I am fairly new to Java and this would be educational for me. FYI I know this can be done using Stringbuilder but I wanted to write my own solution.

Solution 1: uses two arrays, copying the contents of one into another but in reverse order

//include needed util
import java.util.Scanner;

 /**
 * This class will reverse a string
 * @author Richard
 *
 */

public class ReverseString {
/**
 * This method will take in a string, convert that to two
 * char arrays, then swap the contents of one for the contents
 * of the other except moving in opposite directions and so
 * will reverse the order of the output string
 * @param args
 */

public static void main(String[] args) {

    //declare variables
    Scanner input = new Scanner(System.in);
    String inputString = "";
    int length = 0, indexOfArrayToChange = 0, indexOfReferenceArray = 0;

    //get input string
    System.out.print("Enter the string you want to reverse: ");
    inputString = input.nextLine();

    //find length of string, it needs to be reduced by one to prevent an error
    //as index start at 0.
    length = inputString.length()-1;
    //set 'indexOfReferenceArray' equal to string.length-1 for use later in loop
    indexOfReferenceArray = length;

    //convert to arrays
    char[] arrayToChange = inputString.toCharArray();
    char[] referenceArray = inputString.toCharArray();

    //loop through and reverse order as long as we are still within bounds of original string length
    while (indexOfArrayToChange<=length) {
        //change the first letter of one array to be the same as the last letter of reference array
        arrayToChange[indexOfArrayToChange] = referenceArray[indexOfReferenceArray];
        //increment/decrement to move through arrays
        indexOfArrayToChange++;
        indexOfReferenceArray--;

    }
    //convert back to string
    String newString = new String(arrayToChange);

    //output result
    System.out.println(newString);

    //close resources
    input.close();
}
}

Solution 2: uses one array and swaps from back to front meeting in the middle. However the code within the loop has to be longer to do the swapping.

//include needed util
import java.util.Scanner;

/**
* This class will reverse an inputed string
* @author Richard
*
*/
public class ReverseString {

/**
 * This method will take in a string, convert that to a
 * char array, then swap the contents of the array from back to front
 * meeting in the middle.
 * @param args
 */
public static void main(String[] args) {
    //declare variables
    Scanner input = new Scanner(System.in);
    String inputString = "";
    int length = 0, indexStart = 0, indexEnd = 0;

    //get input string
    System.out.print("Enter the string you want to reverse: ");
    inputString = input.nextLine();

    //find length of string, it needs to be reduced by one to prevent an error
    //as index start at 0.
    length = inputString.length()-1;
    //set 'indexOfReferenceArray' equal to string.length-1 for use later in loop
    indexEnd = length;

    //convert to arrays
    char[] array = inputString.toCharArray();

    //loop through and reverse order until two vars meet in middle of array
    while (indexStart<=(length/2)) {
        //swap array elements
        char temp = array[indexStart];
        array[indexStart] = array[indexEnd];
        array[indexEnd] = temp;
        //increment/decrement to move through arrays
        indexStart++;
        indexEnd--;
    }

    //convert back to string
    String newString = new String(array);

    //output result
    System.out.println(newString);

    //close resources
    input.close();

}

}

Solution 2 is (probably, that depends on many factors) the more efficient one, as its (slightly more complex) loop body is executed only length/2 times, and it doesn't create two character arrays from the input string.

But besides answering your question, I'd like to comment on your coding style - we're on codereview, anyway.

First, I'd always separate input/output and computation, so you can re-use the computation as a "library function" with different input/output or as part of a bigger computation.

Then, you often declare a variable before you want to use it (and even assign an unnecessary initial value to it). It's accepted style to declare a variable exactly when you need it first, and assign it the value you want.

You "adjusted" the length value by -1 because you need that value for the endIndex. That's misleading, as now you have a variable named length, but it doesn't contain the length value. This adjustment should be done in the endIndex variable, not the length.

It's good that you are using Javadoc comments. But you should document pieces of code from an "outside" view: what the method does, not how it's doing its job. See my re-worded javadoc for the main() method.

As a matter of personal style, I'd replace your while loop with a for loop that uses two variables: in every iteration, it increases indexStart and decreases indexEnd.

See my modified version of your solution 2:

package test;

import java.util.Scanner;

public class ReverseString {

    /**
     * This method will read a String from standard input
     * and print the character-reversed string to standard output.
     * @param args
     */
    public static void main(String[] args) {
        //declare variables
        Scanner input = new Scanner(System.in);
        // Don't introduce variables before you need them
        // String inputString = "";
        // int length = 0, indexStart = 0, indexEnd = 0;

        //get input string
        System.out.print("Enter the string you want to reverse: ");
        String inputString = input.nextLine();

        String newString = reverseString(inputString);

        //output result
        System.out.println(newString);

        //close resources
        input.close();
    }

    /**
     * Returns the character-reversed String of the inputString
     * @param inputString the input String
     * @return the input string in reversed order of the characters
     */
    public static String reverseString(String inputString) {
        //find length of string, it needs to be reduced by one to prevent an error
        //as index start at 0.
        // NO! It's not the length that's wrong, it's the indexEnd
        int length = inputString.length();
        //set 'indexOfReferenceArray' equal to string.length-1 for use later in loop
        // int indexEnd = length;

        //convert to arrays
        char[] array = inputString.toCharArray();

        //loop through and reverse order until two vars meet in middle of array
        // while (indexStart<=(length/2)) {
        // I replaced that with a for loop using two variables
        for (int indexStart=0, indexEnd=length-1; indexStart<indexEnd; indexStart++,indexEnd--) {
            //swap array elements
            char temp = array[indexStart];
            array[indexStart] = array[indexEnd];
            array[indexEnd] = temp;
            //increment/decrement to move through arrays
            // That's done by the for loop, now.
            // indexStart++;
            // indexEnd--;
        }

        //convert back to string
        String newString = new String(array);
        return newString;
    }

}

But I'd write still another version of the program:

package test;

import java.util.Scanner;

public class ReverseString2 {

    /**
     * This method will read a String from standard input
     * and print the character-reversed string to standard output.
     * @param args
     */
    public static void main(String[] args) {
        //declare variables
        Scanner input = new Scanner(System.in);
        // Don't introduce variables before you need them
        // String inputString = "";
        // int length = 0, indexStart = 0, indexEnd = 0;

        //get input string
        System.out.print("Enter the string you want to reverse: ");
        String inputString = input.nextLine();

        String newString = reverseString(inputString);

        //output result
        System.out.println(newString);

        //close resources
        input.close();
    }

    /**
     * Returns the character-reversed String of the inputString
     * @param inputString the input String
     * @return the input string in reversed order of the characters
     */
    public static String reverseString(String inputString) {
        int length = inputString.length();
        char[] array = new char[length];

        for (int iSource=0,iTarget=length-1; iTarget>=0; iSource++,iTarget--) {
            array[iTarget] = inputString.charAt(iSource);
        }

        String newString = new String(array);
        return newString;
    }

}


Can I draw a square with pixels more efficiently? I'm drawing a square on a window using an implementation of Xlib. I put a colored pixel with my_pixel_put at a specific (x,y) coordinate.
Code to review
#define LWST_VAL 200
#define HGHST_VAL 400

int     main()
{
    t_data  img; //image data
    int x = LWST_VAL, y = LWST_VAL;

    // steps creating a window, creating an image
    while (x >= LWST_VAL && x <= HGHST_VAL)
    {
        while ((y > LWST_VAL && y < HGHST_VAL && (x == LWST_VAL || x ==
            HGHST_VAL)) || y == LWST_VAL || y == HGHST_VAL)
        {
            my_pixel_put(&img, x, y, 456);
            y++;
        }
        if (x > LWST_VAL && x < HGHST_VAL && y == (LWST_VAL + 1))
            y = HGHST_VAL;
        else
        {
            y = LWST_VAL;
            x++;
        }
    }
    // steps pushing image to the window, keep the window open
}

Output

Question
Could anything improve the performance of my code ? Or its readability ?
Any feedback appreciated.
Edit
This is the prototype of my_pixel_put :
void    my_pixel_put(t_data *img, int x, int y, int colour)

img is a structure containing data about the image such endiannes, bits per pixel, ...
x and y are the width and height of the window.
colour is the RGB colour of the pixel. (0,0) is actually the top left corner of the window (I know don't ask me why).

Disclaimer
I don't have a working C compiler on my work computer, nor do I have Xlib available to me (I'm also not familiar with it, at all). That being said, given that we know our image is a square, you can do this with a single loop.
Use a simpler algorithm
The basic idea is that we know each corner of our square, and then by stepping through the length of a side, we can draw a pixel on each side of the square per iteration. This way, instead of nested looping, you do it all at once.
// assuming a coordinate system where 0,0 is the bottom-left corner of the image
// starting at each corner, draw the corresponding clock-wise line
for (int pos = LWST_VAL; pos <= HGHST_VAL++pos)
{
    // The bottom line
    my_pixel_put(&img, pos, LWST_VAL, 456);
    // The top line
    my_pixel_put(&img, pos, HGHST_VAL, 456);
    // The left line
    my_pixel_put(&img, LWST_VAL, pos, 456);
    // The right line
    my_pixel_put(&img, HGHST_VAL, pos, 456);
}

Cache behavior
The one thing that occurs to me about this implementation is that you might not do quite as well from a caching perspective - the one benefit of doing things on a side-by-side basis is that you're more likely to be operating on data from the cache instead of from memory. I don't know how &img is actually stored or what my_pixel_put is doing, so its hard to give more concrete advice about this.
If you did encounter caching issues, short of changing things to process side-by-side (or maybe top + bottom in one loop, and left + right in another), there isn't a whole lot to do. A normal technique to handle cache churn is to use blocking to break up the loop. That won't help here, unfortunately - see below for why.
Suppose we know the following (these numbers are made up):

16 lines of data can be held in the cache
4 integers can be held in the cache at the same time
img is a matrix stored as a row-wise vector, and my_pixel_put effectively becomes img[WIDTH * y + x] = 456
img is aligned such that the left-most side of the square represents the start of a cache line (there will be many cache lines to get to the right side)

Each iteration of our loop grabs 4 cache lines - one for each side of the square. The top and bottom will be able to re-use the cache line for the next three values, while the right and left will have to get a new cache line. We then have the following sequence for how many cache lines we get at a time:

4
6
8
10
14
16
etc

The 7th loop iteration would force some of the old data out of the cache. If we were going to be using data besides the border of the square (e.g. by filling it somehow) then it would be worthwhile to operate on 6x6 "blocks" of data, because then everything is in-cache instead of in-memory. Because we aren't using the interior of the square, however, we'll never actually get a benefit to blocking up the operation - most of those cache lines will always go to waste
Parallelization
Another benefit of this is that, if you wanted to parallelize this on a CPU or port it to a GPU, it'll be more straightforward. On a CPU, this is an embarrassingly parallel problem - assuming that my_pixel_put is thread-safe as long as you aren't modifying the same pixel, threading it should be trivial. On a GPU, the lack of conditional operations makes it a breeze as well.
You may want to change your memory access patterns if you go parallel, but additional detail is left as an exercise for the reader. As a hint, for CPU-based parallelism each thread should generally be working on distinct pieces of work to avoid ruining cache coherence.
Looking at your code as written, without algorithm changes
Reviewing your actual code, there are some simple ways that you can improve readability & maintainability without changing the algorithm much.

I don't like your outer while loop - you basically just have a for loop, with some extra weirdness about the top vs bottom line.
I have a very similar beef with your inner while loop - this one is even more obviously just a for loop.
Some of your conditions that you pack into your while should be separate, if only for readability's sake. Several of them are obviously removed by switching to a for loop, while others are better suited as an if statement wrapping your loop.
Your macro names are unnecessarily mangled - its okay to have vowels in your macros
Is it really necessary to have these be macros?

for (int x_position = LWST_VAL; x_position <= HGHST_VAL; ++x_position)
{
    if (x_position == LWST_VAL || x_position == HGHST_VAL) {
        for (int y_position = LWST_VAL; y_position <= HGHST_VAL; ++y_position)
        {
            my_pixel_put(&img, x_position, y_position, 456);
        }
    } else {
        my_pixel_put(&img, x_position, LWST_VAL, 456);
        my_pixel_put(&img, x_position, HGHST_VAL, 456);
    }
}

Whoops, I ended up rewriting it more than I meant to - I just couldn't make myself add another loop just because. To keep an equivalent number of loops, you could do something like this:
for (int side_count = 0; side_count < 2; ++side_count) {
    for (int x_position = LWST_VAL; x_position <= HGHST_VAL; ++x_position)
    {
        if (side_count == 0 && (x_position == LWST_VAL || x_position == HGHST_VAL)) {
            for (int y_position = LWST_VAL; y_position <= HGHST_VAL; ++x_position)
            {
                my_pixel_put(&img, x_position, y_position, 456);
            }
        } else {
            my_pixel_put(&img, x_position, side_count == 0 ? LWST_VAL : HGHST_VAL, 456);
        }
    }
}


Growing potatoes in delayed sequences In order to grow potatoes I'm implementing the retry pattern. If I fail to grow them, I'd like to try it again later. The first part I've implemented is a collection of delay generators.

Their base type is the DelaySequence:

public abstract class DelaySequence : IEnumerable<TimeSpan>
{
    protected DelaySequence(int count) { Count = count; }

    public int Count { get; }

    public IEnumerator<TimeSpan> GetEnumerator() => Generate().Take(Count).GetEnumerator();

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

    protected abstract IEnumerable<TimeSpan> Generate();
}

I derived three concrete generators from it.

One regular:

public class RegularDelaySequence : DelaySequence
{
    private readonly TimeSpan _delay;
    public RegularDelaySequence(TimeSpan delay, int count) : base(count) { _delay = delay; }
    protected override IEnumerable<TimeSpan> Generate()
    {
        while (true) yield return _delay;
    }
}

and two irregular:

public abstract class IrregularDelaySequence : DelaySequence
{
    protected IrregularDelaySequence(int count) : base(count) { }
}

public class FibonacciDelaySequence : IrregularDelaySequence
{
    private readonly Queue<TimeSpan> _lastPair;
    public FibonacciDelaySequence(TimeSpan firstTwo, TimeSpan firstStep, int count) : base(count)
    {
        _lastPair = new Queue<TimeSpan>(new[]
        {
            firstTwo,
            firstTwo,
            firstTwo + firstStep,
        });
    }
    protected override IEnumerable<TimeSpan> Generate()
    {
        foreach (var delay in _lastPair)
        {
            yield return delay;
        }

        _lastPair.Dequeue();

        while (true)
        {
            var current = _lastPair.Aggregate(TimeSpan.Zero, (sum, next) => sum + next);
            yield return current;
            _lastPair.Enqueue(current);
            _lastPair.Dequeue();
        };
    }
}

public class GeometricDelaySequence : IrregularDelaySequence
{
    private TimeSpan _current;
    private readonly Func<TimeSpan, TimeSpan> _increment;
    public GeometricDelaySequence(TimeSpan first, Func<TimeSpan, TimeSpan> increment, int count) : base(count)
    {
        _current  = first;
        _increment = increment;
    }
    protected override IEnumerable<TimeSpan> Generate()
    {
        yield return _current;

        while (true)
        {
            yield return (_current = _increment(_current));
        };
    }
}

Their usage is very simple. Create an instance, specify the parameters and you're ready to go:

new RegularDelaySequence(TimeSpan.FromSeconds(1), 7).Dump();
new FibonacciDelaySequence(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(3), 7).Dump();
new GeometricDelaySequence(TimeSpan.FromSeconds(1), ts => ts + ts, 7).Dump();

Dump() is LINQPad's method

Results:

RegularDelaySequence

00:00:01
00:00:01
00:00:01
00:00:01
00:00:01
00:00:01
00:00:01

FibonacciDelaySequence

00:00:01
00:00:01
00:00:04
00:00:05
00:00:09
00:00:14
00:00:23

GeometricDelaySequence

00:00:01
00:00:02
00:00:04
00:00:08
00:00:16
00:00:32
00:01:04

IrregularDelaySequence

I don't think the base IrregularDelaySequence class is needed here since it doesn't contain any members. To check if a specified instance is an irregular sequence, you could just compare its type to the RegularDelaySequence. If the type isn't RegularDelaySequence, the sequence is definitly irregular.

FibonacciDelaySequence

The logic of this class isn't quite clear for me.
I'd prefer to use an "oldschool" variables instead of the Queue:

public class FibonacciDelaySequence : DelaySequence
{
    private TimeSpan _prev;
    private TimeSpan _cur;

    public FibonacciDelaySequence(TimeSpan firstTwo, TimeSpan firstStep, int count) : base(count)
    {
        _prev = firstTwo;
        _cur = _prev + firstStep;
    }
    protected override IEnumerable<TimeSpan> Generate()
    {
        yield return _prev;
        yield return _prev;
        yield return _cur;

        while (true)
        {
            var current = _prev + _cur;
            yield return current;
            _prev = _cur;
            _cur = current;
        }
    }
}


Finding the number of boxes that can fit into a truck using JavaScript <!DOCTYPE html>
<html>
<script>

  ////////////////////////////
 // Code by Zachary Holmes //
////////////////////////////

// This program determines the number of boxes that can fit into the back of a truck

// Declaring box size in centimetres, and converting it into metres
var boxHeight = prompt("What is the height of your box in centimetres?") / 100;
var boxWidth = prompt("What is the width of your box in centimetres?") / 100;
var boxLength = prompt("What is the length of your box in centimetres?") / 100;

// Declaring truck size in metres
var truckHeight = prompt("What is the height of your truck in metres?");
var truckWidth = prompt("What is the width of your truck in metres?");
var truckLength = prompt("What is the length of your truck in metres?");

// Declaring variables used in equation
var totalHeight = (truckHeight / boxHeight) | 0;
var totalWidth = (truckWidth / boxWidth) | 0;
var totalLength = (truckLength / boxLength) | 0;

// Calculations
var boxNumber = totalHeight * totalWidth * totalLength;

// Output answer
if (boxNumber = 1)
 {
  alert("You can fit up to" + " " + boxNumber + " " + "box into the truck");
 }
else
 {
  alert("You can fit up to" + " " + boxNumber + " " + "boxes into the truck");
 }

</script>
</html>

You have a bug in the output: your whole program always does the equivalent of

alert("You can fit up to 1 box into the truck");

One way to prevent that kind of error is to use "Yoda conditions", which put the constant first:

if (1 == boxCount)

I think boxCount is a slightly better name than boxNumber, which sounds like it could be referring to the nth box.

Your if-else brace style is probably one of the rarest and least favoured ones possible.  The braces, being aligned with neither the outer level nor the inner level, create visual clutter.

I'd actually prefer to use a ternary expression instead of repeating the entire statement:

alert("You can fit up to " +
      boxCount + (1 == boxCount ? " box" : " boxes") +
      " into the truck");

totalHeight, totalWidth, totalLength are misnamed, in my opinion.  You're not totalling anything. Furthermore, the values that you store in them are not lengths (with metres as the unit), but unitless numbers.  I think heightMultiples, widthMultiples, and lengthMultiples would be more appropriate.

In real life, the problem that you are trying to solve is trickier than that.  You have the two options for orienting each box, assuming that the boxes need to stay upright.  If you are allowed to tip boxes over on their side, then you have six possible orientations altogether.  You would have to try all allowable orientations for optimal packing.

On the other hand, if the boxes are strapped to pallets, you may have no choice at all in the orientation, due to the way the forklift needs to enter the truck.

Fortunately, packing rectangular prisms is a relatively simple problem, compared to other objects. ☺


Find minimum node in Linked-List I wrote a method that will return the minimum element in a linked-list that operates in O(1) time. I tested and everything works fine. However, I was wondering if there is anything that I can do to make my code more efficient or cleaner.

public class Node {

    //private Object item;
    Node next;
    Object item;

    // constructor
    public Node(Object item) {
        this.item = item;
        next = null;
    }

}

public class MinElementInStack {

    static Node stack;
    static Node minStack;

    public void push(int data) {

        // push nodes into stack
        Node last = new Node(data);
        last.next = stack;
        stack = last;

        // check if node being pushed is new min
        // if it is then push into minStack
        if(minStack == null) { // for first node
            Node toBePushed = new Node(data);
            toBePushed.next = minStack;
            minStack = toBePushed;
        } else { // if int to be pushed is less than current min on stack
                // then push it onto the stack, otherwise push the same min value from previous
            int toBePushedInt = data;
            if(toBePushedInt < (Integer)minStack.item) {
                Node toBePushed = new Node(data);
                toBePushed.next = minStack;
                minStack = toBePushed;
            } else {
                Node toBePushed = new Node((Integer)minStack.item);
                toBePushed.next = minStack;
                minStack = toBePushed;
            }
        }

    }

    public void pop() {
        if (stack != null && minStack != null) {
            stack = stack.next;
            minStack = minStack.next;
        }
    }

    public int min() {

        int min = (int) minStack.item;
        return min;

    }

    public void print(Node node) {
        Node temp = node;
        while (temp != null) {
            System.out.println(temp.item);
            temp = temp.next;
        }
    }

    public static void main(String[] args) {

        MinElementInStack s = new MinElementInStack();

        s.push(5);
        s.push(-28);
        s.push(8);
        s.push(-50);
        s.push(-4);
        s.push(-38);
        s.push(100);

        System.out.println("Original Stack: ");
        s.print(stack);
        System.out.println();
        System.out.println("Min Stack: ");
        s.print(minStack);
        System.out.println();
        System.out.println("Minimum element in current stack: " + s.min());
    }

}

Since your min() in your implementation returns an int, I would modify the type of Node.item to int.
Since we are dealing with a stack of (primitive) integers, I would rename MinElementInStack to IntStack.
Next, why not rename Node to IntStackNode?
Both the fields in Node can be specified as private final + you could add getters for the two fields.
You could define Node as an inner static class in your actual stack class.
pop() should return the integer datum associated with the popped node.
Both pop() and min() should throw EmptyStackException whenever the stack is empty.
I would add the field size to the stack, and the methods size() and isEmpty() which rely on the field size.
Instead of print(), I would override public String toString().
You are overkilling it in the push, mainly due to the fact that you create the new node more than once, and rename data to toBePushedInt.

Summa summarum

All in all, I had this in mind:

import java.util.EmptyStackException;
import java.util.Scanner;

public class IntStack {

    private static final String STRING_BEGIN = "[";
    private static final String STRING_END   = "]";
    private static final String SEPARATOR    = ", ";

    private static final class IntStackNode {

        private final int datum;
        private final IntStackNode previous;
        private IntStackNode previousMinimum;

        IntStackNode(final int datum, final IntStackNode previous) {
            this.datum = datum;
            this.previous = previous;
        }

        int getDatum() {
            return datum;
        }

        IntStackNode getPreviousNode() {
            return previous;
        }

        IntStackNode getPreviousMinimum() {
            return previousMinimum;
        }

        void setPreviousMinimum(final IntStackNode previousMinimum) {
            this.previousMinimum = previousMinimum;
        }
    }

    private IntStackNode top;
    private IntStackNode minimum;
    private int size;

    public void push(final int datum) {
        IntStackNode newnode = new IntStackNode(datum, top);

        if (isEmpty()) {
            minimum = newnode;
        } else if (minimum.getDatum() > datum) {
            newnode.setPreviousMinimum(minimum);
            minimum = newnode;
        }

        top = newnode;
        ++size;
    }

    public int pop() {
        if (top == null) {
            throw new EmptyStackException();
        }

        final IntStackNode ret = top;

        if (minimum == ret) {
            minimum = ret.getPreviousMinimum();
        }

        top = top.getPreviousNode();
        --size;
        return ret.datum;
    }

    public int min() {
        if (isEmpty()) {
            throw new EmptyStackException();
        }

        return minimum.getDatum();
    }

    public int size() {
        return size;
    }

    public boolean isEmpty() {
        return size == 0;
    }

    @Override
    public String toString() {
        IntStackNode node = top;

        if (top == null) {
            return STRING_BEGIN + STRING_END;
        }

        final StringBuilder sb = new StringBuilder(STRING_BEGIN);
        sb.append(top.getDatum());
        node = top.getPreviousNode();

        for (int i = 1; i < size; ++i) {
            sb.append(SEPARATOR).append(node.getDatum());
            node = node.getPreviousNode();
        }

        return sb.append(STRING_END).toString();
    }

    public static void main(final String[] args) {
        final IntStack stack = new IntStack();
        final Scanner scanner = new Scanner(System.in);

        loop:
        while (true) {
            System.out.print("> ");

            final String command = scanner.next().trim().toLowerCase();

            switch (command) {
                case "push":
                    doPush(stack, scanner);
                    break;

                case "pop":
                    doPop(stack);
                    break;

                case "print":
                    System.out.println(stack);
                    break;

                case "min":
                    doMin(stack);
                    break;

                case "quit":
                case "exit":
                    break loop;

                default:
                    System.out.println("Unknown command: \"" + command + "\"");
            }
        }
    }

    private static void doPush(IntStack stack, Scanner scanner) {
        int datum;

        try {
            datum = scanner.nextInt();
        } catch (NumberFormatException ex) {
            System.out.println("ERROR: An integer is required.");
            return;
        }

        stack.push(datum);
    }

    private static void doPop(IntStack stack) {
        try {
            stack.pop();
        } catch (EmptyStackException ex) {
            System.out.println("ERROR: Popping from an empty stack.");
        }
    }

    private static void doMin(IntStack stack) {
        try {
            System.out.println(stack.min());
        } catch (EmptyStackException ex) {
            System.out.println(
                    "ERROR: Asking for minimum element from an empty stack.");
        }
    }
}

Hope that helps.


ExitCode class and derived Status class able to return a value I was thinking about an ExitCode concept in a strict nothrow environment and came up with the following two classes and would like to get some peer review on what flaws they have.
They are very simple in terms of inner complexity but are quite complicated in terms of language features (at least for me :) )

ExitCode is a class encapsulating an arbitrary state and a status message.

#include <string>

class ExitCode
{
public:
    enum class TState
    {
        OK,
        FAILED,
        CORRECTED
    };

    /**
    * Default failing exit code, can be created from a simple string
    */
    ExitCode(const std::string& msg = "", const TState state = TState::FAILED)
        : m_sMessage(msg), m_state(state)
    {
    }

    ExitCode(const TState state) : m_state(state)
    {
    }

    /**
    * Uses nested exit code message and adds it to the outer message
    */
    ExitCode(const std::string& msg, const TState state, const ExitCode& nested) :
        m_sMessage(msg + ": " + nested.m_sMessage), m_state(state)
    {
    }

    ExitCode(ExitCode&& other) // Note: VS 2012 has no noexcept
    {
        m_state = other.m_state;
        m_sMessage = std::move(other.m_sMessage);
    }

    ExitCode& operator=(ExitCode&& other) // Note: VS 2012 has no noexcept
    {
        if (this != &other)
        {
            m_state = other.m_state;
            m_sMessage = std::move(other.m_sMessage);
        }

        return *this;
    }

    const std::string& GetMessage() const
    {
        return m_sMessage;
    }

    TState GetState() const
    {
        return m_state;
    }

private:

    std::string m_sMessage;
    TState m_state;
};

Simple cases could be

ExitCode foo1("All failed");

ExitCode foo2("All good", ExitCode::TState::OK);

ExitCode root("All good, but was", ExitCode::TState::OK, foo);

And a derived Status<T> class which is also able to provide some return value. The idea is that some method X may or may not return a valid output but at the same time may or may not return an error (which should not be propagated via exceptions)

template <typename TResult>
class Status : public ExitCode
{
public:

    // Converting
    Status( const TResult& result, const std::string & msg = "", const TState state = TState::OK )
        : ExitCode( msg, state ), m_result( result )
    {
    }

    // Converting move
    Status( TResult&& result, const std::string & msg = "", const TState state = TState::OK )
        : ExitCode( msg, state ), m_result( std::move( result ) )
    {
    }

    // Converting
    Status( const std::string& msg = "", const TState state = TState::FAILED )
        : ExitCode( msg, state )
    {
    }

    TResult& GetResult() const
    {
        return m_result;
    }

private:

    TResult m_result;
};

Some usage could be

Status<int> s1 = 23;

Status<int> corrected( 1, "Failed to produce valid output, but corrected something", ExitCode::TState::CORRECTED );

Status<int> s2 = "This was bad";

Are there any obvious bad design decisions in such an implementation?

I am currently aware of one: Multiple implicit string conversion when I write code like that:

Status<std::string> status = "Which constructor to use?";

Overall I would say this is good code. Still, there are a few things that you should (or, at least, could) improve:

TResult& GetResult() const is a bug. This should read TResult const& GetResult() const (and if you had done more testing, your compiler likely should have told you).
You are missing an include (#include <utility>) for std::move.
Depending on how you intend to use these classes, you should add a virtual destructor to ExitCode. The reason is that, if you do fancy polymorphism and try to delete an instance of Status through a pointer to ExitCode, you get undefined behavior. If you don't care about the possible polymorphism, you should be fine as-is.
I share your worry about implicit conversions. I would consider making your constructors explicit.
Keep your formatting consistent. Although your formatting is almost always the same, there is one place where you diverge from it, namely const std::string & msg = "" in the first two constructors of Status. Why do you have a space between std::string and &? (I suppose it's just some oversight of yours).
Why is the default value to two of the constructors of Status for the parameter state TState::OK, but the last constructor has TState::FAILED instead? This is very likely to cause confusion.


VBA: Calling one sub repeatedly and scope of variables My code as a whole works for me without issues, but what I want to improve is a small part right out of the middle of it. I hope it's ok to post a simplified snippet here that just shows the structure (it's not something you can copy paste and it works), but I'll try to explain what it is about. If this is against the rules, let me know.

I have a workbook that functions as a database (wbDB). I look for the number of an object (objectNumber) and, if found, add it to a sheet (wsTest) in another workbook via Sub AddObject.

Option Explicit

Public Const fRowType As Long = 4
Public Const fRowClosing As Long = fRowType + 1
Public Const fRowLoan As Long = fRowType + 4
'[there are about 30 of those fRow variables; they are needed in another _
 module, that's why they are public]

Dim wbDB As Workbook
Dim wsDB As Worksheet, wsTest As Worksheet

Dim lColTest As Long

Sub AddObject(ByVal objectNumber As String, ByVal rngObjects As Range)
    Const colType As String = "A"
    Const colClosing As String = "BN"
    Const colLoan As String = "CD"
    '[same as fRows, I have over 30 of these "col"-variables]

    Dim rowObject As Long
    Dim foundCell As Range

    Set wbDB = Application.Workbooks("Database.xlsx")
    Set wsDB = wbDB.Sheets(2)
    Set wsTest = ActiveSheet

    '---Find object in rngObjects and set rowObject to respective row
    Set foundCell = rngObjects.Find(What:=objectNumber, LookAt:=xlWhole)
    If foundCell Is Nothing Then
        MsgBox "Object " & objectNumber & "wasn't found and will be skipped."
        Exit Sub
    Else
        rowObject = foundCell.Row
    End If

    lColTest = wsTest.Cells(12, wsTest.Columns.Count).End(xlToLeft).Column
'Type
    Call ImportData(rowObject, colType, fRowType)
'Closing
    Call ImportData(rowObject, colClosing, fRowClosing)
'Loan
    Call ImportData(rowObject, colLoan, fRowLoan)
'[again, there are 30+ of these calls]

End Sub

Sub ImportData(ByVal rowDB As Long, ByVal colDB As String, ByVal fRowSection As Long)
    wsTest.Cells(fRowSection, lColTest + 1).Value = wsDB.Cells(rowDB, colDB).Value
End Sub

If you look at Sub ImportData: It requires lColTest, a module-wide variable, which I initialize in Sub AddObject. On the other hand, rowObject is needed just as often, but I put it as one of the parameters of ImportData. What is better practice? From what I've read is that the wider the scope of a variable, the less desirable it is. However, if it is one of the parameters, I currently copy paste it about 30 times with every Call and every additional parameter makes it a little less readable too.

My idea as the "cleanest" solution would be: put it in as parameter and instead of typing out every Call, I loop through it. Would you agree with that? If the answer is yes, I'd need an array or something similar.

What's the best option here since I need always need a pair of values (column and first row)?
Is there an efficient or readable way to fill the array with all these variables?

Public Const fRowType As Long = 4
Public Const fRowClosing As Long = fRowType + 1
Public Const fRowLoan As Long = fRowType + 4
'[there are about 30 of those fRow variables; they are needed in another _
 module, that's why they are public]

First of all, they're not "variables" they're "constants".

Second, that's screaming for an array or scripting.dictionary to hold them instead of a hoard of Const. They could even be pulled into a class that can easily be initialized provided with Getter properties (if the values are as fixed as Const would indicate) then the class can be passed around wherever it's needed.


Counting sort review I've just wrote a program in C# for counting sort.

space complexity is O(N+K)
time complexity is O(N+K)

I want reviews on my code and any better ways to implement this.

namespace SortingSelfCoded
{
    public static class CountingSorter
    {
        public static int[] CountingSort(int [] input)
        {
            // O(1)
           int max = input[0];

            // O(N)
            for (int i = 0; i < input.Length; i++)
            {
               if (input[i] > max)
                {
                    max = input[i];
                }
            }

            // Space complexity O(N+K)
            int[] counts = new int[max + 1];
            int[] output = new int[input.Length];

            // O(N)
            for (int i = 0; i < input.Length; i++)
            {
                counts[input[i]]++;
            }

            int j=0;
            // O(N+K)
            for (int i = 0; i < counts.Length; i++)
            {
                while (counts[i] > 0)
                {
                    output[j] = i;
                    j++;
                    counts[i]--;
                }
            }

            return output;
        }
    }
}

From a practical use point of view:

I'd use input.Max() (LINQ) to obtain the maximum. Less code to write and does the same.
While it is ok to use single letter variables for loop counter j is not really a loop counter. It's the index of the current element so I would rename j into currentIndex.
Consider changing your last while loop into a for loop. This avoids mutating the state of the count array (saves you a read and write on every iteration):

for (int k = 0; k < count[i]; k++)
{
     output[currentIndex++] = i;
}

Even on a 64 bit system the size of an array is limited to 2GB. So if your largest key is in the vicinity of Int32.Max / 4 or larger then you will get an OutOfMemoryException. Depending what else you are doing in your program you will get it much earlier. If you are experimenting around with sorting algorithms then there are some interesting approaches reducing the key space.


What is an optimal way of not repeating myself in the following code? Following code works and i am simply looking for ways to not repeat myself as well keep to code clean and readable. Would love to see what methods that can be utilized to make it prettier.

const util = require("util");
const fs = require("fs");
const reader = util.promisify(fs.readFile);
const simpleParser = require('mailparser').simpleParser;

async function readFile(filepath) {
    try {
        return await reader(filepath)
    } catch (err) {
        return err
    }
};

async function getAttachment(filepath) {
    let file, email;
    try {
        file = await readFile(filepath);
    } catch (err) {
        return err
    }

    try {
        email = await simpleParser(file)
        return email.attachments[0].content.toString()
    } catch (err) {
        return err
    }
}

async function getFileName(filepath) {
    let file, email;
    try {
        file = await readFile(filepath);
    } catch (err) {
        return err
    }

    try {
        email = await simpleParser(file)
        return email.attachments[0].filename.toString()
    } catch (err) {
        return err
    }
}

async function main() {
    let attachment = await getAttachment('./mock-data/google-report.email')
    let filename = await getFileName('./mock-data/google-report.email')
    console.log(filename)
}

main()

module.exports = {
    getAttachment
}

DRY review

You don't need to wrap try catch around async function or promise. The errors will all get passed onto the final promise.catch. So can remove all 5 try catches.

The function readFile is just wrapping a promise and is completely redundant.

The two functions getAttachment and getFileName are almost identical apart from the final return.

The two file loads are of the same file that need only be done once.

filePath as a string is defined twice, again only once is best.

All you want is attachments[0] of the one email. So you don't need to store file and email.

You use util.promisify and fs.readFile only once so no need to hold a reference for either.

Assigning the variables attachment and filename can be done via destructuring saving another token.

You are creating a module so keep variable names short, use the context to infer meaning.

The end result is
const reader = require("util").promisify(require("fs").readFile);
const parser = require('mailparser').simpleParser;

async function main(file) {
    const atts = (await parser(await reader(file))).attachments[0];
    const [attachment, filename] = [atts.content.toString(), atts.filename.toString()];
    console.log(filename);
}
main("./mock-data/google-report.email").catch(err => /*do something with error*/ );


Determine base URL of PHP app I have this little piece of code which determines the base URL of PHP:

$root = $_SERVER["SERVER_NAME"];
if(basename(__FILE__) !== trim($_SERVER["PHP_SELF"], "/\\")) {
    $root .= "/" . basename(__DIR__);
}

This is working for me, but are there other solutions for this "problem," or are there any cases where this would fail?

Your code will work for one level deep working directories such as:

servername/admin/index.php
servername/sample/test.php

However, go one level deeper and it will not work. If I have servername/one/two/index.php, I will get a servername/two. It is because the basename() returns the working folder of the file.

I find this code useful for common applications:

$request = explode('/', $_SERVER['SCRIPT_NAME']);
array_pop($request);
echo $_SERVER['SERVER_NAME'] . implode('/', $request);

It makes use of server's script name. Given the sample servername/one/two/index.php, it will return /one/two/index.php. We then remove the script from the directory using explode and popping the result, and then finally concatenating it to the server name.


Arduino code for multi button state change detection I created state change detection for multiple buttons, which is intended to run on my Arduino for a touchpad project. In a nutshell, the code makes sure that the Arduino only reacts to button presses and not to holding in the buttons.

My code:

unsigned int buttonStates[] = {0, 0, 0};
unsigned int previousButtonStates[] = {0, 0, 0};
//The Arduino pins for detecting a button press.
const unsigned int buttonPins[] = {A3, A4, A5};

void setup() {
    //Start communication with the serial monitor (console).
    //9600 is the data transfer speed of USB.
    Serial.begin(9600);
    //Wait until the serial port is open, before continuing.
    while(!Serial);
    //Defining the buttons as input.
    for (int i = 0; i < 3; i++) {
    pinMode(buttonPins[i], INPUT);
  }
}

void buttonStateChangeDetect() {
  for (int i = 0; i < 3; i++) {
    //Read the button states.
    buttonStates[i] = digitalRead(buttonPins[i]);
    //Determines if a button has been released after a press.
    if (buttonStates[i] != previousButtonStates[i]) {
      //Checks if the button is pressed. If pressed, the state will change from 0 to 1.
      if (buttonStates[i] == HIGH) {
        //The variable i represent a button. 0 is the first button.
        if (i == 0) {
          Serial.println("Button 1 is pressed.");
        }
        else if (i == 1) {
          Serial.println("Button 2 is pressed.");
        }
        else if (i == 2) {
          Serial.println("Button 3 is pressed.");
        }
      }
    }
    previousButtonStates[i] = buttonStates[i];
    delay(10);
  }
}

void loop() {
  buttonStateChangeDetect();
}

Currently, the code works fine but I still see some flaws in my implementation:

A lot of if-statements need to be added if the Arduino uses more buttons.
Example:if(i==3), if (i==4), if(i==5)..
The responsiveness of the buttons is dependent on the number of buttons.
Example: When 30 buttons are used, the for loop will have to loop 30 times and it could be that a button press isn't registered. The higher the index of the button, the higher the chance is that a button press will not be registered or it will take longer to register.

The only idea that came to my mind for improving my code was instead of using if-statements, I could use switch cases. Any other suggestions would be more than welcome.

Extra detail:
The official documentation with a example and my inspiration for writing this code.
Arduino - StateChangeDetection
I created an online example with code. No need for physical hardware if someone wants to try out my code.
Online Arduino Simulation

Not a review, but an extended comment.

This is a very common problem. Polling the bunch of event sources is easy to implement, but it leads to the flaws you've discoverd. The only way out is to abandon polling, and switch to interrupts. In your case, consider to

    attachInterrupt(digitalPinToInterrupt(pin), ISR, RISING);

for each pin you are interested in, during setup. The ISR shall push a pin which triggered it into a queue. Then the loop would pick the pin numbers from the queue, and do whatever is relevant.

Be careful with synchronizing the queue between the ISR and the main line. Solving the synchronization is hard, but very enlightening.


Validating a Collection of IPv4s I have a method within a class that validates all proxy addresses are valid on initialization. I've tried to refactor this a bit, but this seems to be the best I can do. Any suggestions and/or tips as to how I can possibly improve this method?

def valid?(proxies)
  return false if !proxies.is_a?(Array) || proxies.empty?
  ipv4_address = /^((\d{1,3}[\.]){3}\d{1,3}:\d+)$/
  proxies.each do |proxy|
    return false if proxy !~ ipv4_address
    octets = proxy.scan(/\d+/).map(&:to_i).first(4)
    return false unless octets.all? { |octet| octet.between?(0, 255) }
  end
  true
end

The only simplification I can think of is using a bit more complex regex to match the IP address without having to iterate over the octets:

def valid?(proxies)
  return false if !proxies.is_a?(Array) || proxies.empty?
  ipv4_address = /^((25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)\.){3}(25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d):\d+$/
  proxies.each do |proxy|
    return false if proxy !~ ipv4_address
  end
  true
end

This regex is based on the one in the ipaddress package.


Display PHP Menu Stored in an Array and Looped I'm creating a PHP website for a non-profit. They have some restrictions (no MySQL or pre-installed CMS) so I'm creating a CSS menu displayed by an unordered list where all of the elements are stored in a multi-dimensional array.

I've gotten it to work but being new to PHP, I'm certain it's not optimized to run as smoothly as possible. Also, I feel I've hacked my way through the first and last elements.

Here's the array:

<?php
$siteURL = "http://www.mysite.org/";

$menu = array
   (
   //"Name","URL",ID,Level,Seq,Under,Target,Display
   //First Menu
   array("TOP1",$siteURL."top1.php",1,1,1,0,0,true),
   array("SUB1a",$siteURL."sub1a.php",2,2,1,1,0,true),
   array("SUB1b",$siteURL."sub1b.php",3,2,2,1,0,true),
   array("SUB1c",$siteURL."sub1c.php",4,2,3,1,0,true),
   array("SUB1d","http://www.externalsite.org/",5,2,4,1,1,true),
   array("SUB1e","http://www.externalsite.org/",6,2,5,1,1,true),
   array("SUB1f",$siteURL."sub1f.php",7,2,6,1,0,true),

   //Second Menu
   array("TOP2",$siteURL."top2.php",8,1,2,0,0,true),
   array("SUB2a","http://www.externalsite.org/",9,2,1,8,1,true),
   array("SUB2b",$siteURL."sub2b.php",10,2,2,8,0,true),
   array("SUB2c","http://www.externalsite.org/",11,2,3,8,1,true),
   array("SUB2d","http://www.externalsite.org/",12,2,4,8,1,true),

   //Third Menu
   array("TOP3",$siteURL."top3.php",13,1,3,0,0,true),
   array("SUB3a","http://www.externalsite.org/",14,2,1,13,1,true),
   array("SUB3b","http://www.externalsite.org/",15,2,2,13,1,true),
   array("SUB3c","http://www.externalsite.org/",16,2,3,13,1,true),
   array("SUB3d","http://www.externalsite.org/",17,2,4,13,1,true),

   //Fourth Menu
   array("TOP4",$siteURL."top4.php",18,1,4,0,0,true),
   array("SUB4a",$siteURL."downloads/sub4a.pdf",19,2,1,18,0,true),
   array("SUB4b",$siteURL."sub4b.php",20,2,2,18,0,true),

   //Fifth Menu
   array("TOP5",$siteURL."top5.php",21,1,5,0,0,true),

   //Sixth Menu
   array("TOP6",$siteURL."top6.php",22,1,6,0,0,false),

   //Seventh Menu
   array("TOP7",$siteURL."top7.php",23,1,7,0,0,false),

   //Eighth Menu
   array("TOP8","http://www.externalsite.org/",24,1,8,0,1,true),

   );
?>

And here's the logic:

<?php
$count = count($menu);
$i = 0;
while ($i < $count) {
    if (($menu[$i][3]===1) && ($menu[$i][7]===true)) {
        if ($menu[$i][6]===1) {
            echo "
".$menu[$i][0]."
\n";
        }
        else {
            echo "
".$menu[$i][0]."
\n";
        }
        for ($j = 0; $j < $count; $j++){
            if (($menu[$j][5]===$menu[$i][2]) && ($menu[$j][7]===true)) {
                if ($menu[$j][6]===1) {
                    echo "
".$menu[$j][0]."
\n";
                }
                else {
                    echo "
".$menu[$j][0]."
\n";
                }
            }
        }
        echo '

';
    }
    $i++;
}
?>

I had a hard time finding a relevant post. I'd appreciate some help or even just validation that I'm doing this correctly.

I'm not sure for optimization but for readability and maintenance I would use a Class and create the menu items as objects instead of multi-dimension arrays.

class MenuItem {

  protected $id;
  protected $name;
  protected $url;
  protected $level;
  protected $seq;
  protected $under;
  protected $target;
  protected $display;

  public function __construct($name, $url, $id, $level, $seq, $under, $target, $display) {
    $this->id = $id;
    $this->name = $name;
    $this->url = $url;
    $this->level = $level;
    $this->seq = $seq;
    $this->under = $under;
    $this->target = $target;
    $this->display = $display;
  }

  public function getId() {
    return $this->id;
  }

  public function getName() {
    return $this->name;
  }

  public function getUrl() {
    return $this->url;
  }

  public function getLevel() {
    return $this->level;
  }

  public function getSeq() {
    return $this->seq;
  }

  public function getUnder() {
    return $this->under;
  }

  public function getTarget() {
    return $this->target;
  }

  public function getDisplay() {
    return $this->display;
  }

}

And so your creation code would become:

$siteURL = "http://www.mysite.org/";

$menu = array
   (
   //"Name","URL",ID,Level,Seq,Under,Target,Display
   //First Menu
   new MenuItem("TOP1",$siteURL."top1.php",1,1,1,0,0,true),
   new MenuItem("SUB1a",$siteURL."sub1a.php",2,2,1,1,0,true),
   new MenuItem("SUB1b",$siteURL."sub1b.php",3,2,2,1,0,true),
   new MenuItem("SUB1c",$siteURL."sub1c.php",4,2,3,1,0,true),
   new MenuItem("SUB1d","http://www.externalsite.org/",5,2,4,1,1,true),
   new MenuItem("SUB1e","http://www.externalsite.org/",6,2,5,1,1,true),
   new MenuItem("SUB1f",$siteURL."sub1f.php",7,2,6,1,0,true),

   //Second Menu
   new MenuItem("TOP2",$siteURL."top2.php",8,1,2,0,0,true),
   new MenuItem("SUB2a","http://www.externalsite.org/",9,2,1,8,1,true),
   new MenuItem("SUB2b",$siteURL."sub2b.php",10,2,2,8,0,true),
   new MenuItem("SUB2c","http://www.externalsite.org/",11,2,3,8,1,true),
   new MenuItem("SUB2d","http://www.externalsite.org/",12,2,4,8,1,true),

   //Third Menu
   new MenuItem("TOP3",$siteURL."top3.php",13,1,3,0,0,true),
   new MenuItem("SUB3a","http://www.externalsite.org/",14,2,1,13,1,true),
   new MenuItem("SUB3b","http://www.externalsite.org/",15,2,2,13,1,true),
   new MenuItem("SUB3c","http://www.externalsite.org/",16,2,3,13,1,true),
   new MenuItem("SUB3d","http://www.externalsite.org/",17,2,4,13,1,true),

   //Fourth Menu
   new MenuItem("TOP4",$siteURL."top4.php",18,1,4,0,0,true),
   new MenuItem("SUB4a",$siteURL."downloads/sub4a.pdf",19,2,1,18,0,true),
   new MenuItem("SUB4b",$siteURL."sub4b.php",20,2,2,18,0,true),

   //Fifth Menu
   new MenuItem("TOP5",$siteURL."top5.php",21,1,5,0,0,true),

   //Sixth Menu
   new MenuItem("TOP6",$siteURL."top6.php",22,1,6,0,0,false),

   //Seventh Menu
   new MenuItem("TOP7",$siteURL."top7.php",23,1,7,0,0,false),

   //Eighth Menu
   new MenuItem("TOP8","http://www.externalsite.org/",24,1,8,0,1,true),

   );

Then you can access values in a more readable way. E.g.

foreach ($menu as $menuItem) {
  if (($menuItem->getLevel() === 1) && ($menuItem->getDisplay() === true)) {
    ...
  }
}