Onset detection using FFT I am using this onset source code and this fft file to try to perform onset detection. Currently the code is working, but as my DSP and C skills are subpar I could use some advice on how to improve it.

My main points of insecurity are the FFT (maybe there is a more suitable method out there?) and the C code. All advice appreciated!

NB: This code will eventually being going into iPhone project.

OnsetsDS  *ods = malloc(sizeof *ods);
float* odsdata = (float*) malloc(onsetsds_memneeded(ODS_ODF_RCOMPLEX, 512, 11));
onsetsds_init(ods, odsdata, ODS_FFT_FFTW3_HC, ODS_ODF_RCOMPLEX, 512, 11, 44100);

int i;
int x;
double (*vocalData)[2] =  malloc(2 * 512 * sizeof(double));
double (*doubleFFTData)[2] =  malloc(2 * 512 * sizeof(double));

for (i = 0; i < vocalBuffer.numFrames; i=i+512){

    bool onset;
    for (x = 0; x < 512; x++){
        *vocalData[x] = (double)vocalBuffer.buffer[i+x];
    }

    fft(512, vocalData, doubleFFTData);
    float *floatFFTData = malloc(512 * sizeof(float));
    int z;
    for (z = 0; z < 512; z++){

        floatFFTData[z] = (float)*doubleFFTData[z];
    }

    onset = onsetsds_process(ods, floatFFTData);

    if (onset){
        printf("onset --> %i\n", i);
    }
}

free(ods->data); // Or free(odsdata), they point to the same thing in this case
return nil;

The link in the post to your fft file is broken, so I can't see the function
prototype.  This is a pity, because it is otherwise difficult to know what to
make of your arrays:

double (*vocalData)[2] =  malloc(2 * 512 * sizeof(double));

This says vocalData is a pointer to an array of two doubles.  You have
allocated 1024 doubles at this address.  So on a 64bit system you have:

                  8 bytes
vocalData[0] ->  |--------|
                 |--------|
vocalData[1] ->  |--------|
                 |--------|
vocalData[2] ->  |--------|
                 |--------|
    etc

You fill the array with

for (x = 0; x < 512; x++){
    *vocalData[x] = (double)vocalBuffer.buffer[i+x];
}

This fills alternate doubles in your allocated space and leaves the others
uninitialized.

                  8 bytes
vocalData[0] ->  |xxxxxxxx|
                 |--------|
vocalData[1] ->  |xxxxxxxx|
                 |--------|
vocalData[2] ->  |xxxxxxxx|
                 |--------|
    etc

I don't know for sure that this is not what you want, but...

Also all that converting from float to double and then back to float is very
inefficient.  As onsetsds_process takes floats, I can see the need, but
maybe you should use an FFT function that also deals in floats.

Finally it would be better to #define FFT_SIZE 512 at the top and use
FFT_SIZE throughout instead of the explicit 512.  This is better practice,
for example making it much easier to change the FFT length if it is necessary.

Oh and don't forget to free any memory you have allocated once you have
finished with it.  In this case, that means all of your allocations should be
freed on exit (although if your return 0 is the end of the program that is
academic).


Input Iterator implementation I need some review on my implementation of input iterator. I am currently re-implementing std::vector, and need input_iterator for a specific constructor.
Any criticism is much appreciated! Thank you.
#ifndef INPUT_ITERATOR
#define INPUT_ITERATOR

template<typename T>
class input_iterator
{
private:
    T* input_iter;

public:
    using reference = T&;
    using const_reference = const T&;
    using value_type = T;
    using pointer = T*;
    using const_pointer = const T*;

    constexpr input_iterator() noexcept : input_iter{ nullptr } {}

    constexpr explicit input_iterator(pointer inputIter) noexcept : input_iter{ inputIter } {}

    constexpr explicit input_iterator(const input_iterator& other) noexcept : input_iter{ other.input_iter } {}

     ~input_iterator() noexcept = default;

    constexpr reference operator=(const input_iterator& other){
        if (input_iter != other.input_iter)
            input_iter = other.input_iter;
        return *this;
    }

    constexpr const_reference operator*() const {
        return *this;
    }

    constexpr const_pointer operator->() const {
        return this;
    }

    constexpr input_iterator& operator++() {
        ++input_iter;
        return *this;
    }

    constexpr input_iterator& operator++(int) {
        input_iterator current{ input_iter };
        ++(*this);
        return current;
    }

    constexpr bool operator==(const input_iterator& other) const noexcept {
        return (input_iter == other.input_iter);
    }

    constexpr bool operator!=(const input_iterator& other) const {
        return !(input_iter == other.input_iter);
    }

};

