Parsing Binary Tree Array in Python I got this from a practice problem set, and looking for feedback on my approach.

  Suppose you're given a binary tree represented as an array. Write a function that determines whether the left or right branch of the tree is larger. The size of each branch is the sum of the node values. The function should return the string "Right" if the right side is larger and "Left" if the left side is larger. If the tree has 0 nodes or if the size of the branches are equal, return the empty string.

My logic is that each "breadth" of the tree is 2^depth wide, and we start the work on the second "depth". I can use that to establish how to slice the array to get each "breadth" out of the array (ie, first round is arr[1:3], second is arr[3:7], etc). From there I sum the left/right slices of that slice.

I think my solution uses \$O(n)\$ time and \$O(1)\$ space and should handle unbalanced trees, but parsing trees as arrays is new to me, so wondering about edge cases I should be considering.

def solution(arr):
    breadth = 2
    floor = 1
    left = 0
    right = 0

    while floor < len(arr):
        level = arr[floor:(floor+breadth)]
        pivot = breadth / 2

        left += sum(level[:pivot])
        right += sum(level[pivot:])

        floor += breadth
        breadth *=2

    if left < right:
        return 'Right'
    elif right < left:
        return 'Left'
    else:
        return ''

import unittest

class Test(unittest.TestCase):

    def test_function(self):
        self.assertEqual(solution([3,6,2,9,-1,10]), 'Left')
        self.assertEqual(solution([1,4,100,5]), 'Right')
        self.assertEqual(solution([1,10,5,1,0,6]), '')
        self.assertEqual(solution([]), '')
        self.assertEqual(solution([1]), '')

t = Test()
t.test_function()

One alternate I can think of is evaluating each value. Could this be better than my current solution?

for i in range(len(level)):
    if i < pivot:
        left += level[i]
    else:
        right += level[i]

I think the solution you have is likely to be optimal (or close enough) in terms of runtime, and I think your approach of summing over the slice is much better than doing a for loop.

The only downside of this approach is that it's a little hard to grok, and consequently difficult to extend or maintain, because the approach you're taking is so specific to the implementation of the tree and to the particular task you're doing.  In other words, this code would probably not be reusable for a different task that involved traversing the same tree, or for applying the same task to a different tree representation.

If efficiency is of paramount importance, there's a place for tightly optimized code, but in that case I'd recommend having some explanatory comments to speed the reader's comprehension.  Something with a diagram of the tree layout, say:

# Our tree:
#                      arr[0]                      <-  breadth 1, floor 0
#          arr[1]                  arr[2]          <-  breadth 2, floor 1
#    arr[3]      arr[4]      arr[5]      arr[6]    <-  breadth 4, floor 3

# Begin at the second level (top of the left and right subtrees).
breadth = 2  # number of nodes on the current level
floor = 1    # index of the first node on the current level
left = 0     # sum of the left subtree up to this point
right = 0    # sum of the right subtree up to this point

This is the diagram I built in my head when I was reading the code; having it in front of me already would have saved me a couple of minutes.  With that diagram and the explanations of the different values, it's a lot easier to read through the code and visualize what the slices correspond to.

An entirely different direction to go in for this problem would be to build an abstraction that lets you traverse the tree in a more tree-like way:

from typing import List, NewType, Optional

def solution(arr: List[int]) -> str:
    """Takes a binary tree packed into an array root-first.
    Returns 'Right' or 'Left' to indicate which subtree is larger."""

    # Helpers to read and traverse nodes in the tree.
    # The internal representation of our Node is simply an array index, but
    # these helper functions expose derived "properties" as if it were an object.
    Node = NewType('Node', int)

    def make_node(index: int) -> Optional[Node]:
        return Node(index) if index < len(arr) else None

    def left_child(node: Node) -> Optional[Node]:
        return make_node(node * 2 + 1)

    def right_child(node: Node) -> Optional[Node]:
        return make_node(node * 2 + 2)

    def root_node() -> Optional[Node]:
        return make_node(0)

    def value(node: Node) -> int:
        return arr[node]

    # Recursive function to sum the subtree under a node.
    def sum_subtree(node: Optional[Node]) -> int:
        if node is None:
            return 0
        return (
            value(node)
            + sum_subtree(left_child(node))
            + sum_subtree(right_child(node))
        )

    # Get sums of left and right subtrees.
    root = root_node()
    if root is None:
        return ''
    left_sum = sum_subtree(left_child(root))
    right_sum = sum_subtree(right_child(root))

    if right_sum > left_sum:
        return 'Right'
    elif left_sum > right_sum:
        return 'Left'
    else:
        return ''

This approach breaks the logic into layers.  The foundational layer is the five functions that define the Node abstraction, which gives us a way to read the tree that's completely decoupled from the array.  The details of how the tree is stored in the array (e.g. the magic 2 * index + 1 logic) are completely contained inside these functions.  You could also implement this collection of functions as a class (storing a reference to the array as an instance attribute), which would make it easy to use outside this function and unit-test on its own; I've used a NewType wrapper because it makes it easy to see (and typecheck) the abstraction while still giving us the low overhead of a single int at runtime.

This tree abstraction lets us build a sum_subtree helper that recursively sums up everything under a node.  Even if the reader didn't understand the way the tree is packed into an array (or care to), they could read this function and understand how it's navigating the tree to arrive at a sum, and if they wanted to write a similar function they'd be able to do so without having to understand the array representation.

Finally, we have the top-level logic that gets the two top-level subtrees, compares their sums, and returns a result in the expected format.

