Which is better-way to casting model I need to convert my model to fit data in tableview cell finding better approach
My Model

class MyResponseModel {
    var question: String = ""
    var id: String = ""
    var answer: String = ""
    //Other variables also but not required to display
     init(fromDictionary dictionary: [String:Any]) {
        self.question = (dictionary["question"] as? String) ?? ""
        self.id = (dictionary["id"] as? String) ?? ""
        self.answer = (dictionary["answer"] as? String) ?? ""
    }
    //MARK: Converting model to cell data
    func toModel() -> MyTblCellDetail {
        return MyTblCellDetail(title:self.question,detail:self.answer)
    }
}
//MARK: Initialize cell data with custom model
struct MyTblCellDetail {
    var title: String = ""
    var detail: String = ""
    var isSelected:Bool = false

    init(title:String,detail:String) {
        self.title = title
        self.detail = detail
    }
    init(data:MyResponseModel) {
        self.title = data.question
        self.detail = data.answer
    }
}

Note: I am reusing same cell to display most of the data so i will use
the same method to converting all class model to MyTblCellDetail
struct

So my question is which is better way to do the same or any other approach i can use for the same. Thank you!

Here is suggestions. Every approach has pros/cons, depending your app architecture and model complexity.
class MyResponseModel {
  // I think values in response model should be let instead of var
  // By definition response model should be immutable
  let question: String
  let id: String
  let answer: String

  // I guess MyResponseModel will be initialized from JSON and you can just conform it to Codable
  // Then you don't need this init(from dictionary: [String: Any])
  init(from dictionary: [String: Any]) {
    self.question = (dictionary["question"] as? String) ?? ""
    self.id = (dictionary["id"] as? String) ?? ""
    self.answer = (dictionary["answer"] as? String) ?? ""
  }
}

struct MyTableCellViewModel {
  // I'm not sure what logic do you have, but I think it is better to define properties using let instead of var
  let title: String
  let detail: String
  let isSelected: Bool

  init(title: String, detail: String, isSelected: Bool) {
    self.title = title
    self.detail = detail
    self.isSelected = isSelected
  }

  // Approach #1: create initializer from necessary response model
  init(responseModel: MyResponseModel, isSelected: Bool) {
    self.init(title: responseModel.question, detail: responseModel.answer, isSelected: isSelected)
  }
}

// Approach #2: create separate mapper class (useful when you need to have external dependencies in order to map
// For example you need to add some info that is not relevant to MyResponseModel.
// Let's take cell selection info as an example

protocol MyTableCellSelectionProvider {
  func isCellSelected(for responseModel: MyResponseModel) -> Bool
}

struct MyTableCellViewModelMapper {
  let cellSelectionProvider: MyTableCellSelectionProvider

  func map(responseModel: MyResponseModel) -> MyTableCellViewModel {
    return MyTableCellViewModel(
      title: responseModel.question,
      detail: responseModel.answer,
      isSelected: cellSelectionProvider.isCellSelected(for: responseModel)
    )
  }
}

// Approach #3: define cell view model protocol

protocol MyCellViewModel {
  var title: String { get }
  var detail: String { get }
}

extension MyResponseModel: MyCellViewModel {
  var title: String { return question }
  var detail: String { return answer }
}


Checking the difference between old and new XML I have two xml files, about 500mb each, an old and a new. I want to find out what items have been added or removed in the new.xml compared with the old.xml.

I had some help on this code and I'm a bit new to C++ but I was wondering if this is the optimal way to approach a problem such as this.

#include <set>
#include <string>
#include <sstream>
#include <iostream>
#include <algorithm>

#include "include/pugixml.hpp"

#define con(m) std::cout << m << '\n'
#define err(m) std::cerr << m << std::endl

using str_set = std::set<std::string>;

int main()
{
    pugi::xml_document doc;

    str_set a;
    doc.load_file("old.xml");

    // fill set a with just the ids from file a
    for(auto&& node: doc.child("site_entries").children("entry"))
        a.emplace(node.child("id").text().as_string());

    str_set b;
    doc.load_file("new.xml");

    // fill set b with just the ids from file b
    for(auto&& node: doc.child("site_entries").children("entry"))
        b.emplace(node.child("id").text().as_string());

    // now use the <algorithms> library

    str_set b_from_a;
    std::set_difference(a.begin(), a.end(), b.begin(), b.end()
        , std::inserter(b_from_a, b_from_a.begin()));

    str_set a_from_b;
    std::set_difference(b.begin(), b.end(), a.begin(), a.end()
        , std::inserter(a_from_b, a_from_b.begin()));

    str_set a_and_b;
    std::set_intersection(a.begin(), a.end(), b.begin(), b.end()
        , std::inserter(a_and_b, a_and_b.begin()));

    for(auto&& v: a)
        con("a       : " << v);

    con("");

    for(auto&& v: b)
        con("b       : " << v);

    con("");

    for(auto&& v: b_from_a)
        con("b_from_a: " << v);

    con("");

    for(auto&& v: a_from_b)
        con("a_from_b: " << v);

    con("");

    for(auto&& v: a_and_b)
        con("a_and_b : " << v);

    con("");
}

Your code seems simple and how it works is clear enough. I only have a few remarks:

This line seems a little bit off:

#include "include/pugixml.hpp"

Generally speaking, in C and C++, libraries provide an include dir that has to be added to the search directories so that we can write:

#include "pugixml.hpp"

While it certainly does not make your code invalid, it makes it feel a bit less pedantic.
Your code currently does not handle parsing errors. See pugi's documentation for more information about that.
Now, let's have a look at your set operations. We will use the following diagram to simplify the explanation:

You are currently computing \$A \setminus B\$ and \$B \setminus A\$, then \$A \cap B\$ (where \$A \setminus B\$ represents the difference between \$A\$ and \$B\$). Technically speaking, it would be faster to compute \$A \cap B\$ first then to compute \$A \setminus (A \cap B)\$ and \$B \setminus (A \cap B)\$ since it would mean fewer elements to compare in the sets (there are fewer elements in \$(A \cap B)\$ than there are in \$A\$ or in \$B\$).

In other words, your code should be:

str_set a_and_b;
std::set_intersection(a.begin(), a.end(), b.begin(), b.end()
    , std::inserter(a_and_b, a_and_b.begin()));

str_set b_from_a;
std::set_difference(a.begin(), a.end(), a_and_b.begin(), a_and_b.end()
    , std::inserter(b_from_a, b_from_a.begin()));

str_set a_from_b;
std::set_difference(b.begin(), b.end(), a_and_b.begin(), a_and_b.end()
    , std::inserter(a_from_b, a_from_b.begin()));


Percentage of digits in n that are even Question 7 from Lab 3 of Berkeley's CS61A course says:

The following examples of recursive functions show some examples of common recursion mistakes. Fix them so that they work as intended. [...]
def even_digits(n):
    """Return the percentage of digits in n that are even.

    >>> even_digits(23479837) # 3 / 8
    0.375
    """
    if n == 0:
        return num_digits / num_evens
    num_digits, num_evens = 0, 0
    if n % 2 == 0:
        counter += 1
    num_evens += 1
    return (even_digits(n // 10) + num_evens) / counter

As per the assignment, below code is suppose to solve this problem, recursively (MUST not be non-recursive).
def even_digits(n, count_even = 0, count = 0):
    """Return the percentage of digits in n that are even.

    >>> even_digits(23479837) # 3 / 8
    0.375
    """
    if n == 0:
        return count_even / count
    if (n % 2) == 0:
        return even_digits(n // 10, count_even + 1, count + 1)
    else:
        return even_digits(n // 10, count_even, count + 1)

Is this code readable? Can we improve this solution?

There is a serious error in your code. If the function is called as even_digits(0), then it causes the following error:

ZeroDivisionError: integer division or modulo by zero

since in the first if you divide by count which is 0.

So, before improving the code, you should check if it is correct.

ADDED:

THE CASE OF n = 0

It is interesting to understand what should be the result in case the input (that is n) is 0.

The problem is labeled as:

  Percentage of digits in n that are even

and the natural interpretation is:

  Percentage of digits in the decimal representation of n that are even

So, what result should give the function when provided with the value 0? I argue it should be 1, since every decimal representation of 0 has only zeroes as digits, so it has a 100% even digits.


Reversing the digits of an integer For CS class, I had to reverse the digits of an integer from the input. This is my solution:

System.out.println(new StringBuilder(new Integer(new Scanner(System.in).nextInt()).toString()).reverse().toString());

My main question is this: is this too golfed?

Obviously I could simply add whitespace and comments, but is this level of directness practical?

Also, is there a better way to do this? Perhaps something like the following:

final String string = new Integer(new Scanner(System.in).nextInt()).toString();
for (int i = string.length(); i != -1; System.out.print((i-- != 0) ? string.charAt(i) : "\n"));

In Java, and in Code Review, the preference is for creating both highly readable, and highly efficient code.

It is an almost bizarre fact, that one leads to the other.

your code has one efficiency, and that is space-on-disk.... it is neither readable, nor high-performance.

System.out.println(new StringBuilder(new Integer(new Scanner(System.in).nextInt()).toString()).reverse().toString());

For a start, having to scroll off the page means you have to read with your mouse, not with your eyes.
Secondly, it is very hard to evaluate what parenthesis match when they are all clumped together.

Breaking your code down (which should not be necessary), we have:

int valinput = new Scanner(System.in).nextInt();
String stringinput = new Integer(valinput).toString();
String reversed = new StringBuilder(stringinput).reverse().toString();
System.out.println(reversed);

Now, you need error handling on the Scanner, and it needs to be closed as well. Without that, you have a program that is ugly when a user types in 123.0.

Since you have no error handling, you may as well just junk the lines with the Integer...

And, there is no reason to convert the StringBuilder to a String directly...

Without error handling, you can:

System.out.println(new StringBuilder().append(scanner.nextInt()).reverse());

That is, effectively equivalent to your code, but it has different error-condition characteristics..

Also, it is, suprisingly, quite readable, and fast.

Now, all you need is the try/catch for the scanner, and you are sorted.


Design for class which accepts large array in constructor Let's say we have the following class which performs interpolation on irregularly spaced data:

template<class Real>
class interpolator
{
public:
    interpolator(const Real* const x, const Real* const y, size_t length)
    {
        m_x.resize(length);
        m_y.resize(length);
        for (size_t i = 0; i < length; ++i)
        {
             m_x[i] = x[i];
             m_y[i] = y[i];
        }
    }

    Real interpolate_at(Real x)
    {
        // use m_x and m_y to produce an estimate of y(x):
        return something;
    }

private:
    std::vector<Real> m_x;
    std::vector<Real> m_y;
};

This design has a number of things going for it: For one it is easy to use:

std::vector<double> x;
std::vector<double> y;
// Initialize x and y ...
interpolator<double> interp(x.data(), y.data(), y.size());

Since the constructor accepts primitive types, it can also be wrapped in Python with moderate effort (you'd need a shared object and some template instantiations, but let's assume that can be done). You can also let x and y go out of scope and the interpolator will still work.

The tragic flaw is that it performs a huge memcpy, which is slow and not space efficient.

Is there a way to refactor this class such that

It is easy to wrap in Python (std::shared_ptr's in interfaces are a nightmare to wrap . . .)
It doesn't perform a memcpy
The input data can be allowed to go out of scope and the interpolator still work

Don't pass arrays as pointers ...

... for multiple reasons. Your current design

always copies the arrays x, y deeply
can be misused by passing nullptr and trusts the user that length is chosen correctly.

Instead of

interpolator(const Real* const x, const Real* const y, size_t length) {  /* ... */ }

use

interpolator(std::vector<Real> x, std::vector<Real> y)
: m_x{std::move(x)}
, m_y{std::move(y)}
{ if (m_x.size() != m_y.size()) throw std::logic_error{"NO!"}; }

or if the memory layout doesn't effect the performance too much

interpolate(std::vector<std::array<Real, 2>> xy): m_xy{std::move(xy)} {}

If you want to initialise your data from any range use templates and constrain its type to guard for errors at compile time.

Prefer regular functions for computations

This means, that an implemented function is also a function in the mathematical sense. Same input data produces same output data. I found that this almost always leads to evaluations which do not change an inner state. Thus it smells to me that

Real interpolate_at(Real);

is not

Real interpolate_at(Real) const;


C.Insertion sort with guard I have already addressed this issue and corrected something .really..now the graph looks different
Please tell me if the program works correctly?You can see the results in the picture above.I will be grateful if you find any mistakes (if there are any, of course..)
/I'm just not sure if the average number of swaps and comps is calculated correctly.I thought that with this sort there should be fewer of them, but the data obtained from the classic Insertion sort is not much different.Maybe that's how it should be, which is why I'm asking./
Thank you all in advance
 #include <stdio.h>
    #include <stdlib.h>
    #include <time.h>

    int main(int argc, char *argv[]) {

        FILE *f=fopen("stat18.csv","w");
        int n=100;
        int i,s;
        while (n<=10000){
        int st=0;
        for (s=0;s<5;s++)
        {
            int *a;
            a=(int *)malloc(n*sizeof(int));
            srand(time(NULL));
            int k;
            for (k=0;k<n;k++)
            {
                *(a+k)=rand()%50;
            }
            int j,comps,swaps;
            swaps=0;
            comps=0;
        int rem = a[0];
        a[0] = -1;

        for( i = 2, j; i < n; i++)
        {
            int x = a[i];
            j = i;
            while(j > 1 && a[j-1] > x)
            {
                comps += 2;
                swaps++;
                a[j] = a[j-1];
                j--;
            }
            comps += 2;
            a[j] = x;
        }
        a[0] = rem;
        for(i = 0; i < n-1; i++)
        {
            if(a[i] > a[i+1])
            {
                comps++;
                swaps++;
                int temp = a[i];
                a[i] = a[i+1];
                a[i+1] = temp;
            }
            comps++;
            i++;
        }
        st+=swaps + comps;
        free(a);
    }

        st=st/5;
        fprintf(f,"%d ; %d\n", n , st);
        if (n<1000){
            n+=100;}
        else {n+=1000;}
        }
        fclose(f);
        return 0;
    }

Random seeding

        srand(time(NULL));

You call this inside a loop (or two loops actually).  But here's the problem, time returns the current number of seconds since an arbitrary historical point of time (1970 January 1st at midnight).  Most of the time in your loop, that will be the same number.  So you are seeding the number the same.  But occasionally you will tick over to the next second.
You should either create a variable
    time_t invocation_time = time(NULL);

that you can then use
        srand(invocation_time);

Or just move that seeding call outside the loops.
You would create the variable if you want to always seed with the same number so you get the same "random" sequence each time.  You could even set the value to something fixed rather than the current time.  I just used the current time because that's what you had been using and I have no particular reason to prefer a different value.  By creating the variable, you ensure that you will be seeding with the same value each time and will get the same sequence of numbers.  You won't sometimes repeat and sometimes not, as you do now.
If you want a different sequence each time, then just seed the randomness once per invocation of your program.  Repeated calls to srand make things less random rather than more.  You would only do this if you wanted to repeat the original results.  And frankly, if that's what you wanted, it would probably be easier to just generate the data once and make a copy for each iteration.
Calling srand once at the beginning of the program is the more typical approach.  Calling it multiple times with the same value is an option if you want to make the values repeatable in some way.  The way that you are doing it now is neither of those things.  It is neither random nor repeatable.  Sometimes it repeats and sometimes it is from a pseudorandom sequence.  Which is somewhat unpredictable, although there likely would be patterns.
I think that what you want in this situation is to seed once at the beginning of the program, so you get different values in a each time.  But it really depends on exactly what you are trying to test.
Readability

            *(a+k)=rand()%50;

It would be more common to write this
            a[k] = rand() % 50;

Now we can easily see that you are doing an array dereference.  It's also conceivable that this might have performance impact.  Because iterating over an array is something that would be done frequently and might be optimized.  Whereas pointer addition is rarer and might not be, even if identical in effect.  But I still think that the primary reason to write it this way is that it is easier for people reading your code to see what's happening.
You will also notice that I added whitespace.  This also makes it easier to see what things go together and what are different.  The compiler will remove this during tokenization, so it has impact on the executable.  But for the source code, it makes it easier for the person reading the code to figure out what it is doing.
Consistent indentation
Similarly, it will be easier to read your code if you always indent inside a block and reduce the indentation when the block ends.  This makes it easier to see where your code starts and ends.
Counting
Now to get to the question that you were actually asking.

        for( i = 2, j; i < n; i++)
        {
            int x = a[i];
            j = i;
            while(j > 1 && a[j-1] > x)
            {
                comps += 2;
                swaps++;
                a[j] = a[j-1];
                j--;
            }
            comps += 2;
            a[j] = x;
        }

Whether this is counting the comparisons correctly or not depends on exactly what you want to count.
You are not counting a comparison i < n that you are doing on each iteration of the outer loop.  I would usually think of this as something that you should measure.
You are counting two comparisons on the failure of the inner loop even when only one is done.  I.e. when j is equal to 1, you count two comparisons even though short circuiting will only do the j > 1 and not the a[j-1] > x.  Perhaps you want to count as if short circuiting does not occur.
In terms of swaps, you aren't counting either

            int x = a[i];

or

            a[j] = x;

but you are counting each

                a[j] = a[j-1];

What's a swap?  If it is an assignment, then you should count the first two.  If it is an exchange, you should not count the last one, as it is not a swap.  You are doing no "swaps" in that loop.  You are moving a block of memory.
Note that what I'd actually call a swap involves three assignments (although it's quite possible that only two of them hit memory, as one can be held in a register).  So if your improvement to insertion sort is that you are doing fewer assignments, you are counting the wrong thing.  You should be counting assignments, not swaps.

        for(i = 0; i < n-1; i++)
        {
            if(a[i] > a[i+1])
            {
                comps++;
                swaps++;
                int temp = a[i];
                a[i] = a[i+1];
                a[i+1] = temp;
            }
            comps++;
            i++;
        }

Comparing
I'm having trouble understanding what you consider to be "classic" insertion sort and what your revision is.  This would be easier to understand if you used functions.  Then you could call each with the same data and compare those results.  And for those of us reading your code, we could see what was different between the two functions.
That would also create more of a separation between your test setup and the sorting.  As is, those things are pretty much run together.  There isn't a clear demarcation between what is setup and what is sorting.  If your sorting were entirely in functions, then it would be clearer.  The stuff outside the functions is setup.  The stuff inside is sorting.
It would also be easier if we could see the results from both the classic and revised versions.  Perhaps they are showing data as expected.  Perhaps not.  Without the classic version, it's hard for me to see what you are trying to compare.  So it's difficult to critique it.


Which is the better way to return a new instance of a boost intrusive_ptr Unlike most(?) smart pointers, boost::intrusive_ptr has a non-explicit constructor. Given that, one could write

typedef boost::intrusive_ptr<Foo> FooPtr;

FooPtr MyFactory()
{
    return new Foo(a, b, c);
}

Or one could write

FooPtr MyFactory()
{
    return FooPtr(new Foo(a, b, c));
}

The latter is more consistent with how the code would have to be written if FooPtr were another kind of smart pointer, like shared_ptr or unique_ptr. The former is more concise. I personally prefer concise as long as it doesn't lead to trouble for readers and maintainers later. I'm wondering whether the former case is "too" concise?

When in doubt, be explicit.  While both of the functions given will work, the second expresses programmer intent more clearly.  Adding a comment along the lines of "This will call intrusive_ptr's implicit constructor" to the first can help, though at that point it's more concise to call the constructor directly.  Returning a raw pointer and relying on the implicit constructor for intrusive_ptr may also be viewed as a mistake introduced by changing the declaration of MyFactory() but forgetting its definition.  Relying on implicit constructors may also make the code harder to debug and maintain because by default, the constructor for intrusive_ptr increments the retain count in Foo.  Calling it explicitly gives you the benefit of being able to search for "FooPtr(" when trying to track down every place a Foo is  retained.  Also, if the Foo constructor initializes its retain count to 1, you may be introducing a memory leak by relying on the implicit constructor.  Explicitly specifying FooPtr(new Foo(a, b, c), false) would prevent this.


Reduce duplication while accessing Visual Studio services If you write a VS extension, you can get access to VS services with the IServiceProvider interface:

var vsUIShell = (IVsUIShell)serviceProvider.GetService(typeof(SVsVsUIShell));
vsUIShell.SetWaitCursor(); // for example

To reduce duplication every time I need this, I created a class like this:

static class VisualStudioServices
{
    public static IVsUIShell UIShell { get { return vsUIShell.Service; } }
    static readonly LazyService<SVsUIShell, IVsUIShell> vsUIShell = new LazyService<SVsUIShell, IVsUIShell>();

    class LazyService<TService, T>
    {
        static T GetService()
        {
            return (T)ServiceProvider.GlobalProvider.GetService(typeof(TService));
        }

        readonly Lazy<T> serviceLazy = new Lazy<T>(GetService);

        public T Service { get { return this.serviceLazy.Value; } }
    }
}

To add a new service to the list, I have to mention its name 8 time (assuming the Service type and the interface type are the same, which is not always true).

I'm looking for a way to reduce this duplication further. Any suggestions?

Your current design will potentially make unit testing code which uses it hard. Consider making the VisualStudioServices class non-static and create an interface for it. You could then inject the ServiceProvider as a dependency rather having an implicit dependency on the static GlobalProvider. Something like this:

interface IVisualStudioServices
{
    IVsUIShell UIShell { get; }
}

class VisualStudioServices : IVisualStudioServices
{
    public IVsUIShell UIShell { get { return _VsUiShell.Service; } }

    private readonly LazyService<SVsUIShell, IVsUIShell> _VsUiShell;

    public VisualStudioServices(ServiceProvider serviceProvider)
    {
        _VsUiShell = new LazyService<SVsUIShell, IVsUIShell>(serviceProvider);
    }

    class LazyService<TService, T>
    {
        ServiceProvider _ServiceProvider;
        LazyService(ServiceProvider serviceProvider)
        {
            _ServiceProvider = serviceProvider;
        }

        readonly Lazy<T> serviceLazy = new Lazy<T>(() => (T)_ServiceProvider.GetService(typeof(TService)));

        public T Service { get { return serviceLazy.Value; } }
    }
}

Any code requiring access to it should have a dependency on IVisualStudioServices which can be easily mocked out.


Scrape every Java themed codingbat problem set and your personal solutions I made the switch from JavaScript to Python lately. This is my first Python project and my first web-scraper. Do I have good practices? And could this be optimized?
from bs4 import BeautifulSoup
import requests

urlHalf = 'codingbat.com'

print("what is your email")
inputOne = input(">")
print("password")
inputTwo = input(">")

post_params = {
    "uname": inputOne,
    "pw": inputTwo,
}

# urls to use
base_url = "https://codingbat.com"
login_url = "https://codingbat.com/login"
java_url = "https://codingbat.com/java/"

def get_href():
    print("Getting directories")
    href_array = []
    initial = requests.get(base_url)
    soup = BeautifulSoup(initial.content, "lxml")
    tab_data = soup.find(class_="floatleft")
    hrefs = tab_data.find_all(class_="h2")
    for href in hrefs:
        href_array.append(href.text)
    return(href_array)

def gather_links():
    directories = get_href()
    print("Getting problem links")
    link_array = []
    for directory in directories:
        r = requests.get(java_url + directory)
        # get links to problems
        content = BeautifulSoup(r.content, "lxml")
        links = content.find_all(
            "a", href=lambda href: href and "prob" in href)
        link_array.append(links)
    return(link_array)

def join_links():
    linkList = gather_links()
    new_link_list = {}
    print("Joining links")
    for link in linkList:
        for each in link:
            text = each.text
            backUrl = each.get("href")
            realUrl = "http://" + urlHalf + backUrl
            new_link_list.update({text: realUrl})
    return new_link_list

def scrape_data():
    f = open('answers/answers.java', 'w')
    linkList = join_links()
    with requests.Session() as session:
        post = session.post(login_url, data=post_params)
    for i in linkList:
        print("Scraping: " + i)
        response = session.get(linkList[i])
        content = BeautifulSoup(response.content, "lxml")
        indentDiv = content.find(class_="indent")
        table = indentDiv.find("form", {"name": "codeform"})
        aceDiv = table.find("div", id="ace_div")
        f.write("//" + i + ": " + linkList[i] + "\n\n")
        f.write(aceDiv.text + "\n")
    print("done")

if __name__ == "__main__":
    scrape_data()

Global nomenclature
urlHalf should be URL_HALF. That said, a better name would be DOMAIN, since that's what that string actually is.
Further, after declaring this you don't use it correctly; these:
base_url = "https://codingbat.com"
login_url = "https://codingbat.com/login"
java_url = "https://codingbat.com/java/"

could be
BASE_URL = 'https://' + DOMAIN
LOGIN_URL = BASE_URL = '/login'
JAVA_URL = BASE_URL + '/java'

Input
print("what is your email")
inputOne = input(">")
print("password")
inputTwo = input(">")

should not occur at global scope; it should occur in a function. Also, the use of input for passwords is insecure; use getpass instead.
Return parens
These parens:
return(href_array)

are redundant.
Generator
link_array = []
for directory in directories:
    # ...
    link_array.append(links)
return(link_array)

can simply be
for directory in directories:
    # ...
    yield links

or, if you want a "flattened" output that does not have nested lists, use yield from links.
File extensions
Does this file:
f = open('answers/answers.java', 'w')

actually receive Java code? It seems not, in which case - use just .txt.


Working Days DateDiff function I have a calendar table with a record for every date I need to care about (i.e. 365 records per year), with various meta-information about each date, including the weekday, the fiscal calendar week, month, quarter and year it's under, whether there's a Holiday on that date, etc.

The table has the following keys / indexes:

,constraint PK_FiscalCalendars primary key clustered (_Id)
,constraint NK_FiscalCalendars unique (CalendarDate)

and:

create nonclustered index IX_FiscalCalendars_HolidayFiscalDayOfWeek
    on dwd.FiscalCalendars (Holiday, CalendarDate, FiscalDayOfWeek);

That's nice, but now I need to use this calendar table in various queries, and one of my requirements is to be able to determine the number of workdays between two dates, taking into account the contents of the Holiday column, which contains a non-null nvarchar(50) value (the name of the Holiday / reason for closing) whenever a weekday/date shouldn't be considered a workday; the column contains a NULL value otherwise.

So I've implemented this little function that intakes two dates and returns the number of workdays between them (inclusively):

create function dbo.WorkWeekDateDiff(@fromDate date, @toDate date)
returns int
as
begin

    /* returns the number of workdays (non-holiday week days) between two dates. */
    declare @result int;

    with calendar as (
        select CalendarDate
        from dwd.FiscalCalendars
        where
            CalendarDate between @fromDate and @toDate
        and Holiday is null -- exclude holidays
        and FiscalDayOfWeek not in (1,7) -- exclude Sunday and Saturday
    )

    select @result = (select count(*) from calendar);
    return @result;

end

Execution plan is showing 73% of the cost going on seeking the non-clustered index, and 27% performing that Count(*). It gets the job done, but since this is a scalar-valued function I'd like it as efficient as possible. Is there anything to improve?

You have a physical table that has been established for the purpose of managing calendar dates. This is great, but with just a little more work it can be extended to do so much more.

Consider adding a column "DayType", with values like:

0 -> Normal
1 -> Weekend
2 -> Holiday

You could add this easily... right?

Then, you can maintain it with:

update FiscalCalendars
set DayType = case
                  when Holiday is not null then 2
                  when FiscalDayOfWeek in (1, 7) then 1
                  else 0
              end

Now your query becomes quite simple:

select count (*)
from FiscalCalendars
where CalendarDate between @fromDate and @toDate
  and DateType = 0

Index the column with (DateType, CalendarDate)

Maintaining this static data in a function would be a poor use of computational power, and make indexing harder.

Your function would thus become:

create function dbo.WorkWeekDateDiff(@fromDate date, @toDate date)
returns int
as
begin

    /* returns the number of workdays (non-holiday week days) between two dates. */
    declare @result int;

    select @result = (select count(*)
                      from dwd.FiscalCalendars
                      where DateType = 0
                        and CalendarDate between @fromDate and @toDate
    );

    return @result;

end


A simple Tic Tac Toe game I am trying to learn some Common Lisp, except basically all of my computing background is the C family of languages. So, I'm starting small. I have made a Tic-Tac-Toe game, and I am looking for some constructive criticism.

In particular, is this idiomatic Lisp? Is it styled normally? What would an experienced lisper improve about this?

Some concerns I have about this code are:

The win checking seems repetitive.
Can it be simplified?
The get-player-move is very long. Should it be broken up?

;;;; A simple tic-tac-toe game

(defvar p1 88) ; char code for 'X'
(defvar p2 79) ; char code for 'O'
(defvar tie 49) ; arbitrary constant to represent ties

(defun is-move-free (move board)
  (let ((player (elt board move)))
    (not (or (= player p1) (= player p2)))))

(defun is-same (a b c)
  (and (= a b) (= b c)))

(defun win-on-rows (board)
  (defun check-a-row (offset)
    (or (is-same (elt board offset) (elt board (+ offset 1)) (elt board (+ offset 2)))))
  (or (check-a-row 0) (check-a-row 3) (check-a-row 6)))

(defun win-on-columns (board)
  (defun check-a-column (offset)
    (or (is-same (elt board offset) (elt board (+ offset 3)) (elt board (+ offset 6)))))
  (or (check-a-column 0) (check-a-column 1) (check-a-column 2)))

(defun win-on-diagonals (board)
  (or (is-same (elt board 0) (elt board 4) (elt board 8)) (is-same (elt board 2) (elt board 4) (elt board 6))))

;;; This function gets the players move, plays it if possible
;;; then gets the next move. The game will play out in it's
;;; entirety through recursively calling this function.
(defun get-player-move (player board move-num)
  (apply #'format t " ~C | ~C | ~C ~%-----------~% ~C | ~C | ~C ~%-----------~% ~C | ~C | ~C~%" (map 'list #'code-char board)) ; Print the board
  (if (>= move-num 9) ; if all the moves have been played, and there is no winner
    tie               ; return the tie constant
    (let ((move (- (parse-integer (read-line)) 1))) ; get the move from input, and convert it to list location
      (if (is-move-free move board)
        (let ((board (substitute player (elt board move) board))) ; apply the move, and get the new board
          (if (or (win-on-columns board) (win-on-rows board) (win-on-diagonals board)) ; check if this was the winning move
            (elt board move) ; return the winner
            (get-player-move (if (= player p1) p2 p1) board (+ move-num 1)))) ; continue the game
        (get-player-move player board move-num))))) ; move again, if the move was taken

(let ((result (get-player-move p1 '(49 50 51 52 53 54 55 56 57) 0)))
  (if (= result tie)
    (format t "It's a tie!")
    (format t "The winner is ~C" (code-char result))))

Here are a few considerations.

DEFPARAMETER vs. DEFVAR

The first three definitions should be given with defparameter instead of defvar. The first operator is used for values that do not change, unless they are modified in the source file and then recompiled. The second for variables that can be modified at run time but, when reloaded and recompiled, does not modify the current run-time value (i.e. are not re-initialized).

Comparison operators on more than one value

In Common Lisp is very handy the possibility of using all the comparison operators with more then one argument. For instance if you want to check if the numbers a, b and c are all equal, you can simply write (= a b c); if you want to check that they are all different among them (that is that the are no two values equal), you can write (\= a b c). This can simplify the function is-move-free as well as make non necessary the function is-same.

Semplification of the win functions

In effect they have all the same pattern: you compare three value starting with a certain index and then incrementing that index with the same value two times. For this reason I suggest a more simple function that includes all the three:

(defun win (board)
  (flet ((check (start increment)
                (= (elt board start) (elt board (incf start increment)) (elt board (incf start increment)))))
    (some #'check '(0 3 6 0 1 2 0 2) '(1 1 1 3 3 3 4 2))))

Note:

The use of the flet to define an internal function: defun should be used only for top level functions. In case of internal recursive functions you should use labels instead of flet.
The use of incf which performs a side-effect incrementing the value of a variable (which is safe in this case since in Common Lisp parameters are passed by value).
The use of some which is a functional that returns a true value if some predicate is at least once true when applied to one (or more) lists. In this case the predicate is check and it is applied to the list of starting indexes, and the list of the increments.

The final code

Here is the entire code with my modifications:

(defparameter p1 88) ; char code for 'X'
(defparameter p2 79) ; char code for 'O'
(defparameter tie 49) ; arbitrary constant to represent ties

(defun is-move-free (move board)
  (let ((player (elt board move)))
    (/= p1 p2 player)))

(defun win (board)
  (flet ((check (start increment)
                (= (elt board start) (elt board (incf start increment)) (elt board (incf start increment)))))
    (some #'check '(0 3 6 0 1 2 0 2) '(1 1 1 3 3 3 4 2))))

;;; This function gets the players move, plays it if possible
;;; then gets the next move. The game will play out in it's
;;; entirety through recursively calling this function.
(defun get-player-move (player board move-num)
  (apply #'format t " ~C | ~C | ~C ~%-----------~% ~C | ~C | ~C ~%-----------~% ~C | ~C | ~C~%" (mapcar #'code-char board)) ; Print the board
  (if (>= move-num 9) ; if all the moves have been played, and there is no winner
      tie               ; return the tie constant
      (let ((move (- (parse-integer (read-line)) 1))) ; get the move from input, and convert it to list location
        (if (is-move-free move board)
            (let ((board (substitute player (elt board move) board))) ; apply the move, and get the new board
              (if (win board) ; check if this was the winning move
                  (elt board move) ; return the winner
                  (get-player-move (if (= player p1) p2 p1) board (+ move-num 1)))) ; continue the game
            (get-player-move player board move-num))))) ; move again, if the move was taken

(let ((result (get-player-move p1 '(49 50 51 52 53 54 55 56 57) 0)))
  (if (= result tie)
      (format t "It's a tie!")
      (format t "The winner is ~C" (code-char result))))

Note the use of the more common indentention convention for the if, as well as the semplification of the format.

An iterative version

Common Lisp has been designed to be used both with applicative (i.e. functional) style, as well as imperative (i.e. with side-effect) style. So, instead of using the applicative style of creating each time a new board with substitute and passing it to a recursive call of the function get-player-move, an alternative could be a simple iterative style, like for instance:

(defun get-player-move (player board move-num)
  (loop
    (apply #'format t " ~C | ~C | ~C ~%-----------~% ~C | ~C | ~C ~%-----------~% ~C | ~C | ~C~%" (mapcar #'code-char board)) ; Print the board
    (let ((move (- (parse-integer (read-line)) 1)))
      (when (is-move-free move board)
        (setf (elt board move) player)
        (when (win board) (return player))
        (incf move-num)
        (when (= move-num 9) (return tie))
        (setf player (if (= player p1) p2 p1))))))

Note that in this case, since the board is modified, one should create it initially with (list 49 50 51 52 53 54 55 56 57), instead of using a constant like '(49 50 51 52 53 54 55 56 57).


Customized Apache host files I'm using customized Apache host files for my Digitalocean websites. Please review the code, whether it's following the good standards or not, and please mention if I'm missing something important. Here is the host file template that I'm using for all my websites (on Ubuntu 16.04):

<VirtualHost *:80>
    ServerName example.com
    ServerAlias www.example.com
    Redirect permanent / https://example.com/
</VirtualHost>

<VirtualHost *:443>
    ServerName www.example.com

    SSLEngine on
    SSLCertificateFile /home/username/certs/example.com.pem
    SSLCertificateKeyFile /home/username/certs/example.com.key

    Redirect permanent / https://example.com/
</VirtualHost>

<VirtualHost *:443>
    ServerName example.com
    DocumentRoot /var/www/example.com/public_html

    SSLEngine on
    SSLCertificateFile /home/username/certs/example.com.pem
    SSLCertificateKeyFile /home/username/certs/example.com.key
</VirtualHost>

<Directory /var/www/example.com/public_html/>
    Options +ExecCGI
            Options FollowSymLinks MultiViews
            AllowOverride All
            Order allow,deny
            allow from all
</Directory>

You don't actually need ServerName and ServerAlias directives for your port 80. This is due to the fact that your are using port-based virtual host. The "*" in the  tag indicates that all incoming connection to port 80 will follow the rules set with within the   tags. However, it is better to keep logs about incoming connections and errors when redirecting.

Therefore, for your port 80, the configuration should be like this:

<VirtualHost *:80>

Redirect permanent / https://example.com/

ErrorLog ${APACHE_LOG_DIR}/error.log
CustomLog ${APACHE_LOG_DIR}/access.log combined

</VirtualHost>

For port 443, the Redirect directive you put there will cause a redirect loop and you will need to remove that. Furthermore, you will need to include the CGI path within the  tags.
It is recommended that you put the CGI files outside of the DocumentRoot for security reasons (malicious visitors might reveal your code and attempt to do something bad). In this case let's say your CGI executables are located under the directory /usr/local/cgi-bin/ . Following that You will need to add the ScriptAlias directive to indicate the path to the CGI directory. Lastly, you need the AddHanlder directive to tells Apache to permit the execution of CGI files.

Remember to keep the logs for HTTPS connection too.

So here you go the configuration for port 443:

<VirtualHost *:443>

    ServerName example.com
    DocumentRoot /var/www/example.com/public_html

    SSLEngine on
    SSLCertificateFile /home/username/certs/example.com.pem
    SSLCertificateKeyFile /home/username/certs/example.com.key

    ErrorLog ${APACHE_LOG_DIR}/https_error.log
    CustomLog ${APACHE_LOG_DIR}/https_access.log combined

    ScriptAlias "/cgi-bin/" "/usr/local/cgi-bin/"

    <Directory "/usr/local/cgi-bin/">
         Options +ExecCGI
         AddHandler cgi-script .cgi .pl
    </Directory>

</VirtualHost>

The tutorial section located under the Apache2.4 Documentation will give you a fairly complete overview for this.


"Stardust" 2D Gravity Simulator - Follow Up 1: The Planets This is a follow up for the 2D Gravity Simulator-like Game question.

Since then I have developed my javascript skills further, and now I'm using ES6 mainly. Note that even through this code uses the EaselJS library, the question is principally about its design so no EaselJS skill needed.

Also I wanna avoid posting a bigger version of the bible here, so this question is only about the game's Planet class and its properties.

The code is more well divided, but I don't know if this organization is effective, and how can changes in a planet's .physicsBody affect its graphical .skin – colors specially. Any tips and ratings on how I'm using class inheritance are really appreciated.

Also, is there any part that could use a more ES6-ish approach?

Planet class:

class Planet {
    constructor (x, y, radius, color) {
        this.physicsBody = new CircleBody(x, y, radius);
        this.skin = new CircleSkin(x, y, radius, color);
    }
    update (scene) {
        // This part is responsible for changing a planet's radius and animating the transition. I have no idea if this should in fact be made here or elsewhere.
        if (this.physicsBody.radius !== this.skin.radius) {
            this.skin.scaleX = this.skin.scaleY = this.skin.radius / this.physicsBody.radius;
            this.skin.radius = this.physicsBody.radius;
            this.skin.redraw();
            createjs.Tween.get(this.skin, {override: true}).to({scaleX: 1, scaleY: 1}, 1000, createjs.Ease.elasticOut);
        }
        if (!this.physicsBody.destroyed) {
            this.skin.x = this.physicsBody.x;
            this.skin.y = this.physicsBody.y;
        } else scene.removeChild(this);
    }
}

CircleBody class:

class CircleBody extends PhysicsBody {
    constructor (x, y, radius) {
        super(x, y);
        this.radius = radius;
    }
    get area () {
        return this._area;
    }
    set area (newValue) {
        this._area = newValue;
        this._radius = Math.sqrt(newValue / Math.PI / 4);
    }
    get radius () {
        return this._radius;
    }
    set radius (newValue) {
        this._radius = newValue;
        this._area = 4 * Math.PI * newValue * newValue;
    }
    get mass () {
        return this.area * this.density;
    }
}

PhysicsBody class:

class PhysicsBody {
    constructor (x, y) {
        this.x = x;
        this.y = y;

        this.px = x;
        this.py = y;

        this.vx = 0;
        this.vy = 0;

        this.density = 1;

        this.affectedByGravity = true;
        this.static = false;
        this.collision = null;
        this.destroyed = false;
    }
    get dynamic () {
        return !this.static;
    }
    set dynamic (newValue) {
        this.static = !newValue;
    }
    get velocity () {
        return new Vector(this.vx, this.vy);
    }
    clearCollisions () {
        this.collision = null;
    }
}

CircleSkin class:

class CircleSkin extends Skin {
    constructor (x, y, radius, color) {
        super(x, y, color);
        this.radius = radius;
        this.redraw();
    }
    redraw (resolution) {
        var r = this.radius;
        this.graphics.clear();
        this.graphics.f(this.color).dc(0, 0, r);
        //this.cache(-r, -r, r * 2, r * 2, resolution || 1);
    }
}

Skin class:

class Skin extends createjs.Shape {
    constructor (x, y, color) {
        super();
        this.x = x;
        this.y = y;
        this.color = color;
    }
}

And finally, this is the class usage in engine:

for (var i = 0; i < 1024; ++i) {
    var x = Utils.getRandomInt(0, innerWidth),
        y = Utils.getRandomInt(0, innerHeight);
    var myPlanet = new Planet(x, y, 2, Utils.getRandomItem(colors));
    myPlanet.physicsBody.n = i;
    myScene.addChild(myPlanet);
}

Utils and Vector dependencies are out of scope and I think their method names are pretty clear so no declaration needed here.

Btw I like the idea of the game/demo!

It would be nice to see a case where a binary star emerges.

Back to code review:

In Class CicleBody

 get mass () {
         return this.area * this.density;
 }

the this.area - did you intentianally wanted to hit the getter of area or perhaps will be better to use the internal this._area?

PhysicsBody class:

As naming goes, the usual convention is to name functions so they reflect what's going on in the code. In that context, I would have that as a function createVelocity() to indicate that in fact you're creating an object.

 get velocity () { // better name: createVelocityVector()
     return new Vector(this.vx, this.vy);
 }

That is important for the performance conscious programmers cause every extra new object or function call might have impact on the performance.

CircleSkin class:

class CircleSkin extends Skin {
    constructor (x, y, radius, color) {
        super(x, y, color);
        this.radius = radius;
        this.redraw(); // <------?!!
    }

It is not usual to have canvas work inside of a object constructor.
Normally you'd have the draw logic invoked by the render timer.
Normally as the JS games are implemented with two continuous timers:

1) core timer - this timer runs the game logic.

2) render timer - this timer runs every ~300 ms, reads the some shared data structure, updated by the core timer, and renders it to the canvas. This way you guarantee 30 fps and your game feels smooth and responsive.

redraw (resolution) {
        var r = this.radius;
        this.graphics.clear();
        this.graphics.f(this.color).dc(0, 0, r);
        //this.cache(-r, -r, r * 2, r * 2, resolution || 1);
    }
}

From the alone is not clear what are resolution and graphics.

graphics feels like enhanced canvas.context2d, if yes, in my opinion is better to pass it as parameter. This way you can easily reuse the code with different graphic context.

My version of that method would look something along the lines:

render(graphics) {
  var r = this.radius;
  graphics.clear();
  graphics.f(this.color).dc(0, 0, r);
}

You mentioned you are using EasyJS .. I am not familiar with that library but it cannot be too different than the above mentioned structure. Perhaps the library is implementing the timers for you and is providing you with this.variable for a canvas like object.

That's okay, however, I would recommend you to open the hood and read the code of the library - you will need to understand in order to be able to write code with good performance.

I wrote a review for a similar question, have a look : https://codereview.stackexchange.com/a/135326/111698.
Hope it helps you.

-- side note --

Ha-Ha on Utils.getRandomInt(0, innerWidth)
I think everyone has implemented a version of that in his code.
Perhaps, they could add that to the Math lib :)


Check string has unique chars only Following is my code to check if string has unique chars.

I am assuming string would have only ascii chars.

#include <iostream>
#include <array>
using namespace std;

int main()
{
    array<char,255> arr;

    char *str = "ABCADEFGHIJKL";

    for (int i = 0; i < 255; i++)
        arr[i] = 0;

    int i = 0;

    while (str[i])
    {
        if (arr[str[i]] == 1)
        {
            cout << "Not unique";
            return 0;
        }

        arr[str[i]] = 1;

        i++;
    }

    cout << "Unique";

    return 0;
}

char *str = "ABCADEFGHIJKL";

This should be const char* str because string literals are usually stored in read-only memory.

for (int i = 0; i < 255; i++)
    arr[i] = 0;

You can get rid of this loop by using aggregrate initialization:

array<char,255> arr{};  // zero-initialize all elements

int i = 0;

while (str[i])

You can get rid of the index variable i by using the string pointer for iteration. Also use a for loop which is more concise:

for(; *str; ++str)
{
    if (arr[*str] == 1)
    {
        cout << "Not unique";
        return 0;
    }

    arr[*str] = 1;
}

I would also change array<char,255> to array<bool,255> which expresses intend better as you only use values of 0 and 1 anyway.

Edit:

As pointed out in this answer the OPs code (and mine aswell) is vulnerable to buffer overruns. Although OP stated that input will be ASCII only (which is in the range of 0..127), if the string is user input then it shouldn't be trusted.

One way the vulnerability could be mitigated would be a range check of the input characters. If they are out of the ASCII range, discard them, replace them by a "dummy" value or do whatever fits your situation.


Extension method that writes to a stream public static void streamReport(this Report report, Stream stream)
{
    using (var streamWriter = new StreamWriter(stream))
    {
       //some logic that calls streamWrite.Write()
       streamWriter.Flush();
    }
    //Should "return stream;" here ?
}

I am writing an extension method to an object in which I would like to transform to a CSV which is generated and returned to a client (web).

My questions are:

Should I return stream (change void to Stream) ?
Should I use the out keyword in before the Stream parameter ? Is this a common way to let the caller know the parameter will be changed ?
Should I change the method to generate a new stream (and not accept one) and trust the user to Dispose it ?
Should I pass StreamWriter instead of Stream ?

Your questions imply that you might not be quite aware of how streams or references work in C#.

You pass in a reference to a Stream
You create a StreamWriter which writes to that Stream
This will automatically make changes visible to anyone holding a reference to the same Stream.

Therefor there is no need to try and return the Stream in any way from the method to make the changes visible to the caller - you just need to write to it. This is at least how I interpret your questions.

However there is a catch in your implementation: disposing the StreamWriter will automatically close the underlying Stream which I find hugely annoying at times.  Only since .NET 4.5 there is a constructor for StreamWriter which allows you to leave the Stream open. So right now your extension method will close the Stream which I guess is not the intention. Your only option to avoid that is to use .NET 4.5 or do not wrap the StreamWriter in a using statement.


Count the one islands in the matrix What is an island?
A group of connected 1s forms an island. For example, the below matrix contains 5 islands:

{{1, 1, 0, 0, 0},
 {0, 1, 0, 0, 1},
 {1, 0, 0, 1, 1},
 {0, 0, 0, 0, 0},
 {1, 0, 1, 0, 1}}

I'm looking for code review, best practices, optimizations etc.

public final class CountIslands {

    private CountIslands() {}

    private static enum Direction {
        NW(-1, -1), N(0, -1), NE(-1, 1), E(0, 1), SE(1, 1), S(1, 0), SW(1, -1), W(-1, 0);

        int rowDelta;
        int colDelta;

        Direction(int rowDelta, int colDelta) {
            this.rowDelta = rowDelta;
            this.colDelta = colDelta;
        }

        public int getRowDelta() {
            return rowDelta;
        }

        public int getColDelta() {
            return colDelta;
        }
    }

    private static boolean isValid(int newRow, int newCol, Direction direction, int[][] m, boolean[][] visited) {
        // visited was constructed from matrix so we are sure that checking visitor for row lengh would do no harm.
        if (newRow < 0 || newRow >= visited.length) return false;
        if (newCol < 0 || newCol >= visited[0].length) return false;
        if (visited[newRow][newCol]) return false;
        return m[newRow][newCol] == 1;
    }

    private static void dfs(int row, int col, int[][] m, boolean[][] visited) {
        visited[row][col] = true;
        for (Direction direction : Direction.values()) {
            int newRow = row + direction.getRowDelta();
            int newCol = col + direction.getColDelta();
            if (isValid(newRow, newCol, direction, m, visited)) {
                dfs(newRow, newCol, m, visited);
            }
        }
    }

    /**
     * Returns the number of 1 islands.
     *
     * @param m     the input matrix
     * @return      the number of 1 islands.
     */
    public static int count(int[][] m) {
        final boolean[][] visited = new boolean[m.length][m[0].length];
        int count = 0;

        for (int row = 0; row < m.length; row++) {
            for (int col = 0; col < m[0].length; col++) {
                if (m[row][col] == 1 && !visited[row][col]) {
                    dfs(row, col, m, visited);
                    count++;
                }
            }
        }
        return count;
    }

    public static void main(String[] args) {
        int[][] m = { { 1, 1, 0, 0 }, { 0, 0, 0, 1 }, { 0, 0, 1, 1 } };

        System.out.println("Expected 2, Actual " + count(m));

        int[][] m1 = { { 1, 1, 0, 0 }, { 0, 0, 1, 1 }, { 0, 0, 1, 1 } };

        System.out.println("Expected 1, Actual " + count(m1));

    }
}

So, I looked at the problem spec, expecting to see a 3-word description o the title of the problem, and not much else, followed by 'Looking for optimizations, and confirmation that compelxity is O(n log(n) ).
Fortunately, I was disappointed ;-) Your description is improved over previous questions, and I can actually follow it without having to google-search for references and hints. It would have been improved slightly if you had spelled out carefully that diagonally touching 1 values combine to form an island... but this is a nit-pick.
Then, I did the normal-for-me task of thinking 'How would I solve this', and I thought for a moment, and decided I would:

scan the matrix cell-by-cell
if you encounter a 1, use recursion to follow all it's adjacent 1 values
mark all visited cells as 'seen'
consider that process a 'hit' for an island
scan the remaining unseen cells for the next unseen island. (go to 1).

So, I looked through your code, and, it took me a moment, but I found all of those processes in your solution.
So, you have solved this the same way I would have tackled it. The algorithm is 'good', and all that is then left, is to look at how you have implemented it...

I like the Enum for the directions. It is a good solution.
I think your variable names have improved recently. I much prefer seeing row and col instead of x and y, etc. It is more descriptive, and it helps.
count() could be countIslands() because that is what is being counted.

That's a pretty short list of nit-picky things. In all, this is a good program.
There is one suggestion I have for your recursion, and that is that there are multiple 'styles' of recursive methods. If you choose one method, and use it consistently (except for those times the other methods are better), it helps. My suggestion is that you should settle on what I call optimistic recursion.... which is the opposite case of what Wikipedia calls 'short-circuit recursion'. What I am saying, is that 'standard' recursion checks the recursive state, and, if it is valid, it does it's work, and it then calls recursion. The short-circuit system does the state-check of the new state before recursing in to that new state.
By way of example, a standard (what I call optimistic) recursion for this problem would be:
private static void dfs(int row, int col, int[][] m, boolean[][] visited) {
    // check recursion state... and expect it to have failures.
    if (row < 0 || row >= m.length || col < 0 || col >= m[0].length) {
        // invalid state, index-out-of-bounds
        return;
    }
    // OK, row/column are valid... more checks
    if (visited[row][col]) {
        // already seen this valid position.
        return;
    }
    // mark the position seen
    visited[row][col] = true;
    // if it's not an island....
    if (0 == m[row][col]) {
       return;
    }
    // OK, we are still on the island, let's optimistically search around....
    for (Direction direction : Direction.values()) {
        dfs(row + direction.getRowDelta(), col + direction.getColDelta(), m, visited);
    }
}

This optimistic approach simplifies the call structure significantly.... (and eliminates the isValid() method).
Food for thought.
Finally, I don't like calling your method dfs.... it's not a depth-first-search.... since that applies to a tree. The 'cells' in the 'landscape' are not linked in a tree structure, as one cell could be a neighbour to many other cells. A better name would be scan, or even just search.


Executing query via nested inner class query handler Background

I often see blogs/articles split out the implementation of the Query and QueryHandler classes. I had briefly asked a question on Stack Overflow about whether or not a self-executing Query class would be a violation of SOLID design principals (I deleted the question as it was feeling like the wrong place to ask for feedback). My opinion was that a Query class represents a single operation within a system, and that the separation of parameters/execution was unnecessary in most cases...

I understand that in some cases there may be legitimate reasons for doing so (i.e., using Query object AS your view model, not to my taste). One comment on the original question suggested the inability to cleanly apply decorators to a merged Query class in order to enhance query execution and although I had considered the suggestions at the time to be the responsibility of a QueryProcessor, it got me thinking: as modification of one will more often than not result in modification of the other. Perhaps nesting the handler class would be a happy middle ground as shown in the implementation below.

Implementation

/// <summary>
/// Query definition used to retrieve a specific user by their username.
/// </summary>
public sealed class GetUserByUsernameQuery : Query<User>
{
    /// <summary>
    /// The unique user username to locate.
    /// </summary>
    public Username Username { get; }

    /// <summary>
    /// Initializes a new instance of <see cref="GetUserByUsernameQuery"/>.
    /// </summary>
    /// <param name="username">The unique user username to locate.</param>
    public GetUserByUsernameQuery(Username username)
    {
        if (username == null) throw new ArgumentNullException(nameof(username));

        Username = username;
    }

    /// <summary>
    /// Handles processing of a <see cref="GetUserByUsernameQuery "/> instance.
    /// </summary>
    internal sealed class Handler : SqlQueryHandler<GetUserByUsernameQuery, User>
    {
        /// <summary>
        /// Executes the specified <paramref name="query"/> instance.
        /// </summary>
        /// <param name="query">The query instance to process.</param>
        protected override IEnumerable<User> Execute(GetUserByUsernameQuery query)
        {
            using (var connection = OpenConnection())
                return connection.Query<User>("SELECT * FROM [dbo].[GetUserByUsername](@username);", query);
        }
    }
}

NOTE: No comments regarding SELECT *. The Inline Table Function explicitly selects only the required columns for the result set and I see no reason to define the columns again in the code.

I know some people consider inner classes to be evil, however in this case my thinking is that an inner class provides a nice balance between a self-executing Query and full separation between the Query/QueryHandler classes (i.e., easy to edit within a single file).

My rational:

There will only ever be a single handler class per query.
The handler will often have to be modified if the query class were to change.
Allows for decorators etc to be applied to the QueryHandler if required.
The QueryHandler class really cannot exist without the Query class.

Are there any major downsides to going down this path? Any other suggestions on how to achieve balance between the two ends of the spectrum? I would also be interested in feedback on using inline table functions for Dapper queries. I typically place my SQL in embedded resource files or a resource file, but I thought this might be better option?

NOTE: My use case does not require that the Query/QueryHandler ever need to live in different assemblies; thus dependencies are not a concern. And worst case, I could easily refactor if required.

I would say that we should be able to execute different query handlers given the same query parameters – for testing purposes. Simple data transfer object (query) being passed to the repository interface allows us to do it in a well-known and understandable way.

The same time the following works well for me (SSAS MDX queries)…

To define MDX I use a partial class in multiple files (example):

// SalesSummary.cs
public static partial class MdxQueries
{
    public static IEnumerable<TradeSummary> SalesSummary(DateTime day, string department) =>
        new MdxQuery($@"MDX SCRIPT WITH PARAMETER={day} AND PARAMETER={department}")
            .Select(r => new TradeSummary
            {
                Product = r["Product"],
                Total = r["Total"]
            });
}

// PurchaseSummary.cs
public static partial class MdxQueries
{
    public static IEnumerable<TradeSummary> PurchaseSummary(DateTime day, string department) =>
        new MdxQuery($@"MDX SCRIPT WITH PARAMETER={day} AND PARAMETER={department}")
            .Select(r => new TradeSummary
            {
                Product = r["Product"],
                Total = r["Total"]
            });
}

Where library classes are:

public class MdxQuery : IEnumerable<MdxRow>
{
    static string ConnectionString => ConfigurationManager.ConnectionStrings["SSAS"]
        .ConnectionString;

    public MdxQuery(string mdx)
        : this(mdx, ConnectionString)
    {
    }

    public MdxQuery(string mdx, string connectionString)
    {
        _mdx = mdx;
        _connectionString = connectionString;
    }

    readonly string _mdx;
    readonly string _connectionString;

    public IEnumerator<MdxRow> GetEnumerator()
    {
        using (var connection = new AdomdConnection(_connectionString))
        {
            connection.Open();
            var command = connection.CreateCommand();
            command.CommandText = _mdx;
            var reader = command.ExecuteReader();
            while (reader.Read())
                yield return new MdxRow(reader);
        }
    }

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

    public static implicit operator MdxQuery(string mdx) =>
        new MdxQuery(mdx);
}

Where:

public struct MdxRow
{
    internal MdxRow(AdomdDataReader reader)
    {
        Values = Enumerable.Range(0, reader.FieldCount)
            .ToDictionary(i => reader.GetName(i), i => new MdxValue(reader[i]));

        ValuesByIndex = Enumerable.Range(0, reader.FieldCount)
            .ToDictionary(i => i, i => new MdxValue(reader[i]));
    }

    public MdxValue this[string column] => Values[$"[Measures].[{column}]"];
    public MdxValue this[int column] => ValuesByIndex[column];
    Dictionary<string, MdxValue> Values { get; }
    Dictionary<int, MdxValue> ValuesByIndex { get; }
}

Where:

public struct MdxValue
{
    public MdxValue(object value)
    {
        Value = value;
    }

    object Value { get; }

    public static implicit operator string(MdxValue value) =>
        Convert.ToString(value.Value);

    public static implicit operator int(MdxValue value) =>
        Convert.ToInt32(value.Value ?? 0);

    public static implicit operator double(MdxValue value) =>
        Convert.ToDouble(value.Value ?? 0);

    public static implicit operator int? (MdxValue value) =>
        value.IsNull ? (int?)null : (int)value;

    public static implicit operator double? (MdxValue value) =>
        value.IsNull ? (double?)null : (double)value;

    public static implicit operator DateTime(MdxValue value) =>
        DateTime.Parse((string)value ?? DateTime.MinValue.ToString());

    public static implicit operator DateTime? (MdxValue value) =>
        value.IsNull ? (DateTime?)null : (DateTime)value;

    public bool IsNull => Value == null;
}

All this stuff is used to implement repositories, which depend on DTO.


Refactor an XML to JSON parser class I used Nokogiri and a piece of ActiveSupport to parse an xml file from a given URL, format the data properly and return a JSON string. The script works as expected, so I'm only wondering if there are ways to make the code better from architecture/naming perspectives.

#!/usr/bin/env ruby

$:.unshift File.dirname(__FILE__)

require 'open-uri'
require 'nokogiri'
require 'active_support/core_ext'

class Scadenzario
  def initialize(url)
    @url = url
    @events = parse_events
  end

  def xml
    Nokogiri::XML::Document.parse(open(@url))
  end

  def to_json
    @events.to_json
  end

private

  def parse_events
    result = []
    xml.xpath('//item').each do |node|
      result << build_event(node)
    end

    result
  end

  def build_event(node)
    event = Hash.new
    event[:title] = node.at_xpath('title').content
    event[:url] = node.at_xpath('link').content
    event[:start] = format_event_time(node)
    event[:allDay] = false

    event
  end

  def format_event_time(node)
    Time.parse(node.at_xpath('pubDate').content).iso8601
  end
end

puts Scadenzario.new('servizi.seac.it/documenti/rss/rss_scadenzario_annuale.xml').to_json

The parse_events method can be written like this, but I think it would make the code rather unclear.

def parse_events
  xml.xpath('//item').inject([]) do |result, node|
    result << build_event(node)
  end
end

It all seems really nice, I would change few things:

Firstly, your format_event_time takes a node object as a param, which is counter-intuitive as I would expect it to accept Time object. I would rename it to 'parse_xml_date` or similar and give it text instead:

def parse_xml_time(time_string)
  Time.parse(time_string).iso8601
end

The main purpose of this change is to make your build_event method the only method specifying the exact xml locations of the required properties, so it is easy to find if those changed.

Secondly,your build_event method can be slightly simplified to (including first change):

def build_event(node)
  {
    title: node.at_xpath('title').content
    url: node.at_xpath('link').content
    start: parse_xml_time(node.at_xpath('pubDate').content)
    allDay: false
  }
end

And finally I would add a small private method get_param:

def get_xml_param(attribute, node)
  node.at_xpath(attribute).content
end

Which would simplify build_event to:

def build_event(node)
  {
    title: get_xml_param('title', node)
    url: get_xml_param('url', node)
    start: parse_xml_time(get_xml_param('pubTime', node))
    allDay: false
  }
end

UPDATE:

Actually there might be few more changes.

Your parse_events method can be written like this:

def parse_events
  xml.xpath('//item').map {|node| build_event(node) }
end

And I would consider caching an xml method:

def xml
  @xml ||= Nokogiri::XML::Document.parse(open(@url))
end


Setting variables inside if / else-if statment blocks I have written the code below and I am trying to work out if there is a more efficient way of doing it: i.e. less lines of code and quicker etc.

I am also wondering whether it is OK to declare variables inside of an if ... else if statement.

function test() {
    var x = 3;
    if (x > 5) {
        var msg = "m", state = "d";
    } else if (x < 5) {
        var msg = "a", state = "d";
    } else {
        var msg = "e", state = "n";
    }
    $('#id1').removeClass().addClass(state);
    $('#id2').html("Some text " + msg + " and more text.");
    $('#id3').attr('title', "Different text " + msg + " still different text.");
}

As background, the following code is the original code I had before refactoring/rewriting:

function test() {
  var x = 3;
  if (x > 5) {
        $('#id1').removeClass().addClass('d');
        $('#id2').html("Some text message and more text.");
        $('#id3').attr('title', "Different text message still different text.");
  } else if (x < 5) {
        $('#id1').removeClass().addClass('d');
        $('#id2').html("Some text message2 and more text.");
        $('#id3').attr('title', "Different text message2 still different text.");
  } else {
        $('#id1').removeClass().addClass('n');
        $('#id2').html("Some text message3 and more text.");
        $('#id3').attr('title', "Different text message3 still different text.");
  }
}

I don't know what the purpose of declaring your variable within your if statement is supposed to be here. For readability, I would declare those variables at the beginning of your function, like so:

function test() {
    var x = 3,
        msg,
        state;
    if (x > 5) {
        msg = "m", state = "d";
    } else if (x < 5) {
        msg = "a", state = "d";
    } else {
        msg = "e", state = "n";
    }
    $('#id1').removeClass().addClass(state);
    $('#id2').html("Some text " + msg + " and more text.");
    $('#id3').attr('title', "Different text " + msg + " still different text.");
}

That being said, I don't know a reason why you couldn't do so; it most depends on readability and the best practices you are following.


Color scale in range I need to select a color given a position within a range.

Using the "bwr" colormap shown below, I want to return a blue color for a value at the beginning of the range, a red color for a value at the end of the range, and a suitable interpolated value for values in between.

The lower and upper ends of the range can be anything. From negative to positive numbers, for example [-0.5,0.5] or [0,1] or [-0.2 to 0.6].

Here is my code:

#include <opencv2/core/core.hpp>

cv::Scalar get_colour(double v, double vmin, double vmax)
{
    double r, g, b = 0;
    double dv;

    if (v < vmin)
        v = vmin;
    if (v > vmax)
        v = vmax;
    dv = vmax - vmin;

    if (v < (vmin + 0.5 * dv)) {
        r = 2 * (v - vmin) / dv;
        g = 2 * (v - vmin) / dv;
        b = 1;
    }
    else if (v >= (vmin + 0.5 * dv)) {
        b = 1 + 2 * (vmin + 0.5 * dv - v) / dv;
        g = 1 + 2 * (vmin + 0.5 * dv - v) / dv;
        r = 1;
    }

    return cv::Scalar(b * 255, g * 255, r * 255);
}

Can you check if this function always work good, no matter what range and value I choose, and what can I write better?

The logic is mostly clear, but it's not obvious exactly how to change to one of the other scales.  There's also a case where calling the function wrongly (with vmin==vmax), you'll divide by zero.  Although that's a bug in the calling code, it may be preferable to take a defensive line on this, and just return the middle value.

I'd be inclined to split this into two three parts:

scale the input value to the range [0,1]
split into one of the sub-ranges [0,0.5) or [0.5,1]
interpolate linearly between the two colours

I'll start with the last of those.  Assuming that the input is between 0 and 1, we can interpolate quite easily:

cv::Scalar get_colour(double alpha, const cv::Scalar& c0, const cv::Scalar& c1)
{
    return (1-alpha) * c0  +  alpha * c1;
}

How do we get that value between 0 and 1?  Steps 1 and 2 look like

cv::Scalar get_colour(double v, double vmin, double vmax)
{
    // clamp value within range
    v = v <= vmin ? vmin
        : v >= vmax ? vmax
        : v;

    const double alpha = (vmax <= vmin)
        ? 0.5                   // avoid divide by zero
        : (v - vmin) / (vmax - vmin);

    static const cv::Scalar blue{ 255, 0, 0 };
    static const cv::Scalar white{ 255, 255, 255 };
    static const cv::Scalar red{ 0, 0, 255 };

    if (alpha < 0.5) {
        return get_colour(alpha * 2, blue, white);
    } else {
        return get_colour(alpha * 2 - 1, white, red);
    }
}

Here, we have used constants for the colours in both parts of the gradient, meaning that if we want to change to different colours, there's only one place that needs changing.

I hope it's apparent with this change that we could make the interpolation into an object, with the range and colours being member data.  Depending on your application, that may be easier to work with than free functions with lots of parameters.


Memoization via template This is kind of follow up of this question on stack overflow...

I wrote the following to utilize memoization for functions that take a single parameter and return a value:

#include <iostream>
#include <map>
using namespace std;

template <class T, class R, R (*Func)(T)>
R memoized(T in) {
    static std::map<T,R> memo;
    typename std::map<T,R>::iterator found = memo.find(in);
    if (found != memo.end()) { return found->second; }
    std::cout << "not found" << std::endl; // only for demo
    R res = Func(in);
    memo[in] = res;
    return res;
}

double test(double x){return x*x;}
double test2(double x){return x;}

int main() {
    std::cout << memoized<double,double,test>(1) << std::endl;
    std::cout << memoized<double,double,test>(1) << std::endl;
    std::cout << memoized<double,double,test>(1) << std::endl;
    std::cout << std::endl;
    std::cout << memoized<double,double,test2>(1) << std::endl;
    std::cout << memoized<double,double,test2>(1) << std::endl;
    std::cout << memoized<double,double,test2>(1) << std::endl;

    return 0;
}

output:

not found
1
1
1

not found
1
1
1

It is a rather strong restriction that it works only for functions taking a single parameter, but thats ok for now. Is there anything else wrong with this approach?

PS: on purpose this is using only pre C++11

I would change memoized to work with a functor type and use a traits approach to deduce the return type and the argument type.

template <typename Functor>
typename Functor::R memoized(typename Functor::T in) {
   using T = typename Functor::T;
   using R = typename Functor::R;

   static std::map<T,R> memo;
   typename std::map<T,R>::iterator found = memo.find(in);
   if (found != memo.end()) { return found->second; }
   std::cout << "not found" << std::endl; // only for demo
   R res = Functor()(in);
   memo[in] = res;
   return res;
}

struct Test1
{
   using T = double;
   using R = double;
   R operator()(T x) { return x*x; }
};

struct Test2
{
   using T = double;
   using R = double;
   R operator()(T x) { return x; }
};

My rationale for using a functor is that it simplifies user code.

int main() {
    std::cout << memoized<Test1>(1) << std::endl;
    std::cout << memoized<Test1>(1) << std::endl;
    std::cout << memoized<Test1>(1) << std::endl;
    std::cout << std::endl;
    std::cout << memoized<Test2>(1) << std::endl;
    std::cout << memoized<Test2>(1) << std::endl;
    std::cout << memoized<Test2>(1) << std::endl;

    return 0;
}


User registration and authentication using hashed passwords I'm working on a game project (with Unity) that shall handle user credentials and other data. The game connects send requests to a php script and this script send requests to the database. After spending some days learning about sql injections and password handling, I came up with this approach. I am obviously still very naive with all this, but I would like to know if it is good or not. The first one is the script for creating a new user, and the second one is for login.

<?php

$servername = "localhost";
$username = "whatever";
$password = "whatever";
$dbname = "whatever";

try {

    $db = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

    $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    $salt = bin2hex(random_bytes(strlen($_POST["providedpass"])));
    $hashpsw = hash('sha256', $_POST["providedpass"].$salt);

    $name = $_POST["usrnm"];

    $stmt = $db->prepare("INSERT INTO users (username, password, salt, level) VALUES (?,?,?,?)");

    if ($stmt->execute(array($name,$hashpsw, $salt, 1))) {

        echo "USER CREATED";
    }

    $db = null;

} catch (PDOException $e){

    echo "Error: " . $e->getMessage();
}

?>

-

<?php

$servername = "localhost";
$username = "whatever";
$password = "whatever";
$dbname = "whatever";

try {

    $db = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

    $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    $stmt = $db->prepare("SELECT * FROM users WHERE username = ?");

    $providedname= $_POST["usrnm"];

    $stmt->execute(array($providedname));

    while($row = $stmt->fetch(PDO::FETCH_OBJ)){

            echo "FOUND USER";

            $hashpsw = hash('sha256', $_POST["providedpass"].$row->salt);

            if($hashpsw == $row->password) {

                        echo "CORRECT CREDENTIALS";

            }

    }

    $db = null;

} catch (PDOException $e){

    echo "Error: " . $e->getMessage();

}
?>

Nope, it is not safe. You can refer to the following article thet explains why you shouldn't use SHA-family hashing and required to use password_hash() instead. Beside other things, it means that salt field should be removed, and password field should be varchar(255).

To soften the pill, I've got the reference code to check the hashed passsword.

There is one more issue which is connected to security: wherever an error occurs, you are echoing it right in the hands of a potential hacker, revealing a lot of sensitive information about your software, database structure, etc. You should never ever be doing things like  echo "Error: " . $e->getMessage();! For more info refer to my article on the basics of PHP error reporting

Everything else is rather good, save for peculiar code formatting and unnecessary WHILE statement. When you expect just a single row, there is no need to run a while loop. Just fetch your row right away.

Also, it does no good to duplicate the database connection code. Store it in a file and just include wherever needed.

So the refactored version would be like

db.php:

<?php

$servername = "localhost";
$username = "whatever";
$password = "whatever";
$dbname = "whatever";
$charset = 'utf8mb4';

$options = [
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
    PDO::ATTR_EMULATE_PREPARES   => false,
];
$dsn = "mysql:host=$servername;dbname=$dbname;charset=$charset";
try {
     $db = new PDO($dsn, $username, $password, $options);
} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

register.php

require 'db.php';
$hashpsw = password_hash($_POST["providedpass"],);

$stmt = $db->prepare("INSERT INTO users (username, password, level) VALUES (?,?,?)");
$stmt->execute(array($_POST["usrnm"], $hashpsw, 1));
echo "USER CREATED";

auth.php:

require 'db.php';
$stmt = $db->prepare("SELECT * FROM users WHERE username = ?");
$stmt->execute([$_POST['usrnm']]);
$user = $stmt->fetch();

if ($user && password_verify($_POST['providedpass'], $user['password']))
{
    echo "FOUND USER";
}


Linked list C implementation I'm reading a data structure and algorithm book in C.  I have implemented a generic single linked list in the C programming language, and I want to know your opinion on my implementation:

linked_list.h

#ifndef LINKED_LIST_H_INCLUDED
#define LINKED_LIST_H_INCLUDED

#define list_size(list) (list->size)

#define list_head(list) (list->head)
#define list_tail(list) (list->tail)
#define list_is_head(list, element) (element == list_head(list))
#define list_is_tail(list, element) (element == list_tail(list))

#define list_append(list, data)     list_ins_next(list, list->tail, data)
#define list_preprend(list, data)   list_ins_next(list, NULL, data)

#define list_data(element)  (element->data)
#define list_next(element)  (element->nextElmt)

typedef struct ListElmt_{
    void *data;
    struct ListElmt_ *nextElmt;
} ListElmt;

typedef struct List_{
    ListElmt *head;
    ListElmt *tail;
    size_t size;
    void (*destroy)(void *data);
    int (*cmp)(const void *fdata,const void *sdata);
}List;

List *list_init(void (*destroy)(void *data),
        int (*cmp)(const void *fdata, const void *sdata));
void list_destroy(List *list);
int list_ins_off(List *list, size_t off, void *data);
int list_ins_next(List *list, ListElmt *element, void *data);
int list_drem_next(List *list, ListElmt *element);
int list_rem_next(List *list, ListElmt *element, void ** data);
ListElmt * list_find(List *list, void *data, ListElmt *sElmt);

#endif

linked_list.c

#include <stdlib.h>

#include "linked_list.h"

List * list_init(void (*destroy)(void *data),
        int (*cmp)(const void *fdata, const void *data))
{
    List *rlist;

    if((rlist = malloc(sizeof *rlist)) != NULL) {
        rlist->head = NULL;
        rlist->tail = NULL;
        rlist->size = 0;
        rlist->destroy = destroy;
        rlist->cmp = cmp;
    }

    return rlist;
}

void list_destroy(List *list) {

    ListElmt * el;
    void *data;

    while( (el=list_head(list)) != NULL) {
        list_rem_next(list, NULL, &data);
        list->destroy(data);
    }

    free(list);
}

int list_ins_next(List *list, ListElmt *elmt, void *data) {

    ListElmt *addElmt = malloc(sizeof *addElmt);
    if(addElmt == NULL)
        return -1;

    addElmt->data = data;

    if(elmt == NULL) {
        addElmt->nextElmt = list->head;
        elmt = list->head = addElmt; // using elmt for checking list tail
    }
    else {
        addElmt->nextElmt = elmt->nextElmt;
        elmt->nextElmt = addElmt;
    }

    // updating list->tail if needed
    if(elmt == list->tail)
        list->tail = addElmt;

    list->size++;

    return 0;
}

int list_ins_off(List *list, size_t off, void *data) {
    if(off < 0 || off >= list->size)
        return list_append(list, data);

    size_t i;
    ListElmt *elmt = NULL;

    for(i = 0; i < off; ++i)
        elmt = elmt->nextElmt;

    list_ins_next(list, elmt, data);
    return 0;
}

int list_drem_next(List *list, ListElmt *element) {
    void *data;
    int ret = list_rem_next(list, element, &data);

    if(ret != 0)
        return ret;

    list->destroy(data);
    return 0;
}

int list_rem_next(List *list, ListElmt *element, void **data) {
    ListElmt *remElmt;

    // the list still not contain any element
    if(list->size == 0 || element->nextElmt == NULL)
        return -1;

    *data = element->nextElmt->data;
    remElmt = element->nextElmt;

    element->nextElmt = element->nextElmt->nextElmt;

    if(remElmt == list->tail)
        list->tail = element;

    free(remElmt);
    list->size--;

    return 0;
}

ListElmt * find(List *list, void *data, ListElmt *sElmt) {
    if(list->size == 0 || list->cmp == NULL)
        return NULL;

    sElmt = sElmt ? sElmt : list_head(list);

    while(sElmt && list->cmp(sElmt->data, data))
        sElmt = sElmt->nextElmt;

    return sElmt;
}

In order of appearance:

list_init

While destroy is definitely a property of the list, cmp is not; it is a property of a particular searc. Only find really cares of how to compare elements. It is better to pass a comparator to find directly.
list_desroy

may and should take advantage of knowing the list size. If perchance the list is corrupted, it is possible that the (el=list_head(list)) != NULL condition is never satisfied, and the loop becomes infinite. Counting iterations gives you an ability to fail gracefully.
Insertions

Naming is questionable. list_ins_next should be list_insert_after; list_ins_off should be list_insert_at. In any case, do not abbreviate insert.
Removals

The purpose of list_drem_next is quite unclear. Why to destroy data of the next element? It is much more straightforward to destroy data of this element. In any case, you must nullify the data pointer once data are destroyed.

The name itself is also questionable. list_destroy_data sounds better.
Missing functionality

There is no way to remove the head element.


UI code for save editor I'm wondering what I could do to improve the function that handles initializing the UI. I think it looks disgusting. Any ideas on what I could do besides splitting the code up into multiple functions to make it look better?

def init_ui(window, root_dir):
    window.title('ACFA Save Editor')
    tk.Label(window, text='Pilot name: ').grid(row=0)
    tk.Label(window, text='AC name: ').grid(row=1)
    tk.Label(window, text='COAM (money): ').grid(row=2)
    tk.Label(window, text='FRS Memory: ').grid(row=3)
    e_p_name = tk.Entry(window)
    e_p_name.grid(row=0, column=1)
    e_ac_name = tk.Entry(window)
    e_ac_name.grid(row=1, column=1)
    e_coam = tk.Entry(window)
    e_coam.grid(row=2, column=1)
    e_frs_memory = tk.Entry(window)
    e_frs_memory.grid(row=3, column=1)
    messagebox.showinfo('Open Save Folder', 'Ex: GAMEDATXXXX')
    directory = askdirectory(initialdir=root_dir)
    APGD = directory + '/APGD.dat'
    GAMEDAT = directory + '/' + directory.split('/')[-1] + '_CONTENT'
    p, a, c, f = read_data(APGD)
    e_p_name.insert(0, p)
    e_ac_name.insert(0, a)
    e_coam.insert(0, c)
    e_frs_memory.insert(0, f)
    b_apply = tk.Button(window, text='Apply', width=10, command=lambda:
        apply(e_p_name.get(), e_ac_name.get(), e_coam.get(),
        e_frs_memory.get(),
        APGD, GAMEDAT)).grid(row=4, column=0)
    b_quit = tk.Button(window, text='Quit', width=10,
        command=window.destroy).grid(row=4, column=1)
    return window

The rest of the code can be found here.

It depends what direction you want to go with your program, but my suggestion would be to make small factories for components you use the most often. Here it seems you are building a common UI feature: a form with several fields. Since you only have a single form you don't really benefit from abstracting the form for now, except for clarity, but you benefit from abstracting the fields as you use 4 of them.

So this code separates better generic UI components and data although it's not perfect; but UI code rarely is. Since you usually only have a handfew windows in the average app it's often OK to hardcode some values and components. It's a matter of making compromises between genericity, features, and verbosity.

(Disclaimer: I haven't tested it, please let me know if it needs fixing)

class FormField:
    def __init__(self, window, row, label, placeholder=None):
        tk.Label(window, text=label).grid(row=row)
        self.entry = tk.Entry(window)
        self.entry.grid(row, column=1)
        if placeholder:
            self.entry.insert(0, placeholder)

    def get(self):
        return self.entry.get()

class Form:
    def __init__ (self, window, row, field_keywords, callback, button_text):
        self.fields = []
        for i, field_keyword in enumerate(field_keywords):
             self.fields.append(window, row=row + i, **field_keyword)
        self.callback = callback
        # This '10' is hardcoded, but you could compute it from button_text length
        b_apply = tk.Button(window, text=button_text, width=10, command=self.submit))

    def submit(self):
        self.callback(*[field.get() for field in self.fields])

def init_ui(window, root_dir):
    # Below is code that is tricky to make any more generic and can be left as is
    # Making UI abstractions for single use components can be complexity you don't need
    window.title('ACFA Save Editor')
    messagebox.showinfo('Open Save Folder', 'Ex: GAMEDATXXXX')
    directory = askdirectory(initialdir=root_dir)
    APGD = directory + '/APGD.dat'
    GAMEDAT = directory + '/' + directory.split('/')[-1] + '_CONTENT'
    p, a, c, f = read_data(APGD)
    fields_kws = [{'label':'Pilot name: ', 'placeholder':p},
                  {'label':'AC name: ', 'placeholder':a},
                  {'label':'COAM (money): ', 'placeholder':c},
                  {'label':'FRS Memory: ', 'placeholder':f}]
    ui_form = Form(window, 1, field_kws, lambda p, a, c, f: apply(p, a, c, f, APGD, GAMEDAT), 'Apply')

    b_quit = tk.Button(window, text='Quit', width=10,
        command=window.destroy).grid(row=4, column=1)
    return window


Is this how an MVC controller supposed to be? This is the controller I created for my MVC framework, and I think I finally got it right.

Anything I can do to make the code more efficient?

Is this how a MVC controller is supposed to look like?

abstract class Controller
{
    protected $requestHandler;

    public function __construct(RequestHandler $rh, $action)
    {
        $this->requestHandler = $rh;

        $this->$action();
    }

    protected abstract function index();
}

class SignUpController extends Controller
{
    protected function index()
    {
        $headerData = array(
            'title' => 'Sign up',
            'stylesheets' => ['/stylesheets/signup.css'],
            'scripts' => array(
                '//ajax.googleapis.com/ajax/libs/jquery/1.11.0/jquery.min.js',
                '/javascripts/misc.js'
            )
        );

        View::render('Header', $headerData);

        if ($this->requestHandler->getMethod() === 'POST') {
            $v = new Validator();

            $v->addRule(1, ['required', 'max' => 35]);
            $v->addRule(2, ['required', 'between' => [5, 255], 'email']);
            $v->addRule(3, ['required', 'min' => 6]);
            $v->addRule(4, ['required']);

            $firstname    = $v->validate(1, (isset($_POST['firstname']))    ? trim($_POST['firstname'])    : false);
            $lastname     = $v->validate(1, (isset($_POST['lastname']))     ? trim($_POST['lastname'])     : false);
            $email        = $v->validate(2, (isset($_POST['email']))        ? trim($_POST['email'])        : false);
            $emailConfirm = $v->validate(2, (isset($_POST['emailConfirm'])) ? trim($_POST['emailConfirm']) : false);
            $password     = $v->validate(3, (isset($_POST['password']))     ?      $_POST['password']      : false);
            $terms        = $v->validate(4,  isset($_POST['terms']));

            if ($emailConfirm === true && trim($_POST['email']) !== trim($_POST['emailConfirm'])) {
                $emailConfirm = $v->setError('match');
            }

            if (!$v->errors()) {
                //user sign up logic here

                View::render('SignUpSuccess', ['email' => trim($_POST['email'])]);
            } else {
                $signUpData = array(
                    'firstname'    => $firstname,
                    'fnVal'        => (isset($_POST['firstname'])) ? $_POST['firstname'] : false,
                    'lastname'     => $lastname,
                    'lnVal'        => (isset($_POST['lastname'])) ? $_POST['lastname'] : false,
                    'email'        => $email,
                    'emVal'        => (isset($_POST['email'])) ? $_POST['email'] : false,
                    'emailConfirm' => $emailConfirm,
                    'emConfVal'    => (isset($_POST['emailConfirm'])) ? $_POST['emailConfirm'] : false,
                    'password'     => $password,
                    'pwVal'        => (isset($_POST['password'])) ? $_POST['password'] : false,
                    'terms'        => $terms,
                    'termsVal'     => isset($_POST['terms'])
                );

                View::render('SignUp', $signUpData);
            }
        } else {
            View::render('SignUp');
        }

        View::render('Footer');
    }
}

MVC has a strict definition. However, frameworks don't actually follow MVC (yet they call themselves such). It's like a free-for-all on framework design, approaches can differ.

Enough of that theory, let's do code.

One issue here is the constructor. If the class is subclassed, but doesn't call the parent when the constructor is overridden, then nothing will run. I suggest you separate a router logic from the controller. The order should be like

Also, I think it's best you place all request data (parameters, headers) into an object, like a $request parameter handed into the function that's called in the controller by the router (pre-cleaned and pre-formatted). Also, throw in getters to normalize the data, so that you won't be doing 'isset` all the time. Something like:

$pass = $req->post('password'); // Gets the password param from a post request, else false

Don't render by parts, like header, content, footer in the controller. It's best you invest in having a templating system. The template has the fully formed HTML, and all your render does is fill in the blanks. See mustache templates, you might like it.

So the order is:

Receive request (in CodeIgniter, that's the index.php)
index.php loads core libraries and dependencies (like routers, url parsers, db driver, XSS prevention, stuff like that).
Parse the request and simplify where needed. What you could do is

Sanitize all inputs from XSS and SQL injections.
Normalize input by placing encapsulating them in an object. Throw in a getter, so that you only need to call something like $request->post('password') to get the password from a POST request.

Then the router determines the route from the request, includes that class, creates an instance, and executes the appropriate method. Something like $classInstance->theAction($normalizedRequestData)
Now we are in the controller. Controller then talks to the model, do something here and there, gets the data, and renders the view with the data from the DB.

$this->render('VIEW_NAME',$dataFromDB);

Template parser kicks in, renders the data and generates the HTML which then gets send back to browser.

If you're a visual guy like I am, I'd prefer to look at a diagram rather than a wall of text. Luckily, CodeIgniter drew their data flow, which got me sold. It's pretty handy diagram, which can be used as basis for framework design.


Function which takes a string as input and returns the most frequent character I wrote a feature. And it works correctly. But in my opinion it is not optimal. But I have not found what standard features you can use to unleash it. Since I made it a simple sort of values. And it takes a long time to execute.

char getMaxOccuringChar(char* str)
{
    int count[ASCII_SIZE] = {0};

    int len = strlen(str);
    int max = 0;
    char result;

    for (int i = 0; i < len; i++) {
        count[str[i]]++;
        if (max < count[str[i]]) {
            max = count[str[i]];
            result = str[i];
        }
    }

    return result;
}

Here’s how I’d write this using the standard library:

#include <algorithm>
#include <unordered_map>
#include <string_view>

char most_frequent_char(std::string_view str) {
    std::unordered_map<char, int> counts;
    for (auto c : str) counts[c] += 1;
    return std::max_element(
        begin(counts), end(counts),
        [](auto a, auto b) {return a.second < b.second;}
    )->first;
}

But to be honest I’m not happy with manually iterating over the string to counts its characters. In actual code I’d probably abstract away the creation of a frequency table, which would presumably also have a max function. In Python this directly corresponds to the collections.Counter class. If we assume the existence of this utility class (left as an exercise to the reader), the implementation becomes

char most_frequent_char(std::string_view str) {
    return max(freq_table{str}).first;
}

Incidentally, in statistics this property is known as the “mode” of a distribution.