#endif
```

Input iterator is an abstract concept
I think you are very confused.
“Input iterator” is a concept. A concept is an abstract specification, not a concrete “thing”. You can’t make an “implementation of input iterator”; that’s gibberish. You can implement something that satisfies the input iterator concept, but you can’t implement input iterator itself.
It’s kinda like saying: “I’m making an implementation of container”. The only valid response to that statement is a blank look and the question: “Okay… WHICH container? A linked list? A hash table? A double-ended queue?” You can’t just make ‘container’; it’s an abstract concept. You can implement something that is a container (that is, something that satisfies the abstract specification of “container”), but you can’t actually “implement container”… that’s gibberish.
You say you “need input_iterator for a specific constructor”… I think you mean you need AN input iterator for a specific constructor; you need a type that satisfies the input iterator concept, not specifically a type that “is” input iterator (because that’s gibberish).
If you need an input iterator, you don’t need to make one. It’s a concept. Anything that satisfies the concept will do. For example, in the standard library, std::istream_iterator is an input iterator:
#include <forward_list>
#include <iostream>
#include <iterator>
#include <list>
#include <sstream>
#include <vector>

template <typename T>
void foo(T, T)
{
    std::cout << "foo called with some type\n";
}

template <std::forward_iterator T>
void foo(T, T)
{
    std::cout << "foo called with forward iterators (or better)\n";
}

template <std::input_iterator T>
void foo(T, T)
{
    std::cout << "foo called with input iterators\n";
}

auto main() -> int
{
    auto vec = std::vector<int>{};
    auto list = std::list<int>{};
    auto slist = std::forward_list<int>{};

    auto iss = std::istringstream{};

    foo(0, 1);
    foo(vec.begin(), vec.end());
    foo(list.begin(), list.end());
    foo(slist.begin(), slist.end());
    foo(std::istream_iterator<int>{iss}, std::istream_iterator<int>{});
}

Output:
$ ./a.out
foo called with some type
foo called with forward iterators (or better)
foo called with forward iterators (or better)
foo called with forward iterators (or better)
foo called with input iterators
$

I can’t quite make sense of what you think you mean when you say you “need input_iterator for a specific constructor”, but if you mean you need to test a specific constructor with input iterators, then you can just do:
auto iss = std::istringstream{"5 4 3 2 1"};

auto v = your_vector<int>(std::istream_iterator<int>{iss}, std::istream_iterator<int>{});
// If the above constructor works as expected, v should now contain: 5, 4, 3, 2, 1

If you mean you need to implement a specific constructor with input iterators, then that’s just:
template <typename T>
class your_vector
{
    // ... [snip] ...

    template <typename T>
    constexpr your_vector(T first, T last)
    {
        while (first != last)
            this->push_back(*first++);
    }

    // ... [snip] ...
};

That’s a constructor that takes input iterators.
Of course, before C++20 it’s standard practice to name the type with something more descriptive than T if you’re using a concept, like:
template <typename T>
class your_vector
{
    // ... [snip] ...

    template <typename InputIterator>
    constexpr your_vector(InputIterator first, InputIterator last)
    {
        while (first != last)
            this->push_back(*first++);
    }

    // ... [snip] ...
};

And as of C++20, you can now actually check that the type satisfies the input iterator concept, because the standard library has a std::input_iterator concept defined already:
template <typename T>
class your_vector
{
    // ... [snip] ...

    template <std::input_iterator It>
    constexpr your_vector(It first, It last)
    {
        while (first != last)
            this->push_back(*first++);
    }

    // ... [snip] ...
};

That’s it. That’s all you need. That’s implementing a constructor with input iterators, and above I show you how to test a constructor with input iterators. You don’t need to make an “input iterator” type, because the standard library already has input iterator types (and, as of C++20, a std::input_iterator concept to check them, to make sure they really are valid input iterators).
Your type is dangerous and buggy
So what you’re trying to do is take a plain pointer—which is a contiguous iterator—and strip its power, to reduce its capability until it looks like an input iterator.
In VERY limited circumstances, this isn’t a terrible idea. For example, if you’re making an algorithm that works differently with input iterators than with forward iterators or better, and you want to test that the right implementation is being used, you could use a type like this to take the pointers to an array or vector data, and forcibly pretend they’re only input iterators.
Of course… you could also just use actual input iterators, for example, by creating an istringstream, and then using istream_iterators. That would be less work, and zero chance of bugs and other headaches.
Where this type gets dangerous is that it will only work for very limited circumstances… and where it won’t work, it will silently trigger undefined behaviour.
This is okay:
auto data = std::vector<int>{};
// fill data with values...

auto first = input_iterator<int>(&data.front());
auto last  = input_iterator<int>(&data.front() + data.size());

any_algorithm(first, last);

This is not:
auto data = std::deque<int>{};
// fill data with values...

auto first = input_iterator<int>(&data.front());
auto last  = input_iterator<int>(&data.front() + data.size());

any_algorithm(first, last);

The only thing that changed is the type of the container. But while std::deque is random-access, it is NOT contiguous, so trying to iterate through it using pointer arithmetic (which input_iterator does internally) is a quick path to UB.
(In fact, the code above will not work for any container except std::vector. Trying it with std::list or anything else is asking for UB.)
Iterators are attached to specific container types for a reason; you can’t just make an iterator out of the æther with no associated container. Again, that’s nonsensical. An iterator needs to know the correct way to move from one element to the next… no, simply skipping to the next address in memory is NOT the correct way to do it, usually.
It is possible to implement an iterator adaptor that can take an iterator and fake it as a more restrictive category. But if you’re going to do that, you have to do it carefully. Simply degrading to a pointer is not the right way to do it.
Your type is not an input iterator… or an iterator at all
Okay, so you can’t implement an abstract concept. But you can implement something that models the abstract concept. You can’t “implement input iterator”, but you can implement something that models input iterator. Does your class model input iterator?
No. It’s closer to a forward iterator, but it isn’t even that.
To see what I mean, here’s a simple program that just prints the name of an iterator’s category, tested with simple pointers, std::istream_iterator, and your input_iterator:
#include <iostream>
#include <iterator>
#include <string_view>
#include <type_traits>

#include <your-input-iterator-declaration>

namespace detail_ {

template <typename T, typename = void>
struct iter_cat_impl_2_
{ using type = std::iterator_traits<T>::iterator_category; };
#if defined(__cpp_lib_ranges) and __cpp_lib_ranges >= 201911L
template <typename T>
struct iter_cat_impl_2_<T, std::void_t<typename std::iterator_traits<T>::iterator_concept>>
{ using type = std::iterator_traits<T>::iterator_concept; };
#endif // defined(__cpp_lib_ranges) and __cpp_lib_ranges >= 201911L

struct dummy_t {};

template <typename T, typename = void>
struct iter_cat_impl_1_ { using type = dummy_t; };
template <typename T>
struct iter_cat_impl_1_<T, std::void_t<typename std::iterator_traits<T>::iterator_category>> : iter_cat_impl_2_<T> {};

using namespace std::string_view_literals;

template <typename T>
inline constexpr auto iter_cat_name = "<not an iterator>"sv;
template <>
inline constexpr auto iter_cat_name<std::input_iterator_tag> = "input iterator"sv;
template <>
inline constexpr auto iter_cat_name<std::output_iterator_tag> = "output iterator"sv;
template <>
inline constexpr auto iter_cat_name<std::forward_iterator_tag> = "forward iterator"sv;
template <>
inline constexpr auto iter_cat_name<std::bidirectional_iterator_tag> = "bidirectional iterator"sv;
template <>
inline constexpr auto iter_cat_name<std::random_access_iterator_tag> = "random access iterator"sv;
#if defined(__cpp_lib_ranges) and __cpp_lib_ranges >= 201911L
template <>
inline constexpr auto iter_cat_name<std::contiguous_iterator_tag> = "contiguous iterator"sv;
#endif // defined(__cpp_lib_ranges) and __cpp_lib_ranges >= 201911L

} // namespace detail_

template <typename T>
inline constexpr auto iter_cat_name = detail_::iter_cat_name<typename detail_::iter_cat_impl_1_<T>::type>;

auto main() -> int
{
    std::cout << "int*                  = " << iter_cat_name<int*>                       << '\n';
    std::cout << "istream_iterator<int> = " << iter_cat_name<std::istream_iterator<int>> << '\n';
    std::cout << "input_iterator<int>   = " << iter_cat_name<input_iterator<int>>        << '\n';
}

Output:
$ ./a.out
int*                  = contiguous iterator
istream_iterator<int> = input iterator
input_iterator<int>   = <not an iterator>
$

As you can see, your type is not an iterator.
The reason your type fails to be detected as an iterator is because you are missing some key typedefs. It looks like you have confused the typedefs necessary for a CONTAINER with the typedefs necessary for an ITERATOR. An ITERATOR does not need const_pointer or const_reference… those make no sense for an iterator. It does need difference_type and iterator_category.
Once you define those typedefs, this type will “work” as an input iterator. But it will also “work” as a forward iterator (though, a well-designed algorithm will still treat it as an input iterator).
Okay, let’s dive into the actual code review.
The review
I’ll skip down to the typedefs, because everything prior to that is cool:
using reference = T&;
using const_reference = const T&; // <- nonsense typedef
using value_type = T;
using pointer = T*;
using const_pointer = const T*; // <- nonsense typedef

The two typedefs I’ve marked are nonsense for an iterator. You have confused the typedefs required for a container with the typedefs required for an iterator.
To understand why those typedefs are nonsensical, consider std::vector<int>::iterator::const_pointer. What do you think that means? std::vector<int>::iterator basically means: “I want a mutable pointer to the elements of an int vector (treated as a plain pointer)”. So std::vector<int>::iterator::const_pointer basically means: “I want a mutable pointer to the elements of an int vector no wait I changed my mind I want a const pointer to those elements (treated as a plain pointer)”. Meanwhile, there’s std::vector<int>::const_iterator, which actually means: “I want a const pointer to the elements of an int vector (treated as a plain pointer)”.
That’s another reason why const_pointer and const_reference make no sense in an iterator: that’s what const_iterators are.
In addition, if you want your type to work as an iterator, there are some other typedefs you need. You need to define difference_type, and you need to define iterator_category. The former could be std::ptrdiff_t, and the latter, since you want it to be an input iterator, should be std::input_iterator_tag.
pointer and reference should also probably be defined differently, but I’ll explain why later.
constexpr explicit input_iterator(const input_iterator& other) noexcept : input_iter{ other.input_iter } {}

~input_iterator() noexcept = default;

These are kinda pointless. If you don’t need to declare the destructor, don’t bother. And in the case of the copy constructor, defining it like this means the type is no longer trivially copyable (or movable). (The explicit keyword is also meaningless here, because a copy constructor is not a converting constructor. It makes no sense to “explicitly copy”.)
constexpr reference operator=(const input_iterator& other){
    if (input_iter != other.input_iter)
        input_iter = other.input_iter;
    return *this;
}

As above, this is kinda pointless, and it means the type is no longer trivially assignable. The implementation is also overly complicated, too, because there’s no need to check for equality before doing the assignment. What exactly would go wrong if the two pointers are the same, yet you did the assignment anyway?
constexpr const_reference operator*() const {
    return *this;
}

constexpr const_pointer operator->() const {
    return this;
}

Seems to me like all these operations, and the increment ops, too, could be noexcept.
However, both of these are wrong. Did you test the type? I can’t imagine it compiled. The reason why is that operator* on an iterator is supposed to return a reference to whatever is being pointed to. But you return *this. That’s a reference to the iterator… not to what the iterator is pointing to.
The same problem exists for operator->. You’re supposed to return the address the iterator is pointing… but you return the address of the iterator.
operator* should probably just be returning *input_iter, and operator-> should be probably returning input_iter itself.
But there’s a much deeper issue: you’re seeing cracks in your conceptual model. An iterator is basically a pointer—the abstraction is just to allow more powerful pointers; ones that can handle iterating in ways other than merely jumping to the next memory address. So you can mentally replace any iterator with T* to get a grasp on how operator* and operator-> should work.
So if you have int*, what should operator* return? The answer is int&… NOT int const&.
If you have int* const, what should operator* return? Again, int&… NOT int const&.
So with input_iterator<int>, operator* should return int&… NOT int const&.
With input_iterator<int> const, operator* should also return int&… NOT int const&.
In other words, this is the correct way to implement operator* for iterators:
constexpr reference operator*() const noexcept { return *input_iter; }

Note that it returns reference, and not const_reference (which is not a thing; iterators don’t have const_reference because it’s nonsensical). Similarly, operator-> should return pointer, not const_pointer.
Ah, but input iterators offer only read-only views of what they point to, so they should return const pointers and references. The correct way to handle this is to properly define reference and pointer… not to invent new typedefs.
reference should be defined as T const&, not T&, because if the iterator is an input iterator, a reference will be read-only. Having reference defined as T& makes no sense for an input iterator. Similarly, pointer should be T const*, not T*. If I do input_iterator<int>::pointer p = &(*i);, that should compile… that’s the point of the pointer typedef. It won’t if pointer is T* while operator* returns const&.
So your typedefs really need to be:
using value_type = T;

using reference = T const&;
using pointer   = T const*;

using difference_type = std::ptrdiff_t;

using iterator_category = std::input_iterator_tag;

That’s about it, except for:
constexpr bool operator==(const input_iterator& other) const noexcept {
    return (input_iter == other.input_iter);
}

constexpr bool operator!=(const input_iterator& other) const {
    return !(input_iter == other.input_iter);
}

You’re missing the noexcept for the inequality operator.
Standard practice is to use non-member functions for binary operators, and to define only one operator where possible and then all others in terms of that one. So:
friend constexpr bool operator==(const input_iterator& lhs, const input_iterator& rhs) noexcept {
    return lhs.input_iter == rhs.output_iter;
}

friend constexpr bool operator!=(const input_iterator& lhs, const input_iterator& rhs) noexcept {
    return not (lhs == rhs);
}

(It’s also a good idea, even though they’re non-member functions, to define them in the class as friends. This is called the “hidden friends” pattern, and it has several benefits, but it’s too much to go into here.)
Summary
The main problem here is conceptual confusion. “Input iterator” is an abstract idea. It’s a concept. You can’t make an implementation of input iterator any more than you can make an implementation of “equality comparable”. You can implement something that is equality comparable, but can’t implement the abstract concept “equality comparable”. (As of C++20, you can describe the concept, and use that to check that types satisfy the concept, but you still can’t implement the abstract concept as a type.)
So saying you are “implementing input iterator” is gibberish. Saying that you need “input iterator” for some constructor is also gibberish. Unfortunately, that’s all you’ve given, so I’m forced to guess what you really mean when you’re saying these things.
If you mean you need a constructor that takes input iterators… well, that’s just a template constructor, with a type that satisfies the input iterator concept. The type could be anything; it doesn’t need to be some concrete input_iterator type. It just needs to satisfy the concept.
The standard library has a couple types that satisfy the input iterator concept: istream_iterator and istreambuf_iterator are the two that spring to mind. So if you’re making a constructor that takes input iterators, just make a constructor template, and make sure it works with istream_iterators (and/or istreambuf_iterators).
For example, this is actually Clang 11’s implementation of std::vector’s input iterator constructor:
template <class _Tp, class _Allocator>
template <class _InputIterator>
vector<_Tp, _Allocator>::vector(_InputIterator __first,
       typename enable_if<__is_cpp17_input_iterator  <_InputIterator>::value &&
                         !__is_cpp17_forward_iterator<_InputIterator>::value &&
                         is_constructible<
                            value_type,
                            typename iterator_traits<_InputIterator>::reference>::value,
                          _InputIterator>::type __last)
{
#if _LIBCPP_DEBUG_LEVEL >= 2
    __get_db()->__insert_c(this);
#endif
    for (; __first != __last; ++__first)
        __emplace_back(*__first);
}

Ignoring the enable_if and the debug stuff, and pretending it’s defined within the class body rather than outside, that’s just:
template <class _InputIterator>
vector(_InputIterator __first, _InputIterator __last)
{
    for (; __first != __last; ++__first)
        __emplace_back(*__first);
}

That’s all there is to it.
(The enable_if just makes sure that _InputIterator actually satisfies the input iterator concept, but not the forward iterator concept. That’s because the forward iterator version uses std::distance() to figure out how many elements there are, so it can preallocate enough memory. You can’t do that with input iterators, because you only get one pass; you can’t first count the elements, allocate the size, then copy the elements, because that second pass won’t work for input iterators. It’s a neat optimization you might consider, though it’s much easier with C++20 concepts than with enable_if.)
And you can test that constructor with input iterators like this:
auto iss = std::istringstream{"42 69 57"};

auto const vec = std::vector<int>(std::istream_iterator<int>{iss}, std::istream_iterator<int>{});

TEST(vec.size() == 3);
TEST(vec.at(0) == 42);
TEST(vec.at(1) == 69);
TEST(vec.at(2) == 57);

Another issue with your design is that “generic” iterators make no sense. Iterators are tightly bound to containers (or views, or other such things). You can’t simply make an iterator that just works with any container. Indeed the only standard containers your iterator works with are std::vector and std::array. For all other containers, it will trigger UB.
And finally, if you want this type to be an actual (input) iterator, you need to add some extra typedefs, modify the existing typedefs, and fix some problems in your dereferencing operators.
But honestly, I would suggest just throwing the whole thing out, because:

It’s poorly conceived (it is trying to be a concrete implementation of an abstract idea).
It’s dangerously buggy (it only works with certain containers).
It’s unnecessary (if you want an input iterator, there are already input iterators in the standard).


Count number of islands 2d grid This is a recursive approach using DFS to counting the number of islands. However, I would like to improve performance, while keeping the code clean concise and readable. Better yet can this be solved using bitwise operations?

var numIslands = function(grid) {

  const search = (row, col, grid) => {
    if (row < 0 || col < 0 ||
      row > grid.length - 1 || col > grid[row].length - 1 ||
      grid[row][col] === '0') {
      return;
    }
    grid[row][col] = '0';
    search(row - 1, col, grid);
    search(row, col - 1, grid);
    search(row + 1, col, grid);
    search(row, col + 1, grid);
  }
  let count = 0;
  grid.forEach((row, index) => {
    row.forEach((value, i) => {
      if (value === "1") {
        search(index, i, grid)
        count++
      }
    })
  })
  return count
}
console.log(numIslands([
  ["1", "1", "1"],
  ["1", "1", "0"],
  ["1", "1", "1"],
  ["0", "1", "0"],
  ["1", "1", "1"]
]))

console.log(numIslands([
  ["1", "1", "1"],
  ["0", "0", "0"],
  ["1", "1", "1"],
  ["0", "0", "0"],
  ["1", "1", "1"]
]))

So the logic I used was to check if a cell is part of an existing island either to above (n) or to the left (w). I came up with three possibilities:

Neither n nor w is an island so start a new island, record the islands number and increment the count.
Only one of the adjoining cells is an island or both are the same island so just add it to the same island.
The adjoining cells are from different islands, merge the islands by decrementing the count.

Something like this:

let numIslands = function(grid) {
  logGrid('input', grid);

  let count = 0;
  let work = [];
  let islandNo = 0;

  grid.forEach((row, rowi) => {
    work.push( (new Array(row.length)).fill(undefined) );
    row.forEach((value, coli) => {
      if (value !== '1')
       return;

      let n       = rowi > 0 ? work[rowi-1][coli] : undefined;
      let w       = work[rowi][coli-1];
      let type;

      if (n===undefined && w===undefined) {
        type  = 'new  ';
        islandNo++;
        work[rowi][coli] = islandNo;
        count++;
      } else if (n===w || n===undefined || w===undefined) {
        type = 'exist';
        work[rowi][coli] = n || w;
      } else /* n !== w */ {
        type  = 'merge';
        work[rowi][coli] = n || w;
        count--;
      }

      // console.log(type, rowi, coli, work[rowi][coli] );

    })
  })
  logGrid('work', work);
  return count
}

   let logGrid = function(title, grid) {
 console.log('------------');
 console.log(title);
 console.log('------------');
 console.log( grid.map(r=> r.map(r=>r||'.').join('')).join("\n"));
 console.log('------------');
   }

   let test = function(grid) {
 grid = grid.map (r => r.replace(/0/g,'.').split("") );
 console.log( numIslands(grid) );
 console.log();
   }

test(['111',
      '11.',
      '111',
      '.1.',
      '111'  ]);

test(['111',
      '...',
      '111',
      '...',
      '111' ]);

test(['0000000000',
      '0111001111',
      '0001001001',
      '0101001001',
      '0101111011',
      '0000000010',
      '0110011000' ] );

Note that I don't consider diagonally adjacent cells to be the same island and this algorithm probably doesn't handle all situations well, especially hollow islands.
Just a styistic note. Use either var or let, but don't mix the two.


ActiveRecord model for users who have issues or who assist with issues I'm running into an issue while playing around with some models in a Rails app. Namely issues of the polymorphic variety. I think I may have solved it, more or less looking for an approval.

I have the following models in my app:

class Issue < ActiveRecord::Base
  belongs_to :creator, class_name: 'User', foreign_key: 'creator_id'
  has_many :user_issues
  has_many :helpers, through: :user_issues, source: :users
end

class User < ActiveRecord::Base
  has_many :issues, foreign_key: 'creator_id'
  has_many :user_issues
  has_many :issues_helped, through: :user_issues, source: :issues
end

class UserIssues < ActiveRecord::Base
  belongs_to :user
  belongs_to :issue
end

Scenario:

UserOne creates an issue and is saved as the IssueOne's creator. UserTwo decides to help UserOne and joins/follows IssueOne (has_many :issues_helped declared in the model) and is allowed to post a comment. UserTwo is a helper in this instance.

Well, UserTwo is now in need of some help and creates a new issue of his own. UserTwo is the creator of IssueTwo. UserOne decides to help out his new buddy UserTwo on his issue and joins/follows IssueTwo, and is allowed to post a comment. UserOne has now become a helper in this instance.

Is this the Rails way of doing it? This is the best solution I have come up with, so far.
Am I misusing the power of has_many :x, through: :y, source: :z?

The foreign key is not needed for belongs_to :creator since rails will infer this foreign key from the association name. The source has to point to the relation for UserIssue (you pluralized this name but active record class names should be singularized in most cases) so instead of source: users, and source: issues, you want source: user, source: issue.

Another way you could have accomplished the same thing, which IMO is a bit cleaner is the following:

class Issue < ActiveRecord::Base
  belongs_to :creator, class_name: 'User'
  has_many :helpings, :foreign_key => 'issue_helped_id'
  has_many :helpers, through: :helpings
end

class User < ActiveRecord::Base
  has_many :issues, foreign_key: 'creator_id'
  has_many :helpings, foreign_key: 'helper_id'
  has_many :issues_helped, through: :helpings
end

class Helping < ActiveRecord::Base
  belongs_to :helper, class_name: 'User'
  belongs_to :issue_helped, class_name: 'Issue'
end

It also gives the join table a more descriptive name. For this to work you need to modify your inflections since rails cannot pluralize issue_helped or singularize issues_helped out of the box. To do this, in config/initializers/inflections.rb add:

ActiveSupport::Inflector.inflections do |inflect|
  inflect.plural 'issue_helped', 'issues_helped'
  inflect.singular 'issues_helped', 'issue_helped'
end


Change password after being validated I'm doing Python programming challenges and have come across one where the object of the challenge was to create a script to change passwords. The script must validate that the password contains, one upper, one lowercase, and one number. After everything has been validated the script should save the password to a text file. I added special characters and some basic encryption to the program to make it a little more interesting.

Source:

# -*- encoding: UTF-8 -*-

import re
import getpass
from random import randint

class PasswordValidator(object):
    """ A password validator, used to validate a password for
        specific characters, numbers, and special characters """

    validated = False

    def __init__(self, password):
        self.password = password

    def validate_length(self):  # Check the length, gotta be over 8
        if len(self.password) > 8:
            return True
        else:
            return False

    def validate_upper_case(self):  # Check for upper case characters
        return re.search("[A-Z]", self.password) is not None

    def validate_lower_case(self):  # Check for lower case characters
        return re.search("[a-z]", self.password) is not None

    def validate_integers(self):  # Check for integers
        return re.search("[0-9]", self.password) is not None

    def validate_spec_chars(self):  # Check for special characters
        return re.search("[!$&*]", self.password) is not None

    def validate_all(self):
        """ Validate everything. """
        length = self.validate_length()
        upper = self.validate_upper_case()
        lower = self.validate_lower_case()
        digits = self.validate_integers()
        specs = self.validate_spec_chars()

        if specs is False:
            return "Failed special characters validation. Allowed special chars: (! $ & *)"
        elif digits is False:
            return "Failed integer validation. Allowed integers: (0 - 9)"
        elif lower is False:
            return "Failed lower case validation. (a - z)"
        elif upper is False:
            return "Failed uppercase validation. (A - Z)"
        elif length is False:
            return "Failed length validation. Allowed length: (> 8)"
        else:
            self.validated = True
            return "Password validated successfully."

def encrypt(password):
    """ Encrypt the password simply example: bÃFuo <- says Test!23456
    :type password: String """
    arr = list(password)
    new_arr = []

    for i in arr:
        num = ord(i) + randint(0, 100)
        new_arr.append(chr(num))

    return ''.join(new_arr)

def obtain_password():
    """ Get the password, simple and easy with getpass library """
    return getpass.getpass('Change your password: ')

def second_authentication(password_to_verify):
    """ Verify that the password is what the user wants it to be,
        by asking for it a second time.
        :type password_to_verify: String """
    password = getpass.getpass('Enter password a second time: ')
    if password == password_to_verify:
        with open('passwords.txt', 'w') as f:
            f.write(encrypt(password_to_verify))
        return "Password changed."
    else:
        return "Password not changed."

if __name__ == '__main__':
    pass_verify = obtain_password()
    val = PasswordValidator(pass_verify)
    print val.validate_all()
    if val.validated is False:
        print "Password not validated."
    else:
        print second_authentication(pass_verify)

My question is, what can I do better to make this more efficient and more accurate?

It would be more intuitive if your validate_all method was just called validate.

In quite a few places you are doing something like this in validate_length:

def validate_length(self):  # Check the length, gotta be over 8
    if len(self.password) > 8:
        return True
    else:
        return False

Just return the right value right away:

def validate_length(self):  # Check the length, gotta be over 8
    return len(self.password) > 8

It would also make more sense if the Validator.validate method just returned True or False instead of setting a class variable and returning a string detailing the status:

def validate(self, password):
    return all(validator(password) for validator in [self.validate_length, self.validate_upper_case, ...])

I would also make failing a testcase raise an exception and catch it in validate:

class ValidatorError(Exception):
    pass
...

class PasswordValidator(object):
    def __init__(self):
        self.validators = [self.validate_length, self.validate_upper_case, ...]

    ...

    def validate_length(self, password):
        """Check the length, gotta be over 8"""
        if len(password) > 8:
            return True
        raise ValidatorError("Failed length validation. Allowed length: (> 8)")

    def validate(self, password):
        try:
            return all(validator(password) for validator in self.validators)
        except ValidatorError as e:
            print e
            return False

...
if __name__ == '__main__':
    pass_verify = obtain_password()
    val = PasswordValidator()
    if not val.validate(pass_verify):
        print "Password not validated."
    else:
        print second_authentication(pass_verify)

Also your encrypt function seems to be one-way. There is no way to validate an entered password in the future (to see whether the entered password is equal to the password set by this program).


HackerRank: Counting Sort 1 I've just solved this challenge, though I feel that there is a problem with stack growth or something because the 5th testcase has a runtime error

import Control.Monad
import Data.Sequence hiding (replicateM, replicate, take, drop)
import Data.List

main :: IO ()
main = do n <- getLine
          input <- getLine
          putStrLn (unwords (map show (calc (map read (words input)) (output))))

output :: [Int]
output = replicate 100 0

calc :: [Int] -> [Int] -> [Int]
calc [] y      = y
calc (x:xs) ys = calc xs ((take (x) ys) ++ ((ys!!x) + 1):(drop (x+1) ys))

I feel that there should be a much neater solution, which doesn't cause an overflow as this is quite messy.

It would be nice to get everyone's thoughts on more efficient/nicer solutions

Let's imagine this problem as building a histogram. We draw out an X axis of 100 (0 <= x < 100, as given by the problem). For each element that we're given, we move the bar up by one in the histogram.

For something like this we have Data.Array. We can build an Array Int Frequency that will map each number to the frequency of the number. Each insertion into the array will be O(1). Initializing the array will also be O(1) since the size of the array is fixed. We'll just have the computer set aside 100 slots of memory for us up top, every time. This should lead to a running time of initialization + n * insertion = O(n), which indeed beats O(n lg n).

Our function will have this type:

count :: [Int] -> Array Int Int
count input = _

The best way to build an array is to ... uh ... I never remember. So we consult the docs, specifically the array construction section. There's a function here called accumArray that folds over a list of (index, value) tuples – that is, it walks each tuple and uses it to accumulate into a value into the array that is returned back to us. In our case, each tuple will be (number, 1): we'll have it so that each tuple represents a single count of the number. Our job is then to convert  a bunch of tuples like this

(2, 1) (5, 1) (5, 1) (3, 1)

to a histogram like this

0: 0
1: 0
2: 1
3: 1
4: 0
5: 2

Back to our code:

count :: [Int] -> Array Int Int
count input = accumArray _ _ (0, 99) (zip input (repeat 1))

The trick here is that repeat 1 is an infinite array, but we know input is finite and zip terminates when either argument terminates.

It remains to specify the accumulating function, which takes two arguments (the frequency of the number thus far and then second value of our tuple, which is always the number 1) and is supposed to return the new frequency. This is as simple as incrementing the old number, or just adding one.

count :: [Int] -> Array Int Int
count input = accumArray (\old one -> old + one) _ (0, 99) (zip input (repeat 1))

This being Haskell, we can simplify this to

count :: [Int] -> Array Int Int
count input = accumArray (+) _ (0, 99) (zip input (repeat 1))

For the greater good. And we're done! Oh, and we need to specify a default frequency for values that we didn't encounter in the list. For us, that's a simple zero.

Combining this with your IO code that reads from stdin, we get:

import Data.Array
import Data.List

main :: IO ()
main = do
  _ <- getLine
  numbers <- getLine
  (putStr . unwords . map show . elems . count . map read . words) numbers

count :: [Int] -> Array Int Int
count input =
  accumArray (+) 0 (0, 99) (zip input (repeat 1))

And if we wanted to be a smart-ass we could golf the solution into a one-liner:

import Data.Array
import Data.List

main :: IO ()
main = getLine >> getLine >>= (putStr . unwords . map show . elems . accumArray (+) 0 (0, 99) . flip zip (repeat 1) . map read . words)

I think that last line even fits into a tweet.


Codility “PermMissingElem” Solution This is my solution to the PermMissingElem problem, I wonder what can be improved? Expected worst case time complexity is O(N), but the performance test shows that it's O(N) or O(N * log(N)), which I suppose there's some solution out there that can truly achieve pure O(N)?

function solution(A) {
    const size = A.length;
    let sum = 0;

    for (i=0;i<size;i++){
        sum += A[i];
    }

    return (((size+ 1)*(size + 2))/2) - sum
}

The original problem is quoted as follows:

  A zero-indexed array A consisting of N different integers is given.
  The array contains integers in the range [1..(N + 1)], which means
  that exactly one element is missing.

  Your goal is to find that missing element.

  Write a function:

  int solution(int A[], int N); that, given a zero-indexed array A,
  returns the value of the missing element.

  For example, given array A such that:

  A[0] = 2   A1 = 3   A[2] = 1   A[3] = 5 the function should return
  4, as it is the missing element.

  Assume that:

  N is an integer within the range [0..100,000]; the elements of A are
  all distinct; each element of array A is an integer within the range
  [1..(N + 1)].

  Complexity:

  expected worst-case time complexity is O(N); expected worst-case space
  complexity is O(1), beyond input storage (not counting the storage
  required for input arguments). Elements of input arrays can be
  modified.

With your JavaScript version, there is not much to optimize. So I am just going to provide some minor improvements:

Within a function, it is better to declare your variables using the var keyword. You should apply this to the i inside for() loop
The JavaScript interpreter is a single thread: this means, unfortunately, you can not perform real parallelism to sum different chunks of the array.
You can choose better variables and function names.


CSS3 Selectors for achievements I'm trying to change the background color of each .achievement div. The content is being generated dynamically so I can't add a class easily to each one.

I have it working with selectors like .achievements .grid-row:nth-child(odd) .col-1-2:nth-child(even) .achievement but they seem a little bloated. The problem I'm having is that the .achievement div is nested a few deep so I can't just use an :nth-child or :nth-of-type on it.

I'm just checking to see if there is a better way to select those that I'm missing out on.

jsFiddle

HTML

Testing...

Testing...

Testing...

Testing...

CSS

.achievements .grid-row:nth-child(odd) .col-1-2:nth-child(even) .achievement {
    background: #5891bc;
}

.achievements .grid-row:nth-child(even) .col-1-2:nth-child(odd) .achievement {
    background: #96c3e6;
    color: #585858;
}

.achievements .grid-row:nth-child(even) .col-1-2:nth-child(even) .achievement {
    background: #d3dfe7;
    color: #585858;
}

You should probably take out the class selectors (.grid-row and .col-1-2) because nth-child doesn't respond to them (i.e. they're not doing anything):

.achievements :nth-child(even) :nth-child(odd) .achievement

In the future, CSS3's toggle() function (which cycles through values) could simplify things:

.achievement {
  background: toggle(#ccc, #5891bc, #96c3e6, #d3dfe7);
}

Sadly this isn't supported by any browsers yet. Also proposed is an :nth-match selector, which would let you do:

:nth-of-type(n4+1 of .achievement) {
  background: #5891bc;
}


SQL query to classify and count logged messages I have an SQL query that fetches some stats, and those stats are displayed on a dashboard. The dashboard runs this query every 15 seconds to fetch the most up-to-date data.

The query is very CPU-heavy for the database server (according to the stats provided by Azure SQL Server), and eats 15% of the available CPU capacity of the server.

The query runs on a log (which records messages received from user and the answers returned by a chatbot). It returns four pieces of information, namely conversations, messages, licenses and failed:

SELECT
    -- Total unique `conversationId`s:
    COUNT (DISTINCT conversationId) AS conversations,

    -- Total records in table:
    COUNT (*) AS messages,

    -- Total unique `licenseId`s:
    COUNT (DISTINCT licenseId) AS licenses,

    -- Total messages where `intent` meets certain criteria:
    (
        SELECT COUNT (*)
        FROM MessageLog
        WHERE intent LIKE 'None' OR intent IS NULL OR intent LIKE ''
      ) AS failed
    FROM MessageLog

What do you think is the most CPU intensive part of that query, and what are the possible ways to consume less CPU?

Update: Here's the DB schema:

CREATE TABLE [dbo].[MessageLog](
    [id] [int] IDENTITY(1,1) NOT NULL,
    [licenseId] [int] NOT NULL,
    [message] [nvarchar](1000) NOT NULL,
    [timestamp] [datetime] NOT NULL,
    [intent] [nvarchar](70) NULL,
    [entities] [nvarchar](3000) NULL,
    [conversationId] [nvarchar](100) NULL,
    [confidence] [float] NULL,
    [failed] [bit] NOT NULL,
    [ipAddress] [varchar](25) NULL,
    [userAgent] [varchar](256) NULL,
    [nlpExecutionTime] [int] NULL,
    [answer] [nvarchar](1000) NULL
) ON [PRIMARY]
GO

ALTER TABLE [dbo].[MessageLog] ADD  CONSTRAINT [DF_MessageLog_failed]  DEFAULT ((0)) FOR [failed]
GO

There are non-clustered indexes on id, intent, conversationId, licenseId, but it doesn't seem to improve the performance.

Guessing:

  What do you think is the most CPU intensive part of that query, and
  what are the possible ways to consume less CPU?

We don't have to guess, you test it using SSMS and execution plans.

Capacity:

  The query is very CPU-heavy for the database server (according to the
  stats provided by Azure SQL Server), and eats 15% of the available CPU
  capacity of the server.

15% of 0 is still 0, how many CPUs do you have for this instance, if you only have 1 CPU 15% could be considered significant, however the issue is most likely not this query, it is the ration of CPU you've given it.

Fundamentals:

When you have a table and you are TRYING to use a surrogate key as a primary key, you still have to have a primary key on the object for it to be considered a table. You are currently querying a heap.

Please start by adding a primary key to the table, identity(1,1) is not a primary key, it is a candidate for a primary key, you still have to tell SQL you want it to be the primary key.

Predicate:

How large is this table, it could be the case that you have 4 billion records in the table and you're asking this question with out a filter on date range. Do you intent for this to be a question for all time?  Is there a process that truncates this table, or starts the process over in some fashion?

Indexes:

Indexes, I would create a non cluster index on intent, you could even try a filtered index on the column.

Performance tuning

You will want to compare the execution plans, find the equivalent of dropcleanbuffers and freeproccache for Azure. Subtreecost is what you're looking to improve, time it takes to return is a non-sequitur. You might need to provide table hints to force it to use the indexes to validate if the index is useful or not.

Tempdb:

What does your tempdb look like, I only ask because of the multiple DISTINCT you use in the queries. If there is a lot of data in the table its dumping to tempdb and disk, what does the I/O look like for tempdb, what does the space look like?

Code Changes

I would also consider changing the code @Malachi offers sound suggestion. I would also consider SQL Server Window Functions.


Toggling between light and dark mode using JavaScript This is some code written in a course I'm busy taking on Udemy. It serves as a basic toggle switch to flip between light and dark mode. This includes the nav bar, background, images and the icon that changes from a sun to a moon.
The objective after the lesson was to clean up the code and make it DRY. I've done so to some of the code already. But there are some instances where it still repeats, for instance isDark in my lightDarkMode function. Is there a way to eliminate the repetitive usage of isDark?
const toggleSwitch = document.querySelector('input[type="checkbox"]');
const nav = document.getElementById('nav');
const toggleIcon = document.getElementById('toggle-icon');
const textBox = document.getElementById('text-box');
const darkLightTheme = ['dark', 'light'];

function imageMode(color) {
    image1.src = `img/undraw_proud_coder_${color}.svg`;
    image2.src = `img/undraw_feeling_proud_${color}.svg`;
    image3.src = `img/undraw_conceptual_idea_${color}.svg`;
}

    lightDarkMode = (isDark) => {
    nav.style.backgroundColor = isDark ? 'rgb(0 0 0 / 50%)' : 'rgb(255 255 255 / 50%)';
    textBox.style.backgroundColor = isDark ? 'rgb(255 255 255 / 50%)' : 'rgb(0 0 0 / 50%)';
    toggleIcon.children[0].textContent = isDark ? 'Dark Mode' : 'Light Mode';
    isDark ? toggleIcon.children[1].classList.replace('fa-sun', 'fa-moon') : toggleIcon.children[1].classList.replace('fa-moon', 'fa-sun');
    isDark ? imageMode(darkLightTheme[0]) : imageMode(darkLightTheme[1]);
}

One thing that you could update is how you are using the ternary operator for isDark.
Ternary operators are used for returning something based on a boolean response, so either return X if true, or Y if false.
However, every time it is used it is evaluating the statement being passed to it. In this case both times it is evaluating isDark. For the average website, this won't affect your processing time, but it is something to think about.
Instead a classic if/else statement would allow you to run more code off a single evaluation.
You will notice this isn't as sleek looking as ternary and is even more lines of code, but gives more capabilities when you need it.
if (isDark) {
  toggleIcon.children[1].classList.replace('fa-sun', 'fa-moon')
  imageMode(darkLightTheme[0])
} else {
  toggleIcon.children[1].classList.replace('fa-moon', 'fa-sun')
  imageMode(darkLightTheme[1])
}


Immediate Smaller Element Time Limit Exceeded Immediate Smaller Element The code is working fine Code is here. For each element in the array, check whether the right adjacent element (on the next immediate position) of the array is smaller. If the next element is smaller, print that element. If not, then print -1. The only problem is I'm getting "Time Limit Exceeded" when I submit my answer here.
It shows

  Your program took more time than expected.Time Limit Exceeded
  Expected Time Limit < 3.496sec
  Hint: Please optimize your code and
  submit again.

How do I optimize my code to pass the time limit exceeded problem?

  Problem Statement: Given an integer array of size N. For each element
  in the array, check whether the right adjacent element (on the next
  immediate position) of the array is smaller. If next element is
  smaller, print that element. If not, then print -1.

  Input: The first line of input contains an integer T denoting the
  number of test cases. T testcases follow. Each testcase contains 2
  lines of input: The first line contains an integer N, where N is the
  size of array. The second line contains N integers(elements of the
  array) sperated with spaces.

  Output: For each test case, print the next immediate smaller elements
  for each element in the array.

  Constraints:
  1 ≤ T ≤ 200
  1 ≤ N ≤ 10E7
  1 ≤ arr[i] ≤ 1000
  Expected Time Limit < 3.496sec

  Example:

  Input:
  2
  5
  4 2 1 5 3
  6
  5 6 2 3 1 7

  Output:
  2 1 -1 3 -1
  -1 2 -1 1 -1 -1

  Explanation:
  Testcase 1: Array elements are 4, 2, 1, 5, 3. Next to 4
  is 2 which is smaller, so we print 2. Next of 2 is 1 which is smaller,
  so we print 1. Next of 1 is 5 which is greater, so we print -1. Next
  of 5 is 3 which is smaller so we print 3.  Note that for last element,
  output is always going to be -1 because there is no element on right.

Here is my code

class Program
{
    static void Main(string[] args)
    {
        int testCases = int.Parse(Console.ReadLine().Trim());

        while (testCases-- > 0)
        {
            int arrSize = int.Parse(Console.ReadLine().Trim());
            string[] arr = Console.ReadLine().Trim().Split(' ');

            for (int i = 0; i < arrSize - 1; i++)
            {
                if (int.Parse(arr[i]) > int.Parse(arr[i + 1]))
                {
                    Console.Write(arr[i + 1] + " ");
                }
                else
                    Console.Write("-1" + " ");
            }
            Console.Write("-1");
            Console.WriteLine();
        }
    }
}

Things to improve

You are looping over the input twice; (1) when splitting the raw input string Console.ReadLine().Trim().Split(' ') (2) when going over the splitted items for (int i = 0; i < arrSize - 1; i++). Try finding a way to go over the raw input in a single pass.
You are parsing most items twice, once as arr[i+1] and once as arr[i] in the next cycle. Try avoiding redundant parsing.
Writing to the console is time expensive. Console.Write("-1" + " "); Try to find a way to build the string and write the result to the console once.

I just tested a solution taking into account the above and I got Execution Time: 1.21. So it's definately possible to go < 3.496


Function to find a substring within a string I've just completed an exercise in which I had to create a program that searches for a string within another bigger string.

If the substring exists then it is outputted to confirm and the starting index is given where the first character can be found.

Aside from printf and scanf, I am not allowed to use any other functions unless I write them myself.

My code works, however I'm concerned that I've written far more code than is necessary. WITHOUT using pointers (something I'll learn next), could I have considerably cut down the amount of code I wrote?

#include <stdio.h>
#include <stdbool.h>

int findString(char mom[], char babe[]);
int stringLength(const char string[]);
bool equalStrings(const char s1[], const char s2[]);

int main(void)
{
    char mommy[80];
    char baby[80];
    int index;

    printf("Which string do you want to search?\n");
    scanf("%s", mommy);

    printf("What do you want to search for?\n");
    scanf("%s", baby);

    index = findString(mommy, baby);

    if(index >= 0)
        printf("Yup, baby was found in mommy at index %i\n", index);
    else
        printf("Sorry, baby not found\n");

    return 0;
}

int findString(char mom[], char babe[])
{
    int index, i, j;
    int momLength = stringLength(mom);
    int babeLength = stringLength(babe);
    char test[babeLength + 1];

    for(i = 0; i <= babeLength-1; i++)
    {
        for(j = 0; j <= momLength; j++)
        {
            if(babe[i] == mom[j])
            {
                test[i] = babe[i];

                if(babe[0] == mom[j])
                    index = j;
            }
        }
    }

    test[babeLength] = '\0';

    if(equalStrings(test, babe) == true)
        return index;
    else
        return -1;
}

int stringLength(const char string[])
{
    int count = 0;

    while(string[count] != '\0')
        count++;

    return count;
}

bool equalStrings(const char s1[], const char s2[])
{
    int i = 0;
    bool areEqual;

    while(s1[i] == s2[i] && s1[i] != '\0'){
        i++;
        if(s1[i] == '\0' && s2[i] == '\0')
        {
            areEqual = true;
        }
        else
        {
            areEqual = false;
        }
    }
    return areEqual;
}

Sorry but your code is an accident waiting to happen.

Using a constant to declare the available space in you char arrays allows your code to be more maintainable.
scanf() can overflow your buffer and corrupt everything after your buffer.  Forcibly set the BufferSize-1 character to NULL after you have run scanf() its the best you can do with the limitations you have be given.
Why are you declaring index so far up the code, always declare a variable as near to the point of first use as you can, also it could be const, which will make it clear to the reader that its value will never change.
Unless you compiler won't allow it declare loop variables inside the for statement i.e. for (int i = 0; i < momLength; ++i).  The ++i makes no difference for simple types, but when you move on to iterators it does.
Always try and use meaningful variable names, i and j are not very meaningful.  You might read this again in 12 months, so make it easy for yourself.
Now the bad news, I think you algorithm is wrong.  Why are you creating a copy of a string (test)?  I think it would be easier to scan through mum until you find the first character of babe (index).  Then match every character of babe to the characters of mom.  If you get to the end of babe then you have a match starting at index, if a character doesn't match then start scanning mum again at index + 1.
You need to check for zero length mom and babe values entered by the user.
stringLength() could go on "infinitely"  As it stands if you scan a value which overflows the buffer then you could keep going until you run out of memory.  You need to impose a maximum length of BufferSize.
And finally, stick some comments in, it will make it easier when you are getting marked and you'll get a better score.


Adding some properties to an Active Directory user I was writing the below section of code which adds some user properties to an Active Directory user, and got wondering - surely we can improve the readability of this :

var firstname = newUser.FirstName;
var lastname = newUser.LastName;
var email = firstname + lastname + "@mydomain.com";
var dotname = $"{firstname}.{lastname}";
var fullname = $"{ firstname} { lastname}";

// Create the new user
var user = myLdapConnection.Children.Add($"CN={firstname} {lastname}", "user");
// Add properties
user.Properties["userprincipalname"].Add(email);
user.Properties["samaccountname"].Add(dotname);
user.Properties["sn"].Add(lastname);
user.Properties["givenname"].Add(firstname);
user.Properties["displayname"].Add(fullname);
user.Properties["description"].Add(newUser.JobTitle);
user.Properties["mail"].Add(email);
user.Properties["homedirectory"].Add($@"\\File\Home$\{dotname}");
user.Properties["homedrive"].Add("H:");
user.CommitChanges();

So I added some tabs after my property-name calls like so :

// Add properties
user.Properties["userprincipalname"].Add(email);
user.Properties["samaccountname"]   .Add(dotname);
user.Properties["sn"]               .Add(lastname);
user.Properties["givenname"]        .Add(firstname);
user.Properties["displayname"]      .Add(fullname);
user.Properties["description"]      .Add(newUser.JobTitle);
user.Properties["mail"]             .Add(email);
user.Properties["homedirectory"]    .Add($@"\\File\Home$\{dotname}");
user.Properties["homedrive"]        .Add("H:");
user.CommitChanges();

Now I think that makes the code a lot more readable, but I have not seen many people take this approach when formatting. Is there any reason for this? I generally do this in both PowerShell and C#, but are there any languages where this is not acceptable for reasons other than the code wont run?

Disclaimer: I never wrote code like that, I do not see a reason to do it and I discourage everyone to do it.
This however may be seen as nothing else aside of a preference.

But what are you trying to achieve?

Prefectionism?

I do not associate myself with perfectionism, and even if I did as I stated, I doubt I would include
this on the things I must do to keep the code 'perfect'.
Being perfectionist does not mean that you are egocentric. If you are not a egocentric perfectionist
you may relize that the set of rules that you would like to apply doesn't need to be the same set of rules
of another perfectionist.

Maintanability?

How is that code more maintanable than the other without spaces/tabs?
It is less maintanable because I have to spend more time adjusting the identation when adding a new property

Readability?

If we are aiming readability maybe we have another options, for example:

user.Properties["userprincipalname"].Add(email);
user.Properties["givenname"        ].Add(firstname);

This is a formatting issue, and all formating issues should/must be automatically
verified by a static analyzer like resharper, or rubberduck-vba, or wathever.
Most of them allow you to configure all settings once and share among all developers so the code
is written always with the same formatting.

TLDR

In the end you must confess that what you really want to write is the following:

user.Email = email;
user.FirstName = newUser.FirstName;

And you can. I'll list the steps needed to do it:

Create a class with a constructor that recieves the ldap user
Foreach property setter write to the respective ldap property Name
Foreach property getter get the value from the respective ldap property Name


Find common directory path I answered the problem of Find common directory path from Rosetta Code.

The problem consists of passing  '/home/user1/tmp/coverage/test',
 '/home/user1/tmp/covert/operator', and '/home/user1/tmp/coven/members' to a function, the resultant path should be the valid directory '/home/user1/tmp' and not the longest common string '/home/user1/tmp/cove'.

The solution (the working code) is:

import Foundation

func getPrefix(_ text:[String]) -> String? {
    var common:String = text[0]
    for i in text {
        common = i.commonPrefix(with: common)
    }
    return common
}

var test = ["/home/user1/tmp/coverage/test",
 "/home/user1/tmp/covert/operator",
 "/home/user1/tmp/coven/members"]

var output:String = getPrefix(test)!
print(output)

Could the above code be further optimized?

Naming

Good names can greatly improve the readability of a program.
Many of the names in the posted code can be improved:

getPrefix: the function returns the common prefix. It would be good to include that important clarifying detail in the name.

If you intend to return the common parent directory, I would call the function getCommonParent.

text: the parameter is not just any text, it's an array of paths, so paths would be a better name that conveys the intention.
i: is fine in simple counting loops, but when you iterate over values that have a meaningful name, it's better to use that, for example in this example path.
test is also an overly generic name, instead of the more descriptive paths

Algorithm

The implementation will call commonPrefix method text.count + 1 times.
This is probably fine,
but you could reduce the number of calls to commonPrefix
by using a divide and conquer strategy.

For example, consider you have paths p1, p2, ..., pn.
You could call commonPrefix for each pair, to get n / 2 results.
Then you could call commonPrefix for each of those pairs.
And so on, until there is only one pair left:

p1 p2 p3 p4 p5 p6 p7 p8
|__|  |__|  |__|  |__|
 |_____|     |_____|
    |___________|
          |
          = the common prefix

Admittedly, this might be overkill for your use case.


Disk enumeration and file listing I am writing C++ code to enumerate the whole HDD and drive listing.  However, it takes more than 15 minutes to complete the disk enumeration of all drives (HDD capacity of 500GB) and compile the response in a binary file.

However, I have a 3rd party executable which gives me the listing of the whole disk in just less than two minutes.  Can you please look into my code and suggest some performance improvement techniques?

EnumFiles(CString FolderPath, CString SearchParameter,WIN32_FIND_DATAW *FileInfoData)
{
         CString SearchFile = FolderPath + SearchParameter;
         CString FileName;

        hFile = FindFirstFileW(SearchFile, FileInfoData); //   \\?\C:\*

            if (hFile == INVALID_HANDLE_VALUE)
            {
                // Error
            }
          else
         {
                do
                {
                    FileName = FileInfoData->cFileName;
                    if (FileInfoData->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
                    {
                        if (! (FileName == L"." || FileName == L".."))
                        {
                                        // Save the Folder Information
                          EnumFiles(FolderPath + FileName +(L"\\"), SearchParameter,FileInfoData);
                        }
                    }
                    else
                   {
                     // Save the File Parameters
                    }

              } while (FindNextFileW(hFile, FileInfoData));
}
           FindClose(hFile);
       }

Can you please look into my code and suggest me some performance improvement techniques.

No -- it doesn't work like that. While there are some obvious things you may catch visually (like observing string comparisons at each iteration) every time you optimize you should start by profiling your execution, then isolating the parts that take the most time, then optimizing, then profiling again.

For example, it's possible that optimizing in this case is not removing or changing something you "do wrong" but splitting your hard drive into chunks and parallelizing the effort.

Either way, first, profile the execution and identify the worst offenders.


Is this good approach to use debug functions for implementing features? Just thinking if using code like this

<?php

abstract class helper
{
    private static $_cycles = array();

    public static function isOdd($v)
    {
        return (0 == ($v % 2)) ? false: true;
    }

    public static function isEven($v)
    {
        return !self::isOdd($v);
    }

    public static function cycle($odd, $even)
    {
        $trace = debug_backtrace();
        $trace = $trace[0];
        $cycle = crc32(serialize($trace));

        if (!isset(self::$_cycles[$cycle]))
        {
            self::$_cycles[$cycle] = 1;
        }

        return (self::isOdd(self::$_cycles[$cycle]++)) ? $odd : $even;
    }
}

for featues like this

<?php foreach ($data as $record): ?>
    <p class="<?php echo helper::cycle('oddCss', 'evenCss'); ?>"><?php echo $record; ?>

<?php endforeach; ?>

is not overcoded

PS
Easy of usage (by html/css coders) is more important then performance in this particular case

UPDATE:
As @Geoffrey mentioned i didn't tell before that same 'oddCss', 'evenCss' pair may be used several times on one page (so basically within one request)

Since this is a code review site, let me point out that the following is an abomination:

return (0 == ($v % 2)) ? false: true;

The constants are redundant. Much simpler:

return (0 != ($v % 2));

(The parentheses are also redundant but that’s unrelated.)


JavaScript Website-Content Grabber I my firm a few people have following problem:

A Content Management System is hosted externally. The treaty doesn't include database-access.

In September the treaty will expire. So they have to get the content copied.

I have tried to figure out something. So this programming is a first-try. A prototype in the best-case.

It works only with the HTML source-code of the articles from these news-site: heise.de

I could have used another site as well. It was just for having something to experiment with.

Here's the code I have written so far:

document.getElementById('exportText').addEventListener('click', function() {
  'use strict';
  var exportText = '';
  var re = /(data\-article\-type="meldung">)(.*)(<\/article)/m;
  var reString  = '(
)(.*?)(<\/h2>)|';
      reString += '(
)(.*?)(<\/p>)|';
      reString += '(
)(.*?)(<\/h3>)|';
      reString += '(
)(.*?)(<\/p>)';
  var matches  = [];
  var mainText = document.getElementById('source')
                         .value                            // Get the content from the first text-box.
                         .replace(/\r?\n|\r|\s{2,}/g, '')  // Remove line breaks; Multiple blanks to ONE blank.
                         .match(re)[0];                    // Restrict the area which is searched.

  re = new RegExp(reString, 'gm');

  // Search in the "meldung-wrapper"-container for h2-, h3-headlines and paragraphs.
  while ((matches = re.exec(mainText)) !== null) {
    exportText += ( matches[2] || matches[5] ||
                    matches[8] || matches[11] || '' ) + '\n\n';
  }

  document.getElementById('target').value = exportText;
});

document.getElementById('reset').addEventListener('click', function() {
  document.getElementById('source').value = '';
  document.getElementById('target').value = '';
});
* {
  background-color: #cdcdcd;
}

.wrap {
  width: 800px;
  margin: 10px auto;
}

label {
  font-weight: 900;
  margin: 10px 0;
  display: block;
}

textarea {
  background-color: #efefef;
  border-radius: 4px;
}

label,
button {
  font-family: verdana;
  sans-serif;
}

.control {
  display: inline-block;
  text-decoration: none;
  color: #fff;
  padding: 6px 8px;
  border: 1px solid;
  text-align: center;
  text-transform: uppercase;
  margin: 12px 5px;
  border-radius: 4px;
  background-color: #9a9a9a;
  font-size: .9rem;
}

.control:hover {
  background-color: #232323;
  color: #efefef;
}

.control:active {
  opacity: 0.6;
}

#target {
  padding: 4px 5px;
}
<!doctype html>

  <title>Exporter Script</title>
  <link rel="stylesheet" href="css/style.css" />

      HTML Source-Code
      <textarea id="source" rows="10" cols="100"></textarea>

      Exportierte Inhalte
      <textarea id="target" rows="20" cols="100"></textarea>

      Export starten
      Reset Textboxes

You've got to paste the source-code of an article into the textbox "HTML Source Code". Then click the button. Then the second textbox contains the textual content of the article including inline-tags (hyperlinks, bold, italic etc. are desired to be exported too).

What do you think about the approach?

- Is this a good approach? Or is there a better way to accomplish this task?

Please keep in mind that I don't have database-access.

- Are there points which can or must be improved in my programming?

- Any ideas how to accomplish a better structure?

Looking forward to reading your hints, comments and recommendations.

Is this a good approach?

Sadly, nope.

  Or is there a better way to accomplish this task?

Yes.

Instead of manually sifting through pages and copying source code, use wget's site mirroring functionality to download the entire site. There are a lot of ways to do this with the various options wget has. HTTrack is also an option if you want the tool to mangle the structure, names, etc. But we only need to download the pages so wget is fine.

Next, instead of having a page that parses the page source for content, consider writing a Node.js script instead*. Use glob to grab the paths of all HTML files, cheerio to traverse and grab content from the HTML like jQuery, and Node's very own fs to read and write to files. Also, regular expressions are the worst tools to parse through HTML. That's a known fact, don't even go there.

In summary, you run wget to download the entire site, and the Node.js script to parse all the HTML that was downloaded. You'll probably spend half a day to a day tops to build this script and the next day you'll have parsed content in seconds.  You just saved 2 months worth of work right there.

With the answer above, the rest becomes irrelevant.

*Any language would actually do (Python, Ruby, PHP, etc). But since you wrote in JS, Node.js would probably make more sense.


Proper techniques for allowing many views to subscribe to window.unload? I would like to execute code for many views whenever the window is unloaded.

For instance, I could have something like this in a view's initialize:

initialize: function() {
    $(window).unload(this.stopListening.bind(this));
}

I'm wondering if it would be better to have a manager which aggregates all of the view's requests into one event like:

define(function () {
    'use strict';

    var ForegroundViewManager = Backbone.Model.extend({

        defaults: function () {
            return {
                views: []
            };
        },

        initialize: function() {
            $(window).unload(function () {
                this.allViewsStopListening();
            }.bind(this));
        },

        allViewsStopListening: function() {
            _.each(this.get('views'), function(view) {
                view.stopListening();
            });
        },

        subscribe: function (view) {
            var views = this.get('views');
            var index = views.indexOf(view);

            if (index === -1) {
                views.push(view);
                console.log("Subscribed a view:", view, views.length);
            } else {
                console.log("Already subscribed!");
            }
        },

        unsubscribe: function (view) {
            var views = this.get('views');
            var index = views.indexOf(view);

            if (index > -1) {
                views.splice(index, 1);
            }
        }

    });

    return new ForegroundViewManager();
});

The second way is a lot more code, but results in just one unload event being bound to the window and then iterating over a collection. Is this preferred? Or is it better to have each view bind itself to window.unload?

An opinion question, always tricky;

I think we can all agree that the second approach is not KISS and requires more bandwidth to download to the browser. As far as I can tell, this will not save any memory either..

I would strongly suggest to use the first approach.

Furthermore:

unsubscribe could be reduced to:

unsubscribe: function (view) {
  this.get('views') = _without( this.get('views') , view );
}

Production code should not use console.log
subscribe could use _.union()
subscribe could be reduced to:

subscribe: function (view) {
  this.get('views') = _union( this.get('views') , view );
}


Find the election winner Challenge

There are n citizens voting in this year's HackLand election. Each voter writes the name of their chosen candidate on a ballot and places it in a ballot box.
The candidate with the highest number of votes wins the election;
if two or more candidates have the same number of votes, then the tied candidates' names are ordered alphabetically and the last name wins.

Complete the electionWinner function in your editor. It has 1 parameter: an array of strings, votes, describing the votes in the ballot box. This function must review these votes and return a string representing the name of the winning candidate.

My Code

function electionWinner(votes) {
    const vObj = {};
    for(let v of votes){
        vObj[v] = (vObj[v] || 0) + 1;
    }

    let winners = [];
    let maxVotes = 0;

    for(let name in vObj){
        if(vObj[name] > maxVotes){
            maxVotes = vObj[name];
            winners = [name];
        }
        else if (vObj[name] === maxVotes){
            winners.push(name);
        }
    }

    if(winners.length === 1){
        return winners[0];
    }
    winners.sort();
    return winners[winners.length - 1];
}

You can write this simpler:

if(winners.length === 1){
    return winners[0];
}
winners.sort();
return winners[winners.length - 1];

Like this:

winners.sort();
return winners[winners.length - 1];

Because if winner contains one element, winners.sort() will do nothing anyway.

vObj is a poor name for a map of vote counts per name.

It could be a good idea to decompose the implementation to smaller functions with a single responsibility:

Build a map of vote counts
Find winners
Find the single winner


Using a BaseController in MVC asp.net to reuse the repository I have a repository class that impliments a repository Interface.
In my Controllers I want a way to access the repository without creating constructors each time in the controller. So now all my Controllers have to do is inherit from Base. Any pitfalls to this, or how I can write it differently.

public class BaseController : Controller
    {
        public IRepository Repo {get;set;}
        public IStudentRepositroy StudentRepo {get;set;};
      public BaseController() : this(new Repository(),new StudentRepository())
      {

      }
      public BaseController(IRepository repo,IStudentRepository studentRepo)
      {
          Repo = repo;
          StudentRepo = studentRepo;
      }

    }

This sounds exactly like a desire for Dependency Injection (using an IoC container).  ASP.NET MVC is perfectly setup for this already with plenty of containers out there for you to use!

You can still have a base constructor take arguments if you wish but I would try and limit how many.

So your controllers would look like:

public StudentController(IRepository repo,IStudentRepository studentRepo)
        : base(repo)
{
      StudentRepo = studentRepo;
}

So by using the IoC framework these will be automatically resolved to your desired class.  By doing this, it opens up the opportunity to more easily inject mock classes into your controllers and further separates them conceptually from implementation detail of the repositories.  Although I typically do not Unit test my controllers (as I try to keep them as skinny as possible), it would also make unit testing easier.

Typically you might do this in the Global.ascx or in the App_Start folder.  You can get most DI frameworks by using Nuget packages (which I would recommend).  There are many out there but here are a couple of examples from ones I have dealt with in the past.

Autofaq

public class AutofaqConfig
{
    public static void Configure()
    {
        var builder = new ContainerBuilder();
        builder.RegisterControllers(typeof(MvcApplication).Assembly).PropertiesAutowired();

        RegisterRepositories(builder);

        var container = builder.Build();
        DependencyResolver.SetResolver(new AutofacDependencyResolver(container));
    }

    private static void RegisterRepositories(ContainerBuilder builder)
    {
        builder.RegisterType<StudentRepository>().As<IStudentRepository>();
        // And any other repositories here
    }
}

And in your global ascx

public class MvcApplication : System.Web.HttpApplication
{
    protected void Application_Start()
    {
        AreaRegistration.RegisterAllAreas();
        FilterConfig.RegisterGlobalFilters(GlobalFilters.Filters);
        RouteConfig.RegisterRoutes(RouteTable.Routes);
        BundleConfig.RegisterBundles(BundleTable.Bundles);

        AutofaqConfig.Configure();
    }
}

Ninject (Using the MVC3 ninject module)

public static class NinjectWebCommon
{
    private static readonly Bootstrapper bootstrapper = new Bootstrapper();

    /// <summary>
    /// Starts the application
    /// </summary>
    public static void Start()
    {
        DynamicModuleUtility.RegisterModule(typeof(OnePerRequestHttpModule));
        DynamicModuleUtility.RegisterModule(typeof(NinjectHttpModule));
        bootstrapper.Initialize(CreateKernel);
    }

    /// <summary>
    /// Stops the application.
    /// </summary>
    public static void Stop()
    {
        bootstrapper.ShutDown();
    }

    /// <summary>
    /// Creates the kernel that will manage your application.
    /// </summary>
    /// <returns>The created kernel.</returns>
    private static IKernel CreateKernel()
    {
        var kernel = new StandardKernel();
        kernel.Bind<Func<IKernel>>().ToMethod(ctx => () => new Bootstrapper().Kernel);
        kernel.Bind<IHttpModule>().To<HttpApplicationInitializationHttpModule>();

        RegisterServices(kernel);

        // Install our Ninject-based IDependencyResolver into the Web API config
        GlobalConfiguration.Configuration.DependencyResolver = new NinjectDependencyResolver(kernel);

        return kernel;
    }

    /// <summary>
    /// Load your modules or register your services here!
    /// </summary>
    /// <param name="kernel">The kernel.</param>
    private static void RegisterServices(IKernel kernel)
    {
        kernel.Bind<StudentRepository>().To<IStudentRepository>();
    }
}

Using Unity (Using Unit.MVC4)

public class UnityConfig
{
    #region Unity Container
    private static Lazy<IUnityContainer> container = new Lazy<IUnityContainer>(() =>
    {
        var container = new UnityContainer();
        RegisterTypes(container);
        return container;
    });

    /// <summary>
    /// Gets the configured Unity container.
    /// </summary>
    public static IUnityContainer GetConfiguredContainer()
    {
        return container.Value;
    }
    #endregion

    /// <summary>Registers the type mappings with the Unity container.</summary>
    /// <param name="container">The unity container to configure.</param>
    /// <remarks>There is no need to register concrete types such as controllers or API controllers (unless you want to
    /// change the defaults), as Unity allows resolving a concrete type even if it was not previously registered.</remarks>
    public static void RegisterTypes(IUnityContainer container)
    {
        // NOTE: To load from web.config uncomment the line below. Make sure to add a Microsoft.Practices.Unity.Configuration to the using statements.
        // container.LoadConfiguration();

        // Repositories
        container.RegisterType<IStudentRepository, StudentRepository>();
    }
}


Object oriented architecture with an image processing code This code is python 3X code (in django).
It takes an instance of user entry, and process images: it creates a resized version of original image, 2 thumbs, saves all, and update instance fields to point to correct files so it's correctly saved.
Then it returns the updated instance.

I'm not sure this code object oriented architecture is correct. It seems lots of line of redundant code. Could I have made it smarter?
I tried to break code on several methods that doe only one thing.
Then I process it with the method call 'execute' which is used elsewhere in the script when this code is used.

Also what do you think of encapsulation of this code?

class ImageProcessing:
    def __init__(self, instance, image):
        self.instance = instance
        self.image = image

        # Settings
        self.sizes                    = OrderedDict()
        self.sizes['image_main']      = (800, 800)
        self.sizes['thumbsize_big']   = (200, 200)
        self.sizes['thumbsize_small'] = (100, 100)
        self.png_compress = 6
        self.jpg_compress = 80
        self.has_iterated = 0
        # Variables
        self.slug = slugify(instance.myField, allow_unicode=True)
        self.saving_path = instance.savePath
        self.base_dir = settings.BASE_DIR + self.saving_path
        self.image_format = self.image.image.format.lower()
        has_iterated = 0

    def resize_image(self, image, size):
        myimage = Image.open(image)
        myimage = myimage.resize(size, Image.ANTIALIAS)
        return myimage

    def save_image(self, image, save_dir, png_compress, jpg_compress):
        if self.image_format == 'png':
            image.save(save_dir + '.png', compress_level=png_compress, format='PNG')
        if self.image_format == 'jpg' or 'jpeg':
            image.save(save_dir + '.jpg', quality=jpg_compress, format='JPEG')

    def execute(self):
        for key, value in self.sizes.items():
            save_dir = self.base_dir + self.slug + '_' + key

            if self.image_format == 'png':
                # Save images on disk
                self.save_image(self.resize_image(self.image, value),save_dir, self.png_compress, self.jpg_compress)
                # Save images in fields
                if self.has_iterated == 0: self.instance.image_main = self.saving_path + self.slug + '_' + key + '.png'
                if self.has_iterated == 1: self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.png'
                if self.has_iterated == 2: self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.png'
                self.has_iterated += 1
            if self.image_format == 'jpg' or 'jpeg':
                self.save_image(self.resize_image(self.image, value),save_dir, self.png_compress, self.jpg_compress)
                if self.has_iterated == 0: self.instance.image_main = self.saving_path + self.slug + '_' + key + '.jpg'
                if self.has_iterated == 1: self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.jpg'
                if self.has_iterated == 2: self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.jpg'
                self.has_iterated += 1

        return self.instance

Reducing repetition

self.sizes

self.sizes being repeated 4 times is not necessary:

    self.sizes                    = OrderedDict()
    self.sizes['image_main']      = (800, 800)
    self.sizes['thumbsize_big']   = (200, 200)
    self.sizes['thumbsize_small'] = (100, 100)

You may give an initial argument to OrderedDict to avoid it:

    self.sizes = OrderedDict( (
        ('image_main', (800, 800)),
        ('thumbsize_big', (200, 200)),
        ('thumbsize_small', (100, 100))
    ))

myimage

def resize_image(self, image, size):
    myimage = Image.open(image)
    myimage = myimage.resize(size, Image.ANTIALIAS)
    return myimage

In fact the myimage variable may be avoided completely, just chaining the method calls is much simpler:

def resize_image(self, image, size):
    return Image.open(image).resize(size, Image.ANTIALIAS)

Whole block repetition

        if self.image_format == 'png':
            # Save images on disk
            self.save_image(self.resize_image(self.image, value),save_dir, self.png_compress, self.jpg_compress)
            # Save images in fields
            if self.has_iterated == 0: self.instance.image_main = self.saving_path + self.slug + '_' + key + '.png'
            if self.has_iterated == 1: self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.png'
            if self.has_iterated == 2: self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.png'
            self.has_iterated += 1
        if self.image_format == 'jpg' or 'jpeg':
            self.save_image(self.resize_image(self.image, value),save_dir, self.png_compress, self.jpg_compress)
            if self.has_iterated == 0: self.instance.image_main = self.saving_path + self.slug + '_' + key + '.jpg'
            if self.has_iterated == 1: self.instance.image_thumbsize_big = self.saving_path + self.slug + '_' + key + '.jpg'
            if self.has_iterated == 2: self.instance.image_thumbsize_small = self.saving_path + self.slug + '_' + key + '.jpg'
            self.has_iterated += 1

The blocks are very similar just the file-extension changes.

extension = 'png' if self.image_format == 'png' else 'jpg'

And then you can delete the conditional branches that follow and leave only one path.

Bug on the use of or

    if self.image_format == 'jpg' or 'jpeg':

Does not work as you intended. or returns the first truthy value, so you wrote the same as:

    if self.image_format == 'jpg':


Vanilla Markov chain in 18 lines I am looking for advice on how I can improve this code. The objective is to make a vanilla Markov Chain text generator in the most concise way. I'm looking for advice on style, naming, 'elegance', and correctness.

import random, re

with open("corpus.txt") as file:
    text = file.read().replace('\n', ' newline ')

text = re.sub(r'([\,\.\!\?\;\:])',r' \1 ',text)
text = re.sub(r'[^a-zA-Z0-9|\,|\.|\!|\?|\;|\:]',r' ',text)

text = [word.lower().strip() for word in text.split(' ') if word.strip() != '']

markov_chain = {}
for current_word, next_word in zip(text, text[1:]):
    markov_chain.setdefault(current_word, []).append(next_word)

current_word = random.choice(list(markov_chain.keys()))
sentence = []

for i in range(0,15):
    sentence.append(current_word)
    if current_word in markov_chain and len(markov_chain[current_word]) > 0:
        current_word = random.choice(markov_chain[current_word])
    else:
        break

print(' '.join(sentence).replace('newline', '\n').replace('\n ', '\n'))

At a high level, I think you're overusing regexes. You can split stuff into words earlier and then clean them, and you don't need to treat newlines as an explicit token. If you do want to split them as an explicit token called newline to protect it from future regexes, you can handle that when you're constructing the markov chain by having a dedicated class, or just a string \n.

class NewlineToken:
    pass

I would take your input text and split it into words first, but do it a line at a time so I don't need to read the whole file into memory. Also, don't use file as the name of a temporary file since it shadows a builtin.

The split method when called with no arguments splits on whitespace.

with open("corpus.txt") as fh:
    for line in fh:
        words = line.split()
        # do stuff with words

After that you can strip punctuation from your words and lowercase it.

word.lower().translate(None, string.punctuation)

I'd probably wrap that all up in a generator that looks something like this.

def stripped_words(path):
    with open(path) as fh:
        for line in fh:
            words = line.split()
            for word in words:
                 yield word.lower().translate(
                         None,
                         string.punctuation
                 )

More about string.punctuation here https://stackoverflow.com/questions/265960/best-way-to-strip-punctuation-from-a-string-in-python

The markov chain construction is good. zip automatically truncates the longer iterator and .setdefault is really useful. I actually didn't know about that until reading this. You'd have to modify it a little bit if you're iterating over words from your file rather than working with an array.

This line selects a word uniformly at random regardless of how many times it appears in the text rather than selecting a word with probability equal to its frequency. I'm not sure what the requirements are, but it is something that jumped out at me.

# more frequent words aren't more likely to be chosen here
current_word = random.choice(list(markov_chain.keys()))

Some miscellaneous comments:

You don't need to escape everything in your first character class.

text = re.sub(r'([,.!?;:])',r' \1 ', text)

In this line

text = re.sub(r'[^a-zA-Z0-9|\,|\.|\!|\?|\;|\:]',r' ',text)

| is part of your character class, I'm not sure whether you intended it because it isn't escaped like your other non-alphanumeric characters are.


Prompt user to choose from menu The following code simply prompts the user to choose from a menu, and then it displays the choice made by that user.
It only accepts input consisting of a single digit between 1 and 6, inclusive.  Anything else will be considered as being invalid.
Questions

While it considers Ctrl-D as an invalid choice, should it be handled differently?  If so, how?

It takes user input as an int.  Would you suggest taking it as a string instead, and then use either stoi or stringstream to convert it to an int?

Can it be re-written in a more elegant and concise way?

Do you see any potential issues?

Any comments?

Source Code
#include <iostream>
#include <limits>

int getChoice()
{
    std::cout << "Main Menu\n"
        << "-------------------\n"
        << "1. Add record\n"
        << "2. Edit record\n"
        << "3. Delete record\n"
        << "4. View record\n"
        << "5. View all records\n"
        << "6. Exit\n";

    bool isValid;
    int choice;

    do
    {
        isValid = true;
        std::cout << "Enter choice: ";
        std::cin >> choice;

        if (std::cin.fail())
        {
            if (std::cin.eof())
            {
                std::cin.clear();
            }
            else
            {
                std::cin.clear();
                std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            }
            isValid = false;
        }
        else
        {
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            if (std::cin.gcount() > 1)
            {
                isValid = false;
            }
            else if (choice < 1 || choice > 6)
            {
                isValid = false;
            }
        }
        if (!isValid)
        {
            std::cout << "Invalid choice\n";
        }
    } while (!isValid);

    return choice;
}

int main()
{
    int choice;
    choice = getChoice();

    std::cout << "User chose option " << choice << std::endl;

    return 0;
}

Input and Results
Main Menu
-------------------
1. Add record
2. Edit record
3. Delete record
4. View record
5. View all records
6. Exit
Enter choice: x
Invalid choice
Enter choice: 8
Invalid choice
Enter choice: 2x
Invalid choice
Enter choice: x2
Invalid choice
Enter choice: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Invalid choice
Enter choice: 888888888888888888888888888888888888888888888888888888888888888888888888888
Invalid choice
Enter choice: Invalid choice // Ctrl-D entered here
Enter choice: ^Z
[1]+  Stopped                 ./getchoice_cpp

Answers to your questions

While it considers Ctrl-D as an invalid choice, should it be handled differently? If so, how?

You should treat the end of the file as the definite end of the input, and return immediately with a value that indicates a failure. Not all EOFs are Ctrl-D's, there is no guarantee that you can even continue reading from std::cin after you encounter an EOF; for example the user might have redirected standard input to a file.

It takes user input as an int. Would you suggest taking it as a string instead, and then use either stoi or stringstream to convert it to an int?

As you have probably seen std::cin >> some_int does not read a whole line, instead it tries to read an integer but stops as soon as it encounters something that is not an integer. Instead of fighting against this, use std::getline() if you want to read a line, and then indeed use std::stoi() or something similar to convert that to a number. You still need to deal with the fact that also stoi() stops as soon as it sees the first character that is not part of an integer.

Can it be re-written in a more elegant and concise way?

Yes, see below.

Do you see any potential issues?

There is code duplication and unnecessary complexity dealing with std::cin. Also, the function is not very generic; if you want to get a choice for a different menu you would have to write a new function.
Reading a line with an integer in it
First, read a single line, and check if that succeeded. If not, either return some value indicating an error, or throw an exception. For example:
std::string line;

if (!std::getline(std::cin, line)) {
    throw std::runtime_error("Error while reading input");
}

Then try to convert it with std::stoi() or any of its variants, depending on the type of integer. Use the second argument to std::stoi() to know where it stopped parsing the integer, and check that this was at the end of the string:
try {
    size_t endpos;
    size_t value = stoi(line, &endpos);

    if (endpos == line.size()) {
        // Success!
        ...
    }
} catch (std::logic_error &) {
    // Error parsing the integer
    ...
}

Make getChoice() more generic
You hardcoded the choices in getChoice(). If you have a program with multiple menus, then you would have to write different versions of getChoice(), duplicating a lot of code. So try to make it more generic. For example, you can have getChoice() take two parameters: one is a string with the title of the menu, the second is a vector of strings with the possible choices.
Full example
Here is an example of what it could look like:
#include <iostream>
#include <stdexcept>
#include <string>
#include <vector>

size_t getChoice(const std::string &title, const std::vector<std::string> &choices)
{
    std::cout << title << "\n"
              << "-------------------\n";

    for (size_t i = 0; i < choices.size(); ++i) {
        std::cout << i << ". " << choices[i] << "\n";
    }

    while (true) {
        std::cout << "Enter choice: " << std::flush;
        std::string line;

        if (!std::getline(std::cin, line)) {
            throw std::runtime_error("Could not read input");
        }

        try {
            size_t endpos;
            size_t choice = stoul(line, &endpos);

            if (endpos == line.size() && choice < choices.size()) {
                return choice;
            }
        } catch (std::exception &) {
            // nothing to do here
        }

        std::cout << "Invalid choice\n";
    }
}

int main()
{
    static const std::vector<std::string> choices = {
        "Add record",
        "Edit record",
        "Delete record",
        "View record",
        "view all records",
        "Exit",
    };

    size_t choice = getChoice("Main menu", choices);

    std::cout << "User chose option " << choice << std::endl;
}

Note that I made the option numbers start with zero, just to keep the code slightly simpler. I also used size_t, as it is unsigned (no need to deal with negative numbers), and it's good practice whenever you deal with indices into arrays, vectors and so on. Note that you don't have to create a named vector in main(), you can even pass a literal to getChoices():
size_t choice = getChoice("Menu", {"First option", "Second option", ...});