This solution is a bit less efficient than yours (the recursive call is going to use log(N) space, and the fact that we're iterating through each node one by one will add a few more clock cycles), but in many real world applications "slow and obvious" is preferable over "fast and clever".  :)


Iterate through python iterable in chunk of n I need a function to iterate through a python iterable in chunks. That is, it takes an iterable and a size of n and yields generators iterating through each chunk of size n. After some experimentation, I wrote this stupid hack because it seems there is no easy way to preemptively check whether an iterable has been exhausted. How can I improve this code?

def iterchunks(it, n):
    def next_n(it_, n_, return_first = None):
        if return_first is not None:
            n_ -= 1
            yield return_first
        for _ in range(n_):
            yield next(it_)

    # check if the iterator is exhausted by advancing the iterator,
    # if not return the value returned by advancing the iterator along with the boolean result
    def exhausted(it_):
        res = next(it_, None)
        return res is None, res

    while True:
        exhsted, rf = exhausted(it)
        if exhsted:
            return
        else:
            # if the iterator is not exhausted, yield the returned value along with the next chunk
            yield next_n(it, n, rf)

Your code has a significant bug. If I ask it to chunk a list with None at a multiple of n plus 1 spot (c * n + 1), it will not return the rest of the list

xs = list(range(75, 90))
xs[5] = None
print([list(c) for c in iterchunks(iter(xs), 5)])
# Outputs [[75, 76, 77, 78, 79]]
# Expected [[75, 76, 77, 78, 79], [None, 81, 82, 83, 84], [85, 86, 87, 88, 89]]

To resolve this, use the standard practice of trying something, and asking for forgiveness later. I would suggest either an iterable you build up. Still this seems like a case of reinventing the wheel, it is unfortunate python doesn't have it built in to the itertools library. It does define grouper in the docs of itertools, which is kinda what you want, except it pads a fill value to the end of the iterable.

def chunk(it, n):
    try:
        while True:
            xs = []  # The buffer to hold the next n items
            for _ in range(n):
                xs.append(next(it))
            yield xs
    except StopIteration:
        yield xs

This is the code from the itertools docs here, with one amendment, to yield from instead of returning the iterable it creates

def grouper(iterable, n, fillvalue=None):
    "Collect data into fixed-length chunks or blocks"
    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
    yield from zip_longest(*[iter(iterable)] * n, fillvalue=fillvalue)


Max contiguous slice in Ruby I need to compute the max contiguous slice in an array. I wrote this function, but I am not sure if it is correct or I am missing edge cases. I ran several cases.

def max_slice(a)
  max_start,max_end = 0,0
  largest_sum = 0
  curr_start,curr_end = 0,0
  curr_sum = 0
  for i in 0..a.length-1
    if (curr_sum + a[i]  < 0 ) then
      curr_start = i+1
      curr_sum = 0
    elsif (curr_sum + a[i] > curr_sum) then
      curr_sum = curr_sum + a[i]
      curr_end = i
      if (curr_sum > largest_sum) then
        max_start, max_end = curr_start,curr_end
        largest_sum = curr_sum
      end
    end
  end
  a[max_start..max_end]
end

p max_slice([-3,39,2,-1,65]) #> [39, 2, -1, 65]
p max_slice([12,-3,39,2,-1,65]) #> [12, -3, 39, 2, -1, 65]
p max_slice([]) #> []
p max_slice([2,3]) #> [2, 3]
p max_slice([-2,3]) #> [3]
p max_slice([-2]) #> [-2]
p max_slice([12,-3,39,2,-1,102]) #> [12, -3, 39, 2, -1, 102]
p max_slice([-5,-3,39,2,-39,65]) #> [39, 2, -39, 65]
p max_slice([-5,5,39,2,-60,65]) #> [65]
p max_slice([-5,35,39,2,-60,65]) #> [35, 39, 2, -60, 65]
p max_slice([-5,35,39,2,-160,65]) #> [35, 39, 2]

What you are doing here is Kadane's algorithm. Wikipedia presents an implementation in Python which should not be hard to rewrite in Ruby.


Optimized jQuery code check I am new to jQuery, and after spending weeks to convert my pages using jQuery, I am now reading about jQuery optimization. In that effort, I'm rewriting one of my functions. Please comment whether it is right (both are working).

Previous

$("#mydiv").empty();
var i = 2;
$("#axisa option").each(function() {
    $("#mydiv").append("

");
    updateCharts($("#wd").slider("value"), $("#ht").slider("value"),$(this).val());
    i++
});

$("#axisb option").each(function() {
    $("#mydiv").append("

");
    updateCharts($("#wd").slider("value"), $("#ht").slider("value"),$(this).val());
    i++
});
$("#hiddendiv").show("slow");

Optimized

var mydiv = $("#mydiv");
var mywid = $("#wd").slider("value");
var myhet = $("#ht").slider("value");
var total = $("#axisa option").length + $("#axisb option").length+2;

mydiv.empty();
var design = "";

for(var i=2;i<total;i++){
    design += "

";
}
mydiv.append(design);
i = 2;

$("#axisa option").each(function() {
    updateCharts(mywid, myhet, $(this).val());
    i++
});

$("#axisb option").each(function() {
    updateCharts(mywid, myhet, $(this).val());
    i++
});
$("#hiddenDiv").show("slow");

Here are some general optimizations that I usually do.

When you have multiple var declarations you can separate with "," making it a bit easier to read and less of a hassle to write:

var mydiv = $("#mydiv").empty(),
    mywid = $("#wd").slider("value"),
    myhet = $("#ht").slider("value"),
    etc = etc.....

Also when I name my variables, ids, classes, etc. it's good to put names that reference what's inside the element, or the role it plays. It will help you remeber what does what when you get into big projects with lots of these declarations.

As a rule of thumb, if you have to call on an element more than once, you should cache its value. This way jQuery doesn't have to jump into the DOM and look for that element each time you call it. You should also try to limit the scope of elements jQuery has to look through to find the one you want. You can use the type of element previous to the id/class. For example:

$("#mydiv").doSomething();
$("#mydiv").doSomethingElse();

Instead do:

var mydiv = $("div #mydiv");

$(mydiv).doSomething();
$(mydiv).doSomethingElse();

Like you said, you are just starting out with jQuery, and with that I highly recommend this screencast by Jeffrey Way called 30 Days to Learn jQuery. He does a really good job of explain some basic principals as well as some more complex concepts.


Threading lambda functions I've created this very small header that allows direct creation of threads from lambdas.
I can't find anything else similar on the net so I want to know whether there are any problems with this that I have not thought of. It is based on tinythread++, but can easily be altered to work with pthread or other threading libraries that can take a void (void*) (or void* (void*)) function and a void* argument for the function to start a thread. Note that this implementation is assuming limited C++0x/11 support, i.e. just lambdas.

#include "tinythread.h"

namespace lthread {
    /// implementation details - do not use directly
    namespace impl {
        template<typename Func>
        void use_function_once(void* _f) {
            const Func* f = (const Func*)_f;
            (*f)();
            delete f; // delete - no longer needed
        }
        template<typename Func>
        void use_function(void* _f) {
            const Func* f = (const Func*)_f;
            (*f)();
        }
    }

    /// Creates a thread based on a temporary function.
    /// Copies the function onto the heap for use outside of the local scope, removes from the heap when finished.
    template<typename Func>
    tthread::thread* new_thread(const Func& f) {
        Func* _f = new Func(f); // copy to heap
        return new tthread::thread(&impl::use_function_once<Func>,(void*)_f);
    }

    /// Creates a thread based on a guaranteed persistent function.
    /// Does not copy or delete the function.
    template<typename Func>
    tthread::thread* new_thread(const Func* f) {
        return new tthread::thread(&impl::use_function<Func>, (void*)f);
    }
}

Example usage:

size_t a = 1;
size_t b = 0;
tthread::thread* t = lthread::new_thread([&] () {
    std::cout << "I'm in a thread!\n";
    std::cout << "'a' is " << a << std::endl;
    b = 1;
});
t->join();
std::cout << "'b' is " << b << std::endl;
delete t;

surely you can just use std::function<void()> instead of templating on the function type?
capturing by reference can go horribly wrong. It's not a bug in your library, just an observation ...

tthread::thread* startthread(size_t a, size_t b)
{
    return lthread::new_thread([&] ()
        {
            std::cout << "I'm in a thread!\n";
            std::cout << "'a' is " << a << std::endl;
            b = 1;
        }
        );
}

int main()
{
    tthread::thread *t = startthread(1,2);
    t->join();
    delete t;
}

use_function_once is vulnerable to the function call throwing an exception: you should probably assign it to a smart pointer before calling, and lose the explicit delete.
that overload of new_thread also has a problem if either new or tthread::thread can throw: it will leak the heap-allocated Func. You can fix this with a smart pointer too.


Displaying dependent options when a selection is made in a dropdown I was wondering if someone could please point me in the right direction with this bit of JavaScript I have been working on.

I have made this form that currently has onblur JavaScript validation and additionally I am to make it dynamic so that when a user selects an option from a dropdown option form a div is shown with corresponding options.  But at the moment that code I have written for this is looking pretty repetitive and suffering from 'code smell'.

Is there a better way to do this? I am thinking a for loop that iterates through the dropdown options but am not 100% sure how to put this in place?

adventureForm = document.querySelector('#adventure');
       charterDrop = document.querySelector('.charter-options');
       mountainDrop = document.querySelector('.mountain-lake-options');
       trampDrop = document.querySelector('.tramp-options');
       photoDrop = document.querySelector('.photo-options');
       dropDowns =  document.querySelector('#dropdowns');

      // check adventure dropdown choice
      if (field.id === "adventure" && adventureForm.selectedIndex === 0) {
      dropDowns.classList.add('hidden');

          } else if (field.id === "adventure" && adventureForm.selectedIndex === 1)  {
              dropDowns.classList.remove('hidden');
             charterDrop.classList.remove('hidden');
           mountainDrop.classList.add('hidden');
           trampDrop.classList.add('hidden');
           photoDrop.classList.add('hidden');
         } else if (field.id === "adventure" && adventureForm.selectedIndex === 2)  {
           dropDowns.classList.remove('hidden');
           charterDrop.classList.add('hidden');
           mountainDrop.classList.remove('hidden');
           trampDrop.classList.add('hidden');
           photoDrop.classList.add('hidden');
         } else if (field.id === "adventure" && adventureForm.selectedIndex === 3)  {
           dropDowns.classList.remove('hidden');
           charterDrop.classList.add('hidden');
           mountainDrop.classList.add('hidden');
           trampDrop.classList.remove('hidden');
           photoDrop.classList.add('hidden');
         } else
if (field.id === "adventure" && adventureForm.selectedIndex === 4)  {
           dropDowns.classList.remove('hidden');
           charterDrop.classList.add('hidden');
           mountainDrop.classList.add('hidden');
           trampDrop.classList.add('hidden');
           photoDrop.classList.remove('hidden');
         }

You can view the JSBin link here.

There are definitely improvements. Let us start with some basics.

Begin your file with

'use strict';

This avoids a class of nasty bugs that easily creep into JavaScript programs.

Do not use global variables. Parameterization is generally the best option but in this case we can avoid polluting the global namespace by simply wrapping our code in an immediately invoked function expression (IIFE).

(function () {
    var elements;
    var i = 0;
    var isError = false;
    var errorMessage;
    var correctMessage = '✓';
    var errorSpan;
    ...
}());

Continuing, there is indeed a lot of repetition as you have correctly recognized. Fortunately, JavaScript makes it easy to remove this kind of duplication while at the same time making our code more declarative. Let's start with some simple refactoring

function hide(element) {
    element.classList.add('hidden');
}

function show(element) {
    element.classList.remove('hidden');
}

Now let us extract the code under // check adventure dropdown choice
gets us to this so far

function checkAdventureDropdown(option) {
    if (option.id !== 'adventure') {
        return;
    }

    if (adventureForm.selectedIndex === 0) {
        dropDowns.classList.add('hidden');
    } else if (adventureForm.selectedIndex === 1) {
        dropDowns.classList.remove('hidden');
        charterDrop.classList.remove('hidden');
        mountainDrop.classList.add('hidden');
        trampDrop.classList.add('hidden');
        photoDrop.classList.add('hidden');
    } else if (adventureForm.selectedIndex === 2) {
        dropDowns.classList.remove('hidden');
        charterDrop.classList.add('hidden');
        mountainDrop.classList.remove('hidden');
        trampDrop.classList.add('hidden');
        photoDrop.classList.add('hidden');
    } else if (adventureForm.selectedIndex === 3) {
        dropDowns.classList.remove('hidden');
        charterDrop.classList.add('hidden');
        mountainDrop.classList.add('hidden');
        trampDrop.classList.remove('hidden');
        photoDrop.classList.add('hidden');
    } else if (adventureForm.selectedIndex === 4) {
        dropDowns.classList.remove('hidden');
        charterDrop.classList.add('hidden');
        mountainDrop.classList.add('hidden');
        trampDrop.classList.add('hidden');
        photoDrop.classList.remove('hidden');
    }
}

Note that we can factor out the common test into an early return.
There is a lot more we can do here. We could simplify with a switch statement, but I think a cleaner approach would be to use an object.
Let us convert our imperative conditional code into a mapping that declares which elements should be visible and which elements need to be hidden for each specific option.

var states = {
    0: { hide: [dropDowns], show: [] },
    1: {
        hide: [trampDrop, mountainDrop, photoDrop],
        show: [dropDowns, charterDrop]
    },
    2: {
        show: [mountainDrop],
        hide: [charterDrop, trampDrop, photoDrop]
    },
    3: {
        show: [trampDrop],
        hide: [charterDrop, mountainDrop, photoDrop]
    },
    4: {
        show: [dropDowns, photoDrop],
        hide: [charterDrop, mountainDrop, trampDrop]
    }
}

Not only is this more readable, as we can see clearly which states include and exclude which components. Now checkAdventureDropdown becomes quite simple, with very few places for bugs to hide.

function checkAdventureDropdown(option, states) {
    if (option.id !== 'adventure') {
        return;
    }
    var state = states[adventureForm.selectedIndex];
    state.show.forEach(show);
    state.hide.forEach(hide);
}

Here is what we end up with

'use strict';
(function () {
    var elements;
    var i = 0;
    var isError = false;
    var errorMessage;
    var correctMessage = '✓';
    var errorSpan;

    var adventureForm;
    var charterDrop;
    var mountainDrop;
    var trampDrop;
    var photoDrop;
    var dropDowns;

    var states = {
        0: { hide: [dropDowns], show: [] },
        1: {
            hide: [trampDrop, mountainDrop, photoDrop],
            show: [dropDowns, charterDrop]
        },
        2: {
            show: [mountainDrop],
            hide: [charterDrop, trampDrop, photoDrop]
        },
        3: {
            show: [trampDrop],
            hide: [charterDrop, mountainDrop, photoDrop]
        },
        4: {
            show: [dropDowns, photoDrop],
            hide: [charterDrop, mountainDrop, trampDrop]
        }
    };
    if (option.id !== 'adventure') {
        return;
    }
    function checkAdventureDropdown() {
        var state = states[adventureForm.selectedIndex];
        state.show.forEach(show);
        state.hide.forEach(hide);
    }
    checkAdventureDropdown();
}());

There is definitely room for improvement, but I would argue that this is a step in the right direction.

Looking over the linked html, each possible value of adventureForm.selectedIndex corresponds to a meaningful string value. We can leverage this value, which is exposed via adventureForm.value, to further increase readability by giving out states meaningful names instead of numeric indeces.

var states = {
    '': { hide: [dropDowns], show: [] },
    charter: {
        hide: [trampDrop, mountainDrop, photoDrop],
        show: [dropDowns, charterDrop]
    },
    mountain: {
        show: [mountainDrop],
        hide: [charterDrop, trampDrop, photoDrop]
    },
    tramping: {
        show: [trampDrop],
        hide: [charterDrop, mountainDrop, photoDrop]
    },
    photograph: {
        show: [dropDowns, photoDrop],
        hide: [charterDrop, mountainDrop, trampDrop]
    }
};

We can make this even cleaner by encapsulating our hide and show into a single function which understands our states and how to toggle them.

'use strict'
(function() {
    if (option.id !== 'adventure') {
        return;
    }

    var states = {
        '': { hide: [dropDowns], show: [] },
        charter: {
            hide: [trampDrop, mountainDrop, photoDrop],
            show: [dropDowns, charterDrop]
        },
        mountain: {
            show: [mountainDrop],
            hide: [charterDrop, trampDrop, photoDrop]
        },
        tramping: {
            show: [trampDrop],
            hide: [charterDrop, mountainDrop, photoDrop]
        },
        photograph: {
            show: [dropDowns, photoDrop],
            hide: [charterDrop, mountainDrop, trampDrop]
        }
    };

    function applyState(state) {
        hide(state.hide);
        show(state.show);

        function hide(elements) {
            elements.forEach(function (element) {
                element.classList.add('hidden');
            });
        }

        function show(elements) {
            elements.forEach(function (element) {
                element.classList.add('hidden');
            });
        }
    }

    var state = states[adventureForm.value];
    if (!state) {
        throw Error('invalid index');
    }
    applyState(state);
}());

Clarity is increasing fast. We can leverage hoisting to improve it still further.

'use strict';
(function() {
    if (option.id !== 'adventure') {
        return;
    }

    var states = {
        '': { hide: [dropDowns], show: [] },
        charter: {
            hide: [trampDrop, mountainDrop, photoDrop],
            show: [dropDowns, charterDrop]
        },
        mountain: {
            show: [mountainDrop],
            hide: [charterDrop, trampDrop, photoDrop]
        },
        tramping: {
            show: [trampDrop],
            hide: [charterDrop, mountainDrop, photoDrop]
        },
        photograph: {
            show: [dropDowns, photoDrop],
            hide: [charterDrop, mountainDrop, trampDrop]
        }
    };

    var state = states[adventureForm.value];
    if (!state) {
        throw Error('invalid index');
    }
    applyState(state);

    function applyState(state) {
        hide(state.hide);
        show(state.show);

        function hide(elements) {
            elements.forEach(function (element) {
                element.classList.add('hidden');
            });
        }

        function show(elements) {
            elements.forEach(function (element) {
                element.classList.add('hidden');
            });
        }
    }
}());

Here, we leverage the fact that JavaScript hoists function definitions to the top of their enclosing lexical scope to allow our code to read more naturally and declaratively. but we can use this technique to go further.

function getStates() {
    return {
        '': { hide: [dropDowns], show: [] },
        charter: {
            hide: [trampDrop, mountainDrop, photoDrop],
            show: [dropDowns, charterDrop]
        },
        mountain: {
            show: [mountainDrop],
            hide: [charterDrop, trampDrop, photoDrop]
        },
        tramping: {
            show: [trampDrop],
            hide: [charterDrop, mountainDrop, photoDrop]
        },
        photograph: {
            show: [dropDowns, photoDrop],
            hide: [charterDrop, mountainDrop, trampDrop]
        }
    };
}

leaving us with the quite elegant

'use strict'
(function() {
    if (option.id !== 'adventure') {
        return;
    }

    var states = getStates();
    var state = states[adventureForm.value];

    if (!state) {
        throw Error('invalid state');
    }
    applyState(state);

    function getStates { ... }

    function applyState(state) { ... }
}());

I would like to add that while the addition of the document.querySelector API is a big improvement, there are libraries such as jQuery that you can leverage to make your html itself more maintainable.


Passing objects atomically across threads without locks or data races for audio synchronization I am learning about one of the hardest parts of Audio development: the synchronization between the audio thread and the GUI thread.  Per the discussion here https://forum.juce.com/t/timur-doumler-talks-on-c-audio-sharing-data-across-threads/26311 and here: https://stackoverflow.com/questions/15460829/lock-free-swap-of-two-unique-ptrt
I'm wondering if the following class solves the problem or comes close to solving it.

template<typename T>
struct SmartAtomicPtr
{
    SmartAtomicPtr( T* newT )
    {
        update( newT );
    }
    ~SmartAtomicPtr()
    {
        update(nullptr);
    }
    void update( T* newT, std::memory_order ord = memory_order_seq_cst )
    {
        keepAlive.reset( atomicTptr.exchange( newT, ord ) );
    }
    std::shared_ptr<T> getShared(std::memory_order ord = memory_order_seq_cst)
    {
       return std::make_shared<T>( atomicTptr.load(ord) );
    }
    T* getRaw(std::memory_order ord = memory_order_seq_cst)
    {
         return atomicTptr.load(ord);
    }
private:
    std::atomic<T*> atomicTptr{nullptr};
    std::shared_ptr<T> keepAlive;
};

I know that whatever value ends up in the shared_ptr won't be deleted until the SmartAtomicPtr goes out of scope, which is fine.

the ultimate goal would be a lock-free, wait-free solution.

an example of where this might get used is the following interleaving of the audio and message thread.  The goal is to keep the returned object from dangling

/*
AudioProcessor owns a SmartAtomicPtr<T> ptr that the message
thread has public access to.
*/
/* audio thread */ auto* t = ptr.getRaw();
/* message thread */ processor.ptr.update( new T() );
/* audio thread */ t->doSomething(); //t is a dangling pointer now

with getShared(), I believe that t no longer dangles:

/* audio thread */ auto t = ptr.getShared();
/* message thread */ processor.ptr.update( new T() );
/* audio thread */ t->doSomething(); //t is one of 2 shared_ptrs
//holding the previous atomic value of ptr

I ran into some double-deletes, but I believe I have solved them, and also prevented the shared_ptr member from being stomped on in the event you call getShared() and update() at the same time, and also kept it leak-free.

any thoughts?

keepAlive.reset is not thread safe. So your class as a whole cannot be thread safe.


Binary check code in pyspark I am trying to find out if a column is binary or not. If a column is only having 1 or 0 then I am flagging it as binary, else non binary. Since I have a database background, I tried to achieve it through a SQL like statement. But when the file is big, this code is not giving me good performance.

Could you please suggest how I can improve this code:

input_data=spark.read.csv("/tmp/sample.csv", inferSchema=True,header=True)
input_data.createOrReplaceTempView("input_data")
totcount=input_data.count()

from pyspark.sql.types import StructType,StructField,StringType
profSchema = StructType([ StructField("column_name", IntegerType(), True)\
                   ,StructField("binary_type", StringType(), True)])
fin_df=spark.createDataFrame([],schema=profSchema) ##create null df

for colname in input_data.columns:
    query="select {d_name} as column_name,case when sum(case when {f_name} in ( 1,0) then 1 else 0 end) == {tot_cnt} then 'binary' else 'nonbinary'\
    end as binary_stat from input_data".format(f_name=colname, d_name="'"+str(colname)+"'",tot_cnt=totcount)
    join_df=spark.sql(query)
    fin_df=fin_df.union(join_df)

Spark dataframes (and columns) have a distinct method, which you can use to get all values in that column. Getting the actual values out is a bit more complicated and taken from this answer to a similar question on StackOverflow:

from pyspark.sql import SparkSession

def get_distinct_values(data, column):
    return {x[column] for x in data.select(column).distinct().collect()}

spark = SparkSession \
    .builder \
    .appName("Python Spark SQL basic example") \
    .config("spark.some.config.option", "some-value") \
    .getOrCreate()

input_data = spark.read.csv("/tmp/sample.csv", inferSchema=True, header=True)
input_data.createOrReplaceTempView("input_data")

print({c: get_distinct_values(input_data, c) == {True, False}
       for c in input_data.columns})
# {'category': False, 'logged_in': True, 'gid': False, 'pol_id': False, 'subcategory': False}

I don't know enough about spark to know how you would cast this back into a spark dataframe, but this should get you at least halfway there and be a bit faster, since it can do the fastest implementation it can to reduce the values to sets.

# /tmp/sample.csv
pol_id,gid,category,subcategory,logged_in
1,1,A,a,1
2,1,A,b,0
1,2,B,b,1
2,2,B,a,0


Return a 404 when a resource is not found in Spring Boot I wrote this code in order to return a http status of not found to my users in case the resource is not present in the DB.

@RestController
public class ExampleController {

    public final ExampleService exampleService;

    @Autowired
    public ExampleController(ExampleService exampleService) {
        this.exampleService = exampleService;
    }

    @ResponseStatus(value = HttpStatus.NOT_FOUND)
    public class ResourceNotFoundException extends RuntimeException {
    }

    @GetMapping("/api/mappings/get-by-id/{id}")
    public ExampleDto getExampleById(@PathVariable String id) {
        Example example = exampleService.findById(id).orElse(null   );
        if (example == null) throw new ResourceNotFoundException();
        return new ExampleDto(example);
    }

}

I would like to know if this code can be considered good and robust enough or if it can be improved and how.

Your URL path isn't very RESTlike. You have built the URL to describe an operation when a more common approach is to build the URL to describe a resource.

So instead of https://example.com/api/mappings/get-by-id/42 one would build a URL like https://example.com/api/mappings/42.

The name get-by-id is fully redundant in REST world as the "get" operation is already defined in the HTTP GET-method (the @GetMapping annotation in Spring Boot) and identifying resources by their identifier is always the default operation :).


Replacing elements from a list and its sublists - part II This is sort of a follow-up to
Replacing elements from a list and its sublists
but now there are arbitrary numbers of words that would be replaced stored in a list.

Now write substitute2 that takes a list, a list of old words, and a list of new words; the last two lists should be the same length. It should return a copy of the first argument, but with each word that occurs in the second argument replaced by the corresponding word of the third argument:

source

Please review my code.

(define (substitute2 lst oldlst newlst)
    (define (maybe-swap elem oldlst newlst)
        (cond ((null? oldlst) elem)
            ((equal? (car oldlst) elem) (car newlst))
        (else (maybe-swap elem (cdr oldlst) (cdr newlst)))))
        (if (list? lst)
            (map (lambda (elem) (substitute2 elem oldlst newlst)) lst)
    (if (= (length oldlst) (length newlst)) (maybe-swap lst oldlst newlst) (error "invalid length"))))

How can I make this code better and more efficient? Did I overuse define?

This looks pretty good. I only see one issue and it's more code appearance than functional

Fix your spacing

You indentation isn't quite right and makes it a little hard to see what clauses line up where. Here's your same code slightly altered:

(define (substitute2 lst oldlst newlst)
    (define (maybe-swap elem oldlst newlst)
        (cond ((null? oldlst) elem)
              ((equal? (car oldlst) elem) (car newlst))
              (else (maybe-swap elem (cdr oldlst) (cdr newlst)))))
    (if (list? lst)
        (map (lambda (elem) (substitute2 elem oldlst newlst)) lst)
        (maybe-swap lst oldlst newlst)))

I lined up the three cond clauses - and cleared up the fact that the map and maybe-swap both belong to the if. As-is, it kind of looks life the if is part of maybe-swap.

Make a swapper function

This is more for fun than it is necessarily a valuable suggestion (I have no idea if this is good or not). But one thing we can do is take oldlst and newlst and turn those into a swapper function - one that takes an elem and returns the correct substituted result. The code to build up such a swapper is pretty similar to your maybe-swap already:

(define (make-swapper oldlst newlst)
   (if (null? oldlst)
       (lambda (elem) elem)
       (lambda (elem)
          (if (equal? elem (car oldlst))
              (car newlst)
              ((make-swapper (cdr oldlst) (cdr newlst)) elem)))))

So then in substitute2, we just make one up front and call it:

(define (substitute2 lst oldlst newlst)
    (define swapper (make-swapper oldlst newlst))
    (define (substitute-impl lst)
        (if (list? lst)
            (map substitute-impl lst)
            (swapper lst)))
    (substitute-impl lst))


Spreadsheet column membership test I've written the code below to compare the two columns of choice of two different excel files (new data and a reference file). If a value in the new data file is present in the reference file, "yes" is added in a new column behind the value in the new data file and "no" if not. The columns to compare can be anything you want and can be specified in the top of the script.

The script does the trick for small datasets (few 100 to few 1000 rows). However, I'm currently comparing two files where each files has ~300 000 columns. It has currently analysed +/- 56% of the new data file rows in ~18 hours. So the script needs to run nearly 36 hours to complete.  It appears that Python only processes ~2 rows per second, hence the time.

Is there a way to speed up this process? I was thinking that perhaps a list comprehension could add a little bit to the speed, but it seems complex to write this as a list comprehension. Any other suggestions?

PS: The error raises perhaps seem redundant but as I'm no Python expert I was experimenting with error raising.

#======SPECIFY COLUMNS TO COMPARE======#
data_column = "column name"
ref_column ="column name"

#=====================================#

import os

basedir = r'C:\path'
datadir = os.path.join(basedir, "data")
refdir = os.path.join(basedir, "ref_db")
exportdir = os.path.join(basedir, "export")

import pandas as pd

#select the excel file (with any name) in the data folder
data_list = [os.path.join(datadir,file) for file in os.listdir(datadir) if file.endswith(".xlsx")]

if len(data_list) == 0:
    raise ImportError("There is no excel file in the data folder")
elif len(data_list) > 1:
    raise ImportError("There is more than 1 excel file in the data folder")

#data_list is a list of excel files in the folder. Select the one with index 0 as this should be the only one present.
new_data = pd.read_excel(data_list[0])

new_data[str("Comparison"+"_"+data_column)] = ""

#load the reference database in the export folder
ref_list = [os.path.join(refdir,file) for file in os.listdir(refdir) if file.endswith(".xlsx")]

if len(ref_list) == 0:
    raise ImportError("There is no excel file in the ref_db folder")
elif len(ref_list) > 1:
    raise ImportError("There is more than 1 excel file in the ref_db folder")

ref_db = pd.read_excel(ref_list[0])

#check if specified columns occur in the excel files
if not data_column in new_data.columns:
    raise KeyError("specified data_column does not exist in the input data file")

if not ref_column in ref_db.columns:
    raise KeyError("specified ref_column does not exist in the input data file")

warn = []

for i,col in enumerate(new_data[data_column]): #iterate over each row in the specified column + save the index "i"

    if col in ref_db[ref_column].values: #check if the value col at index i of new_data occurs in the reference database:

        new_data[str("Comparison"+"_"+data_column)][i] = "yes"

        count = ref_db.groupby(ref_column).size()

        if count[col] > 1:
          warn.append(["WARNING: value",col,"occurs multiple times in column",ref_column,"of the ref_db excel file"])
     #if no:
    else: #assign "no" to the column
        new_data[str("Comparison"+"_"+data_column)][i] = "no"

    #print("Processing sequence",i,"of",len(new_data["sequence"])) #print the progress of the matching process
    print(round((i/len(new_data[data_column]))*100,2),"% of entries processed")

for i,j in enumerate(warn):
    print(*warn[i])

new_data.to_excel(os.path.join(exportdir, "compare_export.xlsx")) #save the file to the export folder

Unfortunately I am not experienced with Pandas especially on large files. But I would have tried a more 'native' solution like VBA but it is because I have some experience with it. There is a chance that VBA (in the form of a macro) would perform faster.

Perhaps you can try a hybrid approach, for example add some temporary columns to the files you are processing in Python, and fill those columns with formulae like COUNTIF etc.
The aim is to let Excel do the computationally-intensive stuff. And then you let your script loop row by one using the precalculations made by Excel.

When you are done, you remove the temporary columns or simply close the file discarding changes.
This may not be the more elegant approach, but I am not surprised that an interpreted language like Python with all the middleware involved is slow for this kind of task.

Another option is to dump the sheets to a database, MS Access or SQLite, add some indexes where appropriate and use SQL to fill out the gaps and then re-import the result to Excel.

This is more a hack than a real solution. My considerations would be:

is this a one-shot operation or a process that you will have to repeat ? This is to answer the question: how much time and energy are you willing to invest in this ?
You have not mentioned the purpose of the Excel files, and you've not shared the structure or a data sample. 300K columns for an Excel file is very big. I am wondering if by any change you are on assignment with some corporate client where clerks use Excel like a database. This is a very common scenario unfortunately. Then I would advise them to use a better solution. The whole process and usage is flawed and the solution can hardly be elegant.

Probably there are solutions already available on the market, including free and open source software. From experience there is little added value trying to patch a flawed process.

Everybody uses Excel, so Excel files proliferate everywhere and the result is poorly-structured data that is hard to exploit.
Using Excel is often the problem and the solution is to move away from Excel.


SearchActivity to search through an ArrayList I've coded this class that lets me search through an ArrayList displayed into a RecyclerView. This is the class:

public class SearchActivity extends AppCompatActivity {
    ArrayList<Accordo> chords;
    RecyclerView rv;
    SearchView sv;
    ArrayList<Accordo> filteredList;

    @Override
    protected void onCreate(@Nullable Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.search_layout);

        /** gestisce la pubblicita */
        MobileAds.initialize(getApplicationContext(), "ca-app-pub-3940256099942544/6300978111");

        AdView searchBanner = (AdView) findViewById(R.id.search_ad);
        AdRequest adRequest = new AdRequest.Builder().build();
        searchBanner.loadAd(adRequest);

        /**-------------------------------*/

        Intent intent = this.getIntent();
        Bundle bundle = intent.getExtras();
        chords = bundle.getParcelableArrayList("chords");
        filteredList = bundle.getParcelableArrayList("chords");

        sv = (SearchView) findViewById(R.id.testo_ricerca);
        sv.setIconifiedByDefault(false);

        rv = (RecyclerView) findViewById(R.id.lista_ricerca);
        rv.setLayoutManager(new LinearLayoutManager(SearchActivity.this, LinearLayoutManager.VERTICAL, false));
        rv.setHasFixedSize(true);

        final SearchAdapter adapter = new SearchAdapter(this, chords);
        rv.setAdapter(adapter);

        //SEARCH
        sv.setOnQueryTextListener(new SearchView.OnQueryTextListener() {
            @Override
            public boolean onQueryTextSubmit(String query) {
                return false;
            }

            @Override
            public boolean onQueryTextChange(String newText) {
                //FILTER AS YOU TYPE
                List<Accordo> filteredModelList = filter(chords, newText);
                adapter.setFilter(filteredModelList);
                return true;
            }
        });

        /** gestisce cosa succede quando un elemento della lista viene cliccato */
        ItemClickSupport.addTo(rv).setOnItemClickListener(new ItemClickSupport.OnItemClickListener(){
            @Override
            public void onItemClicked(RecyclerView recyclerView, int position, View v) {
                Intent intent = new Intent(SearchActivity.this, ChordActivity.class);
                Bundle bundle = new Bundle();
                bundle.putParcelable("selected", filteredList.get(position));
                intent.putExtras(bundle);
                startActivity(intent);
            }
        });

    }

    private List<Accordo> filter(List<Accordo> models, String query) {
        query = query.toLowerCase();

        filteredList = new ArrayList<>();
        for (Accordo model : models) {
            final String text = model.getName().toLowerCase();
            if (text.contains(query)) {
                filteredList.add(model);
            }
        }
        return filteredList;
    }
}

As you can see from the code above, it is based on a filter that every time the user types something in the search box, creates a filteredList and puts it into the RecyclerView. Of course when the user did not type anything the filteredList should have contained the whole ArrayList chords

So I was getting an error (NPE) when I clicked on an Item while nothing had been typed into the search box, because the filteredList was null. I managed to solve it by having both:

chords = bundle.getParcelableArrayList("chords");

filteredList = bundle.getParcelableArrayList("chords");

I think this is a huge waste of memory and resources since we are talking about an ArrayList with ca 300 elements, each of which has 5 images, strings and sounds.

Is there a more efficient way to achieve the same result?

Once is enough of bundle.getParcelableArrayList("chords")

I'm not sure if bundle.getParcelableArrayList creates a new list every time it's called. If it doesn't, then repeated calls won't double the memory used so it won't really be a problem.
But in any case, you don't need to call it twice like this:

chords = bundle.getParcelableArrayList("chords");
filteredList = bundle.getParcelableArrayList("chords");

You can make filteredList reference chords:

filteredList = chords = bundle.getParcelableArrayList("chords");

Use interfaces in declarations

These fields would be better declared as List instead of ArrayList:

ArrayList<Accordo> chords;
ArrayList<Accordo> filteredList;

Just like you used List instead of ArrayList in the filter method.

Also, probably all fields of the activity should be private.

Reduce memory churn

Every time the query text changes,
you recreate a new list,
and re-link the adapter to the new list.
It might be more efficient to reuse the same list,
by clearing and re-adding elements instead of creating a new list.
However,
there is just one tricky point,
of the initial state when there is no filter yet.
The adapter could initially be linked to chords,
and then the first time a query text is entered,
re-link it to filteredList, for example:

@Override
public boolean onQueryTextChange(String newText) {
    if (filteredList == null) {
        // first time used
        filteredList = new ArrayList<>();
        adapter.setFilter(filteredList);
    }
    filter(newText);
    return true;
}

private void filter(String query) {
    query = query.toLowerCase();

    filteredList.clear();
    for (Accordo model : chords) {
        final String text = model.getName().toLowerCase();
        if (text.contains(query)) {
            filteredList.add(model);
        }
    }
}

Notice some other related changes:

I dropped the list parameter of filter: the filtering is always based on chords, which is a field, so the method has direct access to it, no need to pass as parameter
Made filter return void, as now it modifies filteredList in-place


Initializing entity framework context in constructor We use the architecture shown in the code below to initialize the DB context for our MVC 5 project using entity framework 6

We have the following questions related to it:

Is it safe to initialize the Context in the constructor and use it throughout the class?
What are the disadvantages of the given approach?

Constructor:

public class HomeController : Controller
{
    public DBEntities Context { get; set; }
    public HomeController()
    {
        Context = new DBEntities();
    }

    ...
}

We have a single point of entry to the controller using the Perform method (This method is invoked from the client using ajax):

[HttpPost]
public async Task<ContentResult> Perform(string operation, string entity, FormCollection form = null)
{
    var json = string.Empty;
    var returnTuple = new Tuple<string, int, string>(string.Empty, (int)HttpStatusCode.InternalServerError, "Default return value");

    switch (operation)
    {
        case "GET":
            switch (entity)
            {
                case ("ALLSCHOOLS"):
                    returnTuple = await GetSchools(form);
                break;

                ...
            }
        break;

        case "POST":
            switch (entity)
            {
                case "SCHOOL":
                    returnTuple = await SaveSchool(form);
                break;

                ...
            }
        break;
    }

    ...

    if (!string.IsNullOrEmpty(returnTuple.Item1))
    {
        json = returnTuple.Item1;
        Response.StatusCode = (int)HttpStatusCode.OK;
    }
    else
    {
        Response.StatusCode = returnTuple.Item2;
        Response.StatusDescription = returnTuple.Item3;
    }
    return Content(json, "application/json");
}

GetSchools method:

private async Task<Tuple<string, int, string>> GetSchools(FormCollection form)
{
    var json = string.Empty;
    var statusCode = int.MinValue;
    var statusDescription = string.Empty;

    var academicYear = Convert.ToInt32(form["academicyear"]);

    var schoolsObj = await Context.schools.Where(s => s.AcademicYear == academicYear).ToListAsync(); //Context is initialized in constructor shown above

    json = JsonConvert.SerializeObject(new
    {
        EntityObject = schoolsObj,
        Message = string.Empty,
        Formatting.None
    }, new JsonSerializerSettings
    {
        ReferenceLoopHandling = ReferenceLoopHandling.Ignore
    });
    return Tuple.Create(json, statusCode, statusDescription);
}

SaveSchool method:

private async Task<Tuple<string, int, string>> SaveSchool(FormCollection form)
{
    var json = string.Empty;
    var statusCode = int.MinValue;
    var statusDescription = string.Empty;
    var schoolObj = new school();
    var message = string.Empty;

    if (TryUpdateModel(schoolObj, form))
    {
        Context.schools.Add(schoolObj);
        var saveChanges = await Context.SaveChangesAsync();
        if (saveChanges == 0)
        {
            json = string.Empty;
            statusCode = (int)HttpStatusCode.InternalServerError;
            statusDescription = "Unable to save the data. Please try again later.";
            return Tuple.Create(json, statusCode, statusDescription);
        }
        else
        {
            json = JsonConvert.SerializeObject(new
            {
                EntityObject = schoolObj,
                Message = "Data saved successfully."
            }, new JsonSerializerSettings
            {
                ReferenceLoopHandling = ReferenceLoopHandling.Ignore
            });
        }
    }
    else
    {
        message = ModelStateValidationErrors(); //ModelStateValidationErrors() returns all modelstate errors
        statusCode = (int)HttpStatusCode.BadRequest;
        statusDescription = "Please check the error(s) below and resubmit the form.<br/>" + message;
    }
    return Tuple.Create(json, statusCode, statusDescription);
}

It is safe to use the context the way you are using it. However, I don't think you will be able to write unit test. This isn't bad in itself. It does become an issue when your business logic is middle tier. Also why aren't you all using the rest standard instead of your perform method? Rest leaverages http to make your api more clear and interoperable.


apt alternative for proxy environment with bash shell As I am new to bash scripting, and want to use apt-get in my university.  I know that many people have issues when trying to do so.  My focus is simplicity and ease of use, but still need to be somewhat robust.

#!/bin/bash
string="install"
errormsg="\n\tInvalid input\n You should run this script with the following structure:\n\n sudo apt-proxy-install.sh install USERNAME PASSWORD\n\n "
DIRECTORY=~/bin/
if [ "$1" = "$string" ] && { ! ([ -z "$3" ] || [ -z "$3" ]) }
   then
         if [ ! -d "$DIRECTORY" ]; then
            mkdir ~/bin/
         fi
         printf "#!/bin/bash\nhttp_proxy=""http://$2:$3@10.20.10.50:3128"" sudo apt-get \${@:2}" > ~/bin/apt-proxy
         chmod 777 ~/bin/apt-proxy
         PATH=~/bin:$PATH
         addpath="export $PATH"
         sudo cat ~/.bashrc $addpath > ~/.bashrc
   else
         printf "$errormsg"
fi

My question is: is this code acceptable, or should I improve it? If the answer is not, then please give me some hints.

Storing passwords in text files is never a good idea, especially if you give that file world-readable permissions.

Try this:

#!/bin/bash

mkdir -p ~/bin/

# add ~/bin to PATH if not already there
echo '[[ :"$PATH": == *:"$HOME/bin":* ]] || PATH="$HOME/bin:$PATH"' >> .bashrc

# create the apt-proxy script
cat <<'END_SCRIPT' > ~/bin/apt-proxy
#!/bin/bash

stty -echo
printf "Password for %s: " "$LOGNAME"
read password
stty echo
echo

export http_proxy="http://${LOGNAME}:${password}@10.20.10.50:3128"
sudo -S apt-get "$@" <<< "$password"
END_SCRIPT

chmod 755 ~/bin/apt-proxy

cat <<INSTRUCTIONS
The apt-proxy script has been installed.
You may need to log out and log back in before you can use it.

usage: apt-proxy package ...
INSTRUCTIONS

mkdir -p dir will silently do nothing if the directory already exists
you only need to add the user's bin dir to the PATH if it is not already there
DO NOT store the user's password. Have the user enter it each time.
you should not even need to use sudo to edit files in your home directory.
777 permissions is overly generous: the world does not have to be able to edit the file
use sudo -S and pass the user's password via stdin


Is there a good way to do this C++ single-player class? I am making a little single-player game and to help with scoring I have created a Player class. Can you review my implementation?

I know that for a multiplayer game this isn't good. For multiplayer I would need to call functions and have all players in one instance.

File player.h

namespace SDLGamecore {
namespace player {

    class Player
    {
    private:
        long cash;
        int workers;
        std::string name;
    public:
        Player(std::string playerName)
        {
            name = playerName;
            cash = 0;
            workers = 0;
        }
        ~Player(){}

        long getCurrentCash() const;
        int getWorkers() const;
        std::string getPlayerName() const;
    };
}}

File player.cpp

namespace SDLGamecore {
namespace player {

    long Player::getCurrentCash() const
    {
        return cash;
    }

    int Player::getWorkers() const
    {
        return workers;
    }

    std::string Player::getPlayerName() const
    {
        return name;
    }

}}

File main.cpp

Player player("Test");

std::string pName = player.getPlayerName();
std::cout << pName << std::endl;

The code works as expected. Thank you for any feedback and assistance in improving my single player class.

There's not a lot of substance here to review, but anyway, here are some things that may help you improve your code.

Use include guards

There should be an include guard in the .h file.  That is, start the file with:

#ifndef PLAYER_H
#define PLAYER_H
// file contents go here
#endif // PLAYER_H

Make sure you have all required #includes

The code uses std:string within player.h but doesn't #include <string>.  Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation.

Let the compiler create default destructor

The compiler will create a destructor by default which is essentially identical to what you've got, so you can simply omit both the declaraton and implementation from your code.

Prefer modern initializers for constructors

The constructor could use the more modern initializer style rather than the old style you're currently using.  Instead of this:

Player(std::string playerName)
{
    name = playerName;
    cash = 0;
    workers = 0;
}

You could use this:

Player(std::string playerName) :
    cash{0},
    workers{0},
    name{playerName}
{ }

Write member initializers in declaration order

When you start writing constructors in the modern style mentioned above, you'll want to make sure that you write the initializers in the same order as they are declared. Because members are always initialized in declaration order and cash is declared before name in this class. To avoid misleading another programmer, you should make the order match in all constructors.

Don't write Java in C++

While it may be fashionable in Java to write canonical getters and setters of the form you've got, it's definitely not C++ style.  Consider these two lines from main:

std::string pName = player.getPlayerName();
std::cout << pName << std::endl;

We already know the object is a player, so saying player.getPlayerName() seems redundant.  I'd prefer to just write player.name() like this:

std::cout << player.name() << std::endl;

Rethink namespaces

Is it really necessary to have a player namespace encapsulating a Player class?  Specifying your own namespace is a good idea, but nesting them deeply is probably not.

Consider using inheritance

It's likely that your eventual game is going to have more than just Player class objects and it's also likely that those other objects will also have names.  This suggests that it might make sense to have a base class that includes common things such as name and then derive more specialized objects from it.

Provide the entire code to simplify reviews

Finally, it's not really about the code but about the review process.  A minimal real main.cpp would look something like this:

#include <iostream>
#include <string>
#include "player.h"

int main() {
    using namespace SDLGamecore::player;
    Player player("Test");

    std::cout << player.name() << std::endl;
}

It's usually better to post code that will actually compile without modification rather than snippets of code that require the reviewer to imagine the missing bits.  Including everything needed will help you get better and more useful reviews.


Implementing a new event in an old mootools version I'm emulating a little bit the .on() from jQuery in an old Mootools version. One of the project requirements is that Mootools can't (shouldn't) be upgraded (using version 1.2.x).

Basically I'm checking the element itself and its two closest ancestors (like .parents() in jQuery).

My working code is:

Element.implement({
            addLiveEvent: function(event, selector, fn){
                this.addEvent(event, function(e){
                    var t = $(e.target);
                    if (!t.match(selector)) {
                        t = $(e.target.parentNode);
                        if(!t.match(selector)) {
                            t = $(e.target.parentNode.parentNode);
                            if(!t.match(selector)) {return false;}
                        }
                    }
                        fn.apply(t, [e]);
                }.bindWithEvent(this, selector, fn));
            }
        });

I know those if() and the way I'm setting the variable t are not a good practice, however I'm not sure how to improve it.

return false
I'd consider this a bug. Returning false "kills" the event, meaning nothing else will happen. Just because your event handler shouldn't be called doesn't mean that's the end of the story.

For instance, if you have a regular, normal link in the element you're attaching the event listener to, then that link won't behave normally anymore. Clicking it will cause an event to bubble up and get handled by your addLiveEvent code, but if the event target (the link) doesn't match the selector, your code just defaults to returning false, effectively cancelling the click. So now your links don't work anymore. Oops.

Related to this: You should return whatever fn return, if fn gets called at all. Otherwise you have the opposite problem: You want to "cancel" a click, but the return value from the fn handler you define just gets ignored.

Here's an example (note that the 2nd link isn't cancelled by the code, as it should be, but may be cancelled by the browser, since snippets run inside an iframe, and are thus subject to security restrictions).

// OP's code
Element.implement({
  addLiveEvent: function(event, selector, fn) {
    this.addEvent(event, function(e) {
      var t = $(e.target);
      if (!t.match(selector)) {
        t = $(e.target.parentNode);
        if (!t.match(selector)) {
          t = $(e.target.parentNode.parentNode);
          if (!t.match(selector)) {
            return false;
          }
        }
      }
      fn.apply(t, [e]);
    }.bindWithEvent(this, selector, fn));
  }
});

// listen for clicks on a selector that won't match anything
$("case1").addLiveEvent("click", ".no-such-class", function() {});

// listen for link-clicks and try to cancel them
$("case2").addLiveEvent("click", ".cancel-click", function() {
  return false; // doesn't work
});

  A link that should take you to CodeReview (but doesn't)

  A link that should not navigate anywhere (but does)

  Or, well, it tries to navigate there but may be blocked because it's running inside an iframe. Check the console

fn.apply(t, [e])
There's no need for apply when it's just one argument; just use call
bindWithEvent
As far as I can tell, this isn't necessary. You're attaching an even listener to this, and then you're binding it to this. Makes no difference, as far as I can tell.

And yeah, those nested ifs aren't very nice. You're also limiting it to 2 ancestors, which seems like something that could easy prove too limited.

You could do a recursive solution, but it's much simpler to loop through an array instead.

I looked up the docs, and it seems there's a Element.getParents method you can use. So if you know the event.target, you can simply get an array of all its ancestors, and go through them. Of course, you'll want to stop if you hit the element that you originally added the live event listener to (otherwise you'll continue to check ancestors all the way to document), but that's a simple equality comparison.

Please note that it's been almost a decade since I last used Mootools, but this should work:

Element.implement({
  addLiveEvent: function(event, selector, fn) {
    this.addEvent(event, function(event) {
      var target = $(event.target), // get event target
          path = target.getParents(), // get ancestors
          i, l;

      // add the target to the list, so it gets checked first
      path.unshift(target);

      // loop through the list
      for (i = 0, l = path.length; i < l; i++) {
        if ($(path[i]).match(selector)) {
          return fn.call(path[i], event); // got a match
        }

        if (path[i] === this) {
          // we've reached the element we attached to, so stop
          break;
        }
      }
    });
  }
});

// Usage
$("container").addLiveEvent("click", ".clickable", function (event) {
  // here, `this` will refer to the matched element
  alert(this.title);
});
div {
  padding: 0.5em 1em 1em 1em;
  margin-top: 1em;
  border: 1px solid #000;
}

.clickable {
  background: green;
}

  Container (listener attached here)

    Random DIV

      Another DIV

        Click target

          Nested target


Shipping address validations In my journey of learning Rails, I'm wondering if I should test model validations. If so, is this a correct way to do it?
require 'test_helper'

class ShippingAddressTest < ActiveSupport::TestCase

  test "shipping addresses validations" do
    s = ShippingAddress.create
    assert_not s.valid?, "Address was valid without any data"
    assert s.errors.include? :name
    assert s.errors.include? :phone
    assert s.errors.include? :address
    assert s.errors.include? :user
    assert s.errors.include? :city

    s.name = "Some"
    s.phone = "8296964737"
    s.address = "Calle #1"
    s.user_id = 1

    # Test Inclusion of city
    s.city = "Santo"
    assert_not s.valid?, "Address was valid with incorrect city"

    s.city = "Santo Domingo"
    assert s.valid?, "Address not valid with everything filled"
  end

end

I just found out while reading Testing Rails guide about this:

3.3 What to Include in Your Unit Tests
Ideally, you would like to include a test for everything which could possibly break. It's a good practice to have at least one test for each of your validations and at least one test for every method in your model.

In this case, what would be the correct way to add a test for a single validation?

Unit tests

Unit tests should test the smallest behavior possible in each method of tests. Why ? Because when you make changes it helps determine what has break just by looking at failing tests.

  3.3 What to Include in Your Unit Tests

  Ideally, you would like to include a test for everything which could
  possibly break. It's a good practice to have at least one test for
  each of your validations and at least one test for every method in
  your model.

I added emphasis on something I find very important, you should test each validation on it's own. That way it's easier to see every rules your code have. By looking at your tests, I should easily see what your code is about and what I should expect to see.

When you want to comment, refactor

Most of time when you add a comment it's a sign that you should refactor (this is not an absolute rule, but it's apply in a lot of case).

# Test Inclusion of city
s.city = "Santo"
assert_not s.valid?, "Address was valid with incorrect city"

Do you see what you did there ? It's a test case inside your test. If you need to reuse some constant or variable in a test, there are ways to do this without being in the same method. You can setup data in a method and call it in your test methods to keep your code clean.

You should have :

  test "shipping addresses validations" do
    s = ShippingAddress.create
    assert_not s.valid?, "Address was valid without any data"
    assert s.errors.include? :name
    assert s.errors.include? :phone
    assert s.errors.include? :address
    assert s.errors.include? :user
    assert s.errors.include? :city
  end

  test "inclusion of city" do
    s = ShippingAddress.create
    s.name = "Some"
    s.phone = "8296964737"
    s.address = "Calle #1"
    s.user_id = 1
    s.city = "Santo"
    assert_not s.valid?, "Address was valid with incorrect city"

  end

You should have a test for each validation of the fields of your class where only the one field is missing. That way if your validation on the name per example changed, then that tests failed. If you only do one tests with everything you may not be able to trap that one validation has been remove.


Project Euler #11"numbers in a grid" in Rust To learn more about Rust, I implemented Project Euler #11 "Numbers in a grid". Contrary to some people, I prefer not having the actual data grid in my code, so I put it in a file, which I read in the beginning.

I'm wondering how 'Rustic' my code is. Specially I wonder about the following points:

How to most concisely read a table of integers into a data structure? Is it possible to create an array of a certain size? Do I need all this spliting and from_str mapping?
Have I handled the array indices in the right way? I'm not used to needing all this casting.
I my max function sufficiently generic?

use std::io;
fn max(a:int, b:int) -> int {if a > b {a} else {b}}
fn main() {
    let n = 20;
    let mut ar:Vec<Vec<int>> = vec![];
    for line in io::stdin().lines() {
        let s = line.unwrap();
        let ss = s.as_slice().trim().split(' ');
        let xs:Vec<int> = ss.map(|s|from_str::<int>(s).unwrap()).collect();
        ar.push(xs);
    }
    let mut best = 0;
    for i in range(0i,n) {
        for j in range(0i,n) {
            for &(di, dj) in [(0,1),(1,1),(1,0),(1,-1i)].iter() {
                if i+di*3 < n && j+dj*3 < n && j+dj*3 >= 0 {
                    let mut p = 1;
                    for k in range(0i,4) {
                        p *= ar[(i+di*k) as uint][(j+dj*k) as uint]
                    }
                    best = max(best, p);
                }
            }
        }
    }
    println!("{}", best);
}

Just like most people not actively involved in the language design, I'm still finding my feet with Rust as well, so take what I have to say with a large grain of salt.

You don't need to define your own max function: there is one already in the standard library, in std::cmp to be exact:

pub fn max<T: Ord>(v1: T, v2: T) -> T

This will work for anything that has an impl Ord, which int certainly does.

You can avoid the casts if you simply read everything in as a uint in the first place. In fact, it's probably a good idea to split up the code reading and parsing the file from the code that does the calculation. I'd write it something like:

use std::io;

fn read_grid(path: &str) -> Vec<Vec<uint>> {
    let path = Path::new(path);
    let mut file = io::BufferedReader::new(io::File::open(&path));
    let mut lines = file.lines().map(|x| x.unwrap());
    let mut grid: Vec<Vec<uint>> = vec![];

    for line in lines {
        let option = line.as_slice().split(' ').map(|x| -> Option<uint> from_str(x.trim()));
        grid.push(option.filter(|x| x.is_some()).map(|x| x.unwrap()).collect());
    }
    grid
}

This will also skip over any non-convertible values (that is, anything that from_str(x.trim()) gives None from.

I don't doubt there are still much better ways of writing this, but that's about where I'm at for now.


Leetcode container with most water I was doing container with most water question in Leetcode for which I wrote this algorithm

Question link: https://leetcode.com/problems/container-with-most-water

Question:

  Given n non-negative integers a1, a2, ..., an , where each represents
  a point at coordinate (i, ai). n vertical lines are drawn such that
  the two endpoints of line i is at (i, ai) and (i, 0). Find two lines,
  which together with x-axis forms a container, such that the container
  contains the most water.

  Note: You may not slant the container and n is at least 2.

Input: [1,8,6,2,5,4,8,3,7]

/**
 * @param {number[]} height
 * @return {number}
 */

var maxArea = function(height) {
    let currentHighest=0
    while(height.length) {
        const num = height.shift()
        let j=0
        while(j<height.length) {
            const lowerNum = Math.min(num, height[j])
            const distance = j+1
            const area = distance * lowerNum
            currentHighest = Math.max(currentHighest,  area)
            j++
        }
    }
    return currentHighest
};

but it appears so this code isn't opimized, Can someone help me in optimizng the above code?

Ok, so I made implementation very similar to yours with these modifications:

input is not being destroyed (althout it can still be adjusted to destroy it, but it just feels like such algorithm should not do this)
I compute maximum potential container area for values as I iterate them from left
I iterate the inner loop from end towards front
I use the potential to reduce the number of iterations of the inner loop.

function maxArea (input)
{
    // result value will go here
    let currentMax = 0;

    // we will need this value at least 2*(n-1) times:
    const limit = input.length - 1;

    // i<limit because having i=limit would always yield zero size container so skip it:
    for(let i = 0; i < limit; ++i) {
      const left = input[i];

      // zero size edge always yields zero size container so move to next i
      // this check may be omitted as the next will exclude it as well
      //if (left == 0) continue;

      // the largest container we can have on the right has this area:
      let potential = left * (limit - i);

      // if potential is not enough to get over currentMax, just move to next i
      if (potential <= currentMax) continue;

      // search from end towards i becase more distant j is more likely to yield larger container and only the last j can actualy yield current potential
      for (let j = limit; j > i; --j) {
        const right = input[j];

        // if right is at least as high as left then this container has the area equal to potential and no other j will do better for current left value
        if (right >= left) {
          currentMax = potential;
          break;
        }

        // if it is smaller we check if its area is greater then currentMax and use it if it is
        const area = right * (j - i);
        if (area > currentMax) currentMax = area;

        //decrease potential by 1 times left value because we will reduce distance by 1 for next iteration (--j):
        potential -= left;

        // if new potential is not enough to get over currentMax we can stop searching further j's and move to next i
        if (potential <= currentMax) break;
      }
    }
    return currentMax;
}

In conjuction this algorithm is still (like yours) O(n^2) in time and O(1) in space. But it is a  better n^2 then yours, I believe. And (unlike yours) the actual performance will be different for different input values, not only for different lengths of the input array. It is same as yours in worst case (ie strictly decreasing sequence) and actualy O(n) in best case (ie increasing sequence). And of course anywhere in between for most of the possible variations of input values.

EDIT: hehe ok I benched it on random 100k value arrays and mine takes about 4ms, yours 15+ seconds :) The worst case scenario is about 15 seconds yours, 7 seconds mine. But the worst case scenario is very small set of all posibilities, so to get it from randomness is very unlikely. Randomness holds it on few ms. So improvement by factor of several thousands for random 100k value inputs :)


Sherlock and Anagrams Optimization From Hackerrank:

  Given a string S, find the number of "unordered anagrammatic pairs" of substrings. In other words, find the number of unordered pairs of substrings of  S that are anagrams of each other.

  Two strings are anagrams of each other if the letters of one string can be rearranged to form the other string.

  Input Format

  First line contains T, the number of testcases. Each testcase consists of string  S in one line.

  Constraints

  1 <= t <= 10

  2 <= length(s) <= 100

  String S contains only the lowercase letters of the English alphabet.

  Output Format

  For each testcase, print the required answer in one line.

I am losing two test cases with Time Limit Exceeded. Can somebody please tell me how to optimize the code?

#include <bits/stdc++.h>

using namespace std;

int sherlockAndAnagrams(string s)
{

    unordered_map<char,int> count1,count2;
    unordered_map<char,int>::iterator it;
    int l=s.length();
    int len1,len2;
    int wall1,wall2;
    int cnt=0;
    int flg=0;
    int i,j;
    int dflg;
    wall1=0,wall2=l-1;
    for(wall1=0;wall1<l-1;++wall1)
    {
        len1=1;
        count1.clear();
        for(i=wall1;i<l-1;++i,++len1)
        {
            ++count1[s[i]];
            for(wall2=l-1;wall2>wall1;--wall2)
            {
                len2=1;
                count2.clear();
                flg=0;
                dflg=0;
                for(j=wall2;j>wall1;--j,++len2)
                {
                    ++count2[s[j]];
                    if(len2>len1)
                        break;
                    if(len2==len1)
                    {
                        dflg=1;
                        for(it=count1.begin();it!=count1.end();++it)
                        {
                            if(it->second!=count2[it->first])
                            {
                                flg=1;
                                break;
                            }
                        }
                        break;
                    }
                }
                if(dflg==1 && flg==0)
                        ++cnt;
            }
        }
    }
    return cnt;
}

int main() {
        int q;
        cin >> q;
        for(int a0 = 0; a0 < q; a0++){
            string s;
            cin >> s;
            int result = sherlockAndAnagrams(s);
            cout << result << endl;
        }
        return 0;
    }

Your problem is not in the code, but in your algorithm. Brute-forcing the solution as you are doing is fundamentally slow, and you need to tackle the problem from a different angle if you want to make a substantial dent in your runtime.

The problem is constrained in two specific ways we can exploit:

Only letters in the 'a'->'z' range are admissible. That's only 26 letters.
Only strings of length <= 100 characters are admissible. By extension, this means we'll never have more than 100 substrings of a given length to deal with.

Now, if I had a function that converts a substring into "some" representation that's common between annagrammic pairs, but only between annagrammic pairs, I could simply generate that representation for each substring and work off that datastructure instead (probably by using it as a key to a map of number of instances seen).

Since we only have 26 possible letters, a simple std::array<uint8_t, 26>, where each cell represents the number of times a given letter appears in the substring, has all the properties we need.

Just to make sure I'm not spouting nonsense, I gave it a try, and my solution using these principles solves all the hackerrank test cases in under 10ms.

I encourage you to give it a try by yourself, but if you request it, I'll be glad to post the code for my solution here.


Simple Haskell key value file store As an exercise in learning Haskell, I implemented a simple key value store, where you can put and get values (as ByteStrings). (For reference this is inspired by this short note describing bitcask's design). I'd appreciate feedback on any aspect (style, 'haskellyness', library usage etc.) of my implementation:

{-# LANGUAGE OverloadedStrings #-}

module KV (
    evalKV,
    putKV,
    getKV,
    liftIO
) where

import Prelude hiding (mapM)
import Data.Traversable (mapM)
import Control.Monad (liftM, liftM2)
import Control.Monad.State (StateT, evalStateT, get, put, liftIO)
import qualified System.IO as IO
import Data.ByteString.Lazy (ByteString)
import qualified Data.ByteString.Lazy as L
import qualified Data.Binary as B
import qualified Data.Binary.Get as G
import qualified Data.Map as M

type Header    = (Integer, Integer)
               -- keysize  valuesize

type ValueInfo = (Integer, Integer)
               -- offset valuesize

type Key       = ByteString

type Pair      = (Key, ValueInfo)

type Index     = M.Map Key ValueInfo

data KVState = KVState
    { kvHandle :: IO.Handle
    , kvIndex  :: Index
    } deriving (Show)

type KV a = StateT KVState IO a

evalKV :: FilePath -> KV a -> IO a
evalKV p s = IO.withBinaryFile p IO.ReadWriteMode $ \h -> do
    i <- readIndex h
    evalStateT s $ KVState h i

putKV :: ByteString -> ByteString -> KV ()
putKV k v = do
    (KVState h i) <- get
    vi <- liftIO $ writePair h k v
    let i' = M.insert k vi i
    put $ KVState h i'

getKV :: ByteString -> KV (Maybe ByteString)
getKV k = do
    (KVState h i) <- get
    liftIO $ lookupKV h i k

readHeader :: ByteString -> (Header, Integer)
readHeader c = (h, fromIntegral o)
    where (h, _, o) = G.runGetState B.get c 0

readAt :: IO.Handle -> Integer -> Integer -> IO ByteString
readAt h o sz = do
    IO.hSeek h IO.AbsoluteSeek o
    L.hGet h $ fromIntegral sz

readPair :: IO.Handle -> Integer -> IO (Maybe Pair)
readPair h o = do
    IO.hSeek h IO.AbsoluteSeek o
    b <- L.hGet h 10 -- TODO: qualify this arbitrary number
    if L.null b
        then return Nothing
        else do
            let ((ksz, vsz), l) = readHeader b
            k <- readAt h (o + l) ksz
            return $ Just (k, (o + l + ksz, vsz))

writePair :: IO.Handle -> ByteString -> ByteString -> IO ValueInfo
writePair h k v = do
    IO.hSeek h IO.SeekFromEnd 0
    let l = fromIntegral . L.length
    let vsz = l v
    let t = (l k, vsz) :: Header
    L.hPut h (B.encode t)
    L.hPut h k
    p <- IO.hTell h
    L.hPut h v
    return (p, vsz)

readIndex :: IO.Handle -> IO Index
readIndex h = liftM M.fromList $ ri 0
    where ri o = do
            mp <- readPair h o
            case mp of
                Just p@(k, (vo, vsz)) -> do
                    t <- ri (vo + vsz)
                    return $ p : t
                Nothing -> return []

lookupKV :: IO.Handle -> Index -> ByteString -> IO (Maybe ByteString)
lookupKV h i k = mapM r mv
    where r = uncurry $ readAt h
          mv = M.lookup k i

And usage would look like:

{-# LANGUAGE OverloadedStrings #-}

import KV
import Data.ByteString.Lazy.Char8

main = evalKV "test" $ do
    u <- getKV "asdf"
    liftIO $ print u
    putKV "asdf" "qwer"
    v <- getKV "asdf"
    liftIO $ print v

Which would result in the following output:

Nothing
Just (Chunk "qwer" Empty)

One concern I have, in particular, is that I've currently made no consideration for allowing concurrent access to the store, if people have opinions on the best approach for this I'd be really interested to hear them. I have this concern as, if I get time to work on this some more, my intention is to create a way to access the store over HTTP/TCP.

Why not:

type KeySize   = Integer
type ValueSize = Integer
type Header    = (KeySize, ValueSize)

Instead of commenting, like you did here:

type Header    = (Integer, Integer)
               -- keysize  valuesize

Same about ValueInfo.


Filling text boxes with player names received from an array I'm currently trying to make the code of an app I'm developing a bit more efficient and easier to read. Basically what this does is after the user taps with two fingers (gesture) it retrieves a previously saved array from NSUserDefaults of player names, and fills in the 6 text boxes (tagged 6-11) on a table view with these names. If there isn't an existing array it'll use a default set of names.

I'm trying to reduce the length of a long for/if statement.  Any ideas for simplifying this code would be appreciated.

if (gestureRecognizer.state == UIGestureRecognizerStateEnded) {
        NSMutableArray *names = [[NSMutableArray alloc] initWithArray:[[NSUserDefaults standardUserDefaults] objectForKey:@"nameArray"]];
        for (int i = 0; i <= 5; i++) {
            NSIndexPath *indexPath = [NSIndexPath indexPathForRow:i inSection:0];
            UITableViewCell *cell = [playerTable cellForRowAtIndexPath:indexPath];

            for (UIView *view in cell.contentView.subviews) {
                if ([view isKindOfClass:[UITextField class]]) {
                    UITextField *txtField = (UITextField *)view;
                    if (txtField.tag == 6) {
                        if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
                            txtField.text = [names objectAtIndex:0]; }
                        else {
                            txtField.text = @"Peter";
                        }
                    }
                    if (txtField.tag == 7) {
                        if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
                            txtField.text = [names objectAtIndex:1]; }
                        else {
                            txtField.text = @"Julia";
                        }
                    }
                    if (txtField.tag == 8) {
                        if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
                            txtField.text = [names objectAtIndex:2]; }
                        else {
                            txtField.text = @"Durgan";
                        }
                    }
                    if (txtField.tag == 9) {
                        if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
                            txtField.text = [names objectAtIndex:3]; }
                        else {
                            txtField.text = @"Bob";
                        }
                    }
                    if (txtField.tag == 10) {
                        if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
                            txtField.text = [names objectAtIndex:4]; }
                        else {
                            txtField.text = @"Iseland";
                        }
                    }
                    if (txtField.tag == 11) {
                        if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
                            txtField.text = [names objectAtIndex:5]; }
                        else {
                            txtField.text = @"Player";
                        }
                    }
                }
            }
        }
        [self saveNames];
    }

Edit: I've received an answer elsewhere and it greatly reduces the length as well as making it more readable:

NSArray *defaultNames = @[@"Peter", @"Julia",...];
int offsetIndex = 6;
BOOl needCustomNames = [[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"];
for (UIView *view in cell.contentView.subviews)
{
    if ([view isKindOfClass:[UITextField class]])
    {
        UITextField *txtField = (UITextField *)view;
        int index = [txtField tag]-offsetIndex;
        if (txtField.tag >= 6 && txtField.tag <= 11)
        {
            if (needCustomNames)
                txtField.text = [names objectAtIndex:index];
            else
                txtField.text = [defaultNames objectAtIndex:index];
        }
    }
}

We can do better still.

First of all, we're talking about a UITableViewCell to which you've added custom views--or at least modified existing views (hence the tags) as well as adding a gesture recognizer.

What makes most sense is to instead subclass UITableViewCell.  This would help both to clean up the method you have posted here as well as clean up the cellForRowAtIndexPath: which I'm sure is a bit of a mess.

Then, you would no longer need to iterate through all of the content view's subviews.  Instead, best case scenario, you only have a single text field on the cell, and you can access it directly, and switch on its tag, and at worst, you can create a custom IBCollection--an array of text fields--that would be relevant to this method and iterate over just that (which you know would contain only text fields relevant to this method) looking at the tags.

int index = [txtField tag]-offsetIndex;
if (tag >= 6 && tag <= 11)
{
    if (needCustomNames)
        textField.text = [names objectAtIndex:index];
    else
        textField.text = [defaultNames objectAtIndex:index];
}

This chunk of code is confusing.

First of all, tag isn't declared anywhere.

Second of all, presuming the if is supposed to be checking textField.tag, you should either move the int index line inside the if block, or probably better, refactor the if to check the index that was just calculated based on textField.tag and is the value actually used inside the if block.  As it stands, this code might crash if someone unknowingly changes the offsetIndex 7 lines previously and that might be a hard crash to figure out.

I think there's some more cleanup to do, but I'm kind of interested in seeing whether you'd be interested in posting a new question where you've rewritten the code to use a UITableViewCell subclass, which truly would be the best option.


Validate particular id's depending on what process type is coming I have code where I validate deviceId and clientId basis on what ProcessType is coming:

  public static boolean isValid(ProcessType processType, Record record) {
    String deviceId = DataUtils.extract(record, "deviceId");
    String clientId =DataUtils.extract(record, "clientId");

    if(processType == ProcessType.ABC || processType == ProcessType.DEF) {
        if (Strings.isNullOrEmpty(clientId) || Strings.isNullOrEmpty(deviceId)) {
        LogMetric.getInstance().increment(processType.name(), Metric.ID);
        logger.logError("invalid clientId and deviceId provided. ProcessType= ", processType,
            ", Record= ", record);
          return false;
        }
    } else {
        if (Strings.isNullOrEmpty(clientId) && Strings.isNullOrEmpty(deviceId)) {
        LogMetric.getInstance().increment(processType.name(), Metric.ID);
        logger.logError("invalid clientId and deviceId provided. ProcessType= ", processType,
            ", Record= ", record);
          return false;
        }
    }
    return true;
  }

If ProcessType is coming as ABC or DEF, then I will return false if clientId or deviceId is missing
Otherwise I will return false if both clientId and deviceId are missing.
If all the conditions are not met, then I will return true.

I wanted to see if there is any better way to write this. I'm working with Java 7.

I was not able to tell what your isValid method did upon first reading.  I therefore renamed the method to be more explicit about what the method is validating and changed the parameter order to reflect the new name.

I abstracted out the Logging and ID extraction into their own methods to make the code more readable and follow the single responsibility principle.  Now, each method works on the same level of abstraction and has a defined role.

I abstracted out the actual record validation checks ( i.e. isRecordValidForABCOrDEFProcessType ) to encapsulate the checking logic and also make the checks readable.  The longer method names make the code self documenting.

Finally, I gave the boolean expressions names as the expressions are the most vital logic of the record validation.  The logic will also be reread often, so you want the logic to be as clear as possible.

  public static boolean isRecordValidForProcessType( Record record, ProcessType processType ) {
    boolean isProcessTypeABCOrDEF = processType == ProcessType.ABC || processType == ProcessType.DEF;
    boolean isRecordValid = isProcessTypeABCOrDEF
      ? isRecordValidForABCOrDEFProcessType( record )
      : isRecordValidForNonABCOrDEFProcessType( record );

    if( isRecordValid == false )
        logInvalidRecordErrorForProcessType( record, processType );

    return isRecordValid;
  }

  private static boolean isRecordValidForABCOrDEFProcessType(Record record){
    String deviceId = getDeviceIdFromRecord(record);
    String clientId = getClientIdFromRecord(record);
    boolean doesRecordHaveValidIds = ( Strings.isNullOrEmpty(clientId) || Strings.isNullOrEmpty(deviceId) ) == false;
    return doesRecordHaveValidIds;
  }

  private static boolean isRecordValidForNonABCOrDEFProcessType(Record record) {
    String deviceId = getDeviceIdFromRecord(record);
    String clientId = getClientIdFromRecord(record);
    boolean doesRecordHaveValidIds = ( Strings.isNullOrEmpty(clientId) && Strings.isNullOrEmpty(deviceId) ) == false;
    return doesRecordHaveValidIds;
  }

  private static String getDeviceIdFromRecord(Record record) {
    return DataUtils.extract(record, "deviceId");
  }

  private static String getClientIdFromRecord(Record record) {
    return DataUtils.extract(record,"clientid");
  }

  private static void logInvalidRecordErrorForProcessType(Record record, Process processType){
    LogMetric.getInstance().increment(processType.name(), Metric.ID);
      logger.logError("invalid clientId and deviceId provided. ProcessType= ", processType,
                      ", Record= ", record);
  }


Best way to optimize multiple search on Lists java Anyone could help me to optimize this block of code, I already did a bit improvement but since I'm really bad with this kind of algorithms this all I've done so far. Much appreciate if you can explain the improvement.

This basically fill a bus (k is the number of seats on the bus) if all seats are filled save the last index of the person who gets into the bus.

p is a list of persons waiting the bus (each person has patience limit) q is a list of time the bus will take to arrive.

Example;

k=2
p={1,4,2,5}
q={1,4}

On the first time bus will arrive in time 1 so person 1 and 2 will fill the bus and index of person will be saved in a list (r={2}).

On the second time bus will arribe in time 4, person 1 and 3 leave person 2 and 4 fill the bus (r={2,4}).

If the bus can't be filled then the number saved in the list is 0.

public static List<Integer> busFilled(int k, List<Integer> p, List<Integer> q) {

    List<Integer> busSeats = new ArrayList<>();

    // Sort the times of bus arrival
    Collections.sort(q);

    // Loop each bus arrival
    for(int time : q) {
        // If person patience < time set the patience to 0
        for (int i=0; i<p.size()-1; i++) {
            if (p.get(i) < time) {
                p.set(i, 0);
            }
        }

        int aux = 0;
        // Loop each person patience
        for (int i=0; i<=p.size()-1; i++) {
            // If person patience > 0 count
            if ( p.get(i) > 0) {
                aux++;
            }

            // If aux = bus seats add the last number(index) of person who filled the bus
            if (aux == k) {
                busSeats.add(i+1);
                break;
            // If i == total persons in queue set 0
            } else if (i==p.size()-1){
                busSeats.add(0);
            }
        }
    }
    return busSeats;
}

Problem Statement

This is my understanding of the problem statement.

We have a bus that has k seats.
There is a list of people p waiting at a bus stop.  The value in each position of the list is the amount of time the person is willing to wait for a bus.  The list is replenished for each scheduled time the bus arrives.  The wait times do not change with new people.
There is a list of scheduled times q that the bus will arrive.

The method busFilled returns a List of the index of the last person to get on the bus each time the bus arrives at the bus stop.  There should be the same number of elements in the output List as there are in the list of scheduled times q.

If no one can fit on the bus, the List should return zero in that position.

Analysis

I approached the problem the same way I approach any problem.  I break the problem down into smaller and smaller steps until I can write code for each of the steps.

Here's the code I wrote.  The explanation follows the code.

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class BusSimulation {

    public static void main(String[] args) {
        int k = 2;
        int[] people = { 1, 4, 2, 5 };
        List<Integer> p = createList(people);
        int[] busTime = { 1, 4 };
        List<Integer> q = createList(busTime);
        List<Integer> f = busFilled(k, p, q);
        Integer[] output =
                f.toArray(new Integer[0]);
        System.out.println("Result: " +
                Arrays.asList(output));
    }

    public static List<Integer> createList(int[] array) {
        List<Integer> output = new ArrayList<>();
        for (int i = 0; i < array.length; i++) {
            output.add(array[i]);
        }
        return output;
    }

    public static List<Integer> busFilled(int k,
            List<Integer> p, List<Integer> q) {
        List<Integer> output = new ArrayList<>();

        for (int i = 0; i < q.size(); i++) {
            int busTime = q.get(i);
            int index = getPassengerIndex(k, p, busTime);
            output.add(index);
        }

        return output;
    }

    private static int getPassengerIndex(int k,
            List<Integer> p, int busTime) {
        int count = 0;
        int index = -1;

        for (int i = 0; i < p.size(); i++) {
            if (busTime <= p.get(i)) {
                index = i;
                count++;
                if (count >= k) {
                    return index + 1;
                }
            }
        }

        return index + 1;
    }

}

The main method and the createList method set up the problem.

I created a separate method, getPassengerIndex, to get the passenger index for one bus trip.  That way, I avoided a nested for loop and worrying about how to break out of the nested for loop.

The getPassengerIndex method loops through the passenger list.  First, I test to see if the waiting time has been exceeded.  If not, then I save the index and increment the count of the number of passengers boarding the bus.  Finally, I test for the bus being filled.  If so, I return the passenger number.  If the bus isn't filled, I return the last passenger number whose waiting time has not been exceeded.

I could test getPassengerIndex separate from the rest of the code.  Once I got that method working, writing the busFilled method was straight forward.

Decomposition, or divide and conquer, is the most straightforward way to solve problems.