Surely there's a better way to write this statement? I have the following, quite ugly statement - is there a better way to write something of this nature?

if (!item["Created By"].ToString().Contains("System Account") &&
    !item.Url.Contains("_catalogs") &&
    !item.Url.Contains("Style Library") &&
    !item.Url.Contains("Cache Profiles") &&
    !item.ContentType.ToString().Contains("Theme Gallery") &&
    !item.ContentType.ToString().Contains("Converted Forms") &&
    !item.ContentType.ToString().Contains("Page Output Cache") &&
    !item.ContentType.ToString().Contains("Master Page"))
{
    query.RecordItems(item.ID.ToString(), item.ContentType.Name,
                        item.DisplayName, item.Name,
                        "", item.Url, item["Created By"].ToString(),
                        "",
                        Convert.ToDateTime(item["Modified"]),
                        item["Modified By"].ToString(),
                        Convert.ToDateTime(item["Created"]),
                        item["Created By"].ToString());
}

For a frame of reference, here's the proceeding code:

using (var web = activeSiteCollection.OpenWeb())
{
    foreach (SPList list in web.Lists)
    {
        query.RecordLists(list.ID.ToString(), list.Author.ToString(), list.Title,
                            list.DefaultViewUrl,
                            list.ParentWeb.Title, list.ParentWebUrl, list.ItemCount,
                            list.LastItemModifiedDate, list.LastItemDeletedDate);

        // check if files exist in the document library, if they don't break out to next level
        if (list.ItemCount <= 0) continue;

        for (int i = 0; i < list.Items.Count; i++)
        {
            try
            {
                SPListItem item = list.Items[i];

                if (!item["Created By"].ToString().Contains("System Account") &&
                    !item.Url.Contains("_catalogs") &&
                    !item.Url.Contains("Style Library") &&
                    !item.Url.Contains("Cache Profiles") &&
                    !item.ContentType.ToString().Contains("Theme Gallery") &&
                    !item.ContentType.ToString().Contains("Converted Forms") &&
                    !item.ContentType.ToString().Contains("Page Output Cache") &&
                    !item.ContentType.ToString().Contains("Master Page"))
                {
                    query.RecordItems(item.ID.ToString(), item.ContentType.Name,
                                        item.DisplayName, item.Name,
                                        "", item.Url, item["Created By"].ToString(),
                                        "",
                                        Convert.ToDateTime(item["Modified"]),
                                        item["Modified By"].ToString(),
                                        Convert.ToDateTime(item["Created"]),
                                        item["Created By"].ToString());
                }
            }
            catch (NullReferenceException)
            {
                SPListItem item = list.Items[i];

                Logger.Error(
                    "[{0}] Filed moving on file {1} as not all content was present",
                    item.Name);
            }

        }

As well as some standard indenting, a quick thought could be reversing the item being compared and using some temporary variables for readability.

EDIT:
Ok, so svick pointed out that the original code was fundamentally flawed.  So how about creating an extension method on string to do the Contains for us.  Something like:

// Method would do well with some unit tests ....
public static bool ContainsAny(this string source, string[] findStr)
{
    return findStr.Any(p => source.Contains(p));
}

Then used like

var contentTypes = new [] { "Theme Gallery", "Converted Forms", "Page Output Cache", "Master Page" };
var urls = new[] { "_catalogs", "Style Library", "Cache Profiles" };
var accounts = new [] { "System Account" };

var contentType = item.ContentType.ToString();
var createdBy = item["Created By"].ToString();

if (!createdBy.ContainsAny(accounts) &&
    !item.Url.ContainsAny(urls) &&
    !contentType.ContainsAny(contentTypes))
{
    query.RecordItems(
            item.ID.ToString(),
            item.ContentType.Name,
            item.DisplayName,
            item.Name,
            "",
            item.Url,
            createdBy,
            "",
            Convert.ToDateTime(item["Modified"]),
            item["Modified By"].ToString(),
            Convert.ToDateTime(item["Created"]),
            createdBy);
}

Also, do you need to catch the exception in this method?  Could you not just handle that situation in an if statement as it looks like you are letting the exception handle the case where one of the values is null when a simple null check would suffice??


Creating a list containing the rank of the elements in the original list I don't really know how to explain what I'm looking for in a way that makes sense, but here goes:

Say I have a list
$$L=(4,7,9,10,6,11,3)$$
What I want to produce is a corresponding list
$$ K = (1,3,4,5,2,6,0)$$

where element K[i] has the value of the 'rank' of the element for the corresponding location in L. So the higher the number the higher the rank, starting from rank 0 for the smallest number in L

The code I have written for this is:

x = [4,7,9,10,6,11,3]
index = [0]*len(x)

for i in range(len(x)):
    index[x.index(min(x))] = i
    x[x.index(min(x))] = max(x)+1

and it works, but I just feel it looks horrible, and was wondering if there might exist a more aesthetic way.

I believe your solution is \$O(n^2)\$, and we can reduce that to \$O(n \log n)\$. I'm not well-versed in python, but this is the general idea:

The output is a permutation of the indices \$0, 1, \ldots, n - 1\$, where \$n\$ is the length of the input.
We sort the indices based on the element at the specified index. In your example, we get \$6, 0, 4, 1, 2, 3, 5\$, i.e. the \$6\$th element (\$3\$) is smallest, so \$6\$ is first.
Seeing that \$6\$ is at index \$0\$, we know that the \$6\$th element of the output is \$0\$, and so on. This step is easier to explain in code than in words, sorry about that.

In code,

indices = list(range(len(input)))
indices.sort(key=lambda x: input[x])
output = [0] * len(indices)
for i, x in enumerate(indices):
    output[x] = i

Or the more terse

output = [0] * len(input)
for i, x in enumerate(sorted(range(len(input)), key=lambda y: input[y])):
    output[x] = i

I used the timeit module to compare the running times of this version and the original for different input sizes. The functions were each called 1,000 times on randomly shuffled input of size \$n\$. Here are some of the results

   n     this version (s)  original version (s)
  10                 0.02                  0.04
 100                 0.17                  1.40
1000                 1.81                133.35

This is asymptotically optimal, since if it were not, we would have a sub-linearithmic comparison sort:

sorted = [0] * len(input)
for i, x in enumerate(output):
    sorted[x] = input[i]


Asynchronous TCP server After some investigation, I implemented an asynchronous TCP server as per the following example. During my investigation I was unable to find an example that cleanly shuts down the server; after some experimenting I was able to furnish my code with this functionality. I would appreciate a review of the service code for whether I might be doing something stupid/dangerous. (Please note that I have stripped out parameter validation, etc. in order to improve readability).

public class AsyncTcpServer : IDisposable
{
    public class DataReceivedEventArgs : EventArgs
    {
        public NetworkStream Stream { get; private set; }

        public DataReceivedEventArgs(NetworkStream stream)
        {
            Stream = stream;
        }
    }

    public event EventHandler<DataReceivedEventArgs> OnDataReceived;

    public AsyncTcpServer(IPAddress address, int port)
    {
        _listener = new TcpListener(address, port);
    }

    public void Start()
    {
        _listener.Start();
        _isListening = true;
        WaitForClientConnection();
    }

    public void Stop()
    {
        _isListening = false;
        _listener.Stop();
    }

    public void Dispose()
    {
        Stop();
    }

    private void WaitForClientConnection()
    {
        _listener.BeginAcceptTcpClient(HandleClientConnection, _listener);
    }

    private void HandleClientConnection(IAsyncResult result)
    {
        if (!_isListening)
        {
            return;
        }

        var server = result.AsyncState as TcpListener;
        var client = _listener.EndAcceptTcpClient(result);

        WaitForClientConnection();

        OnDataReceived?.Invoke(this, new DataReceivedEventArgs(client.GetStream()));
    }

    private readonly TcpListener _listener;
    private volatile bool _isListening = false;
}

The following test verifies the asynchronous nature of the service (test completes under 10 seconds for 5x client connections blocking for 5 seconds each).

[TestMethod]
public void TestSendReceiveDataFromMultipleClientsConcurrently()
{
    using (var server = new AsyncTcpServer(IPAddress.Any, 54001))
    {
        server.OnDataReceived += (sender, e) =>
        {
            var bytesRead = 0;
            do
            {
                // Read buffer, discarding data
                bytesRead = e.Stream.Read(new byte[1024], 0, 1024);
            }
            while (bytesRead > 0 && e.Stream.DataAvailable);

            // Simulate long running task
            Thread.Sleep(5000);

            var response = Encoding.ASCII.GetBytes("Who's there?");
            e.Stream.Write(response, 0, response.Length);
        };

        server.Start();
        var tasks = new List<Task>();

        for (var i = 0; i < 5; ++i)
        {
            tasks.Add(Task.Run(() =>
            {
                var response = new byte[1024];

                using (var client = new TcpClient())
                {
                    client.Connect("127.0.0.1", 54001);

                    using (var stream = client.GetStream())
                    {
                        var request = Encoding.ASCII.GetBytes("Knock, knock...");
                        stream.Write(request, 0, request.Length);
                        stream.Read(response, 0, response.Length);

                        Assert.AreEqual("Who's there?", Encoding.ASCII.GetString(response).TrimEnd('\0'));
                    }
                }
            }));
        }

        Assert.IsTrue(Task.WaitAll(tasks.ToArray(), 10000));
    }
}

This is an ugly old pattern. Why don't just try the newer async/await? You already use Task in your tests anyway.

In order to implement it the awaitable way you just need to use a different API, in this case AcceptTcpClientAsync and build everything on top of it. With the CancellationToken you can now better control the server.

public class TcpServer : IDisposable
{
    private readonly TcpListener _listener;
    private CancellationTokenSource _tokenSource;
    private bool _listening;
    private CancellationToken _token;

    public event EventHandler<DataReceivedEventArgs> OnDataReceived;

    public TcpServer(IPAddress address, int port)
    {
        _listener = new TcpListener(address, port);
    }

    public bool Listening => _listening;

    public async Task StartAsync(CancellationToken? token = null)
    {
        _tokenSource = CancellationTokenSource.CreateLinkedTokenSource(token ?? new CancellationToken());
        _token = _tokenSource.Token;
        _listener.Start();
        _listening = true;

        try
        {
            while (!_token.IsCancellationRequested)
            {
                await Task.Run(async () =>
                {
                    var tcpClientTask = _listener.AcceptTcpClientAsync();
                    var result = await tcpClientTask;
                    OnDataReceived?.Invoke(this, new DataReceivedEventArgs(result.GetStream()));
                }, _token);
            }
        }
        finally
        {
            _listener.Stop();
            _listening = false;
        }
    }

    public void Stop()
    {
        _tokenSource?.Cancel();
    }

    public void Dispose()
    {
        Stop();
    }
}

EventArgs as a nested class, no no no ;-)

Test

The old test won't work anymore as now you need to make everything async/await.

the event handler now becomes async (sender, e)
Thread.Sleep is now await Task.Delay(3000);
the server needs to run async so you need a Task.Run(async () => {..}
at the end you wait for the server with await serverTask;

I used the console for output as I run this in LINQPad and added the thread-id so to see where it runs.

using (var server = new TcpServer(IPAddress.Any, 54001))
{
    server.OnDataReceived += async (sender, e) =>
    {
        var bytesRead = 0;
        do
        {
            // Read buffer, discarding data
            bytesRead = e.Stream.Read(new byte[1024], 0, 1024);
        }
        while (bytesRead > 0 && e.Stream.DataAvailable);

        // Simulate long running task
        Console.WriteLine($"Doing some heavy response processing now. [{Thread.CurrentThread.ManagedThreadId}]");
        await Task.Delay(3000);
        Console.WriteLine($"Finished processing. [{Thread.CurrentThread.ManagedThreadId}]");

        var response = Encoding.ASCII.GetBytes("Who's there?");
        e.Stream.Write(response, 0, response.Length);
    };

    Task.Run(async () =>
    {
        var serverTask = server.StartAsync();

        var tasks = new List<Task>();

        for (var i = 0; i < 5; ++i)
        {
            tasks.Add(Task.Run(() =>
            {
                var response = new byte[1024];

                using (var client = new TcpClient())
                {
                    client.Connect("127.0.0.1", 54001);

                    using (var stream = client.GetStream())
                    {
                        var request = Encoding.ASCII.GetBytes("Knock, knock...");
                        stream.Write(request, 0, request.Length);
                        stream.Read(response, 0, response.Length);

                        //Assert.AreEqual("Who's there?", Encoding.ASCII.GetString(response).TrimEnd('\0'));
                        Console.WriteLine($"Who's there? Echo: " + Encoding.ASCII.GetString(response).TrimEnd('\0') + $" [{Thread.CurrentThread.ManagedThreadId}]");
                    }
                }
            }));
        }

        //Assert.IsTrue(Task.WaitAll(tasks.ToArray(), 10000));
        Console.WriteLine($"IsTrue: " + Task.WaitAll(tasks.ToArray(), 10000));

        await serverTask;
    });

}


Representing a Java Swing containment hierarchy in Haskell A Java desktop application exposes the class/containment hierarchy of its Swing GUI through RPC. I'm building a Haskell client that connects to the application. The objective is to be able to perform queries like "find all the buttons of width less than w, located within screen rectangle r" over a data structure on the Haskell side.

I'm not sure about how to best represent the structure, especially regarding inheritance/subtyping. I have tried two approaches. The first one:

data Component = Component
    {
        componentId::Int,
        pos::(Int,Int),
        dim::(Int,Int),
        componentType::ComponentType,
        children::[Component]
    }

data ComponentType =
     ButtonComponent Button
     -- other component types here
    |OtherComponent

data Button = Button
    {
        buttonText::Text
        -- other button attributes here
    }

I'm also considering a second solution using existential types, together with Typeable to implement "downcasting":

data Component = Component
    {
        componentId::Int,
        pos::(Int,Int),
        dim::(Int,Int),
        children::[AnyComponent]
    } deriving Typeable

data AnyComponent = forall a . ( Typeable a, HasComponent a ) => AnyComponent a

class HasComponent a where
    base :: a -> Component
    downcast :: Typeable b => a -> Maybe b

instance HasComponent Component where
    base = id
    downcast = cast      -- cast function is from Data.Typeable

instance HasComponent AnyComponent where
    base (AnyComponent a) = base a
    downcast (AnyComponent a) = cast a

data Button = Button
    {
       buttonComponent:: Component,   -- pointer to "superclass" info
       buttonText::Text
       -- other button attributes here
    } deriving Typeable

instance HasComponent Button where
    base = buttonComponent
    downcast = cast

What solution is more idiomatic Haskell? Are there better solutions?

EDIT: Faithfully representing OO semantics in Haskell is currently impossible. In particular, method resolution can be simulated (but not easily, and not quite faithfully) via type classes and instances. This paper is very informative about current best practice, and possible future evolution.

Here is one possibility, which is similar to your first solution:

data ComponentClass a
    = ComponentClass {
        ...
        , comSubclass :: a
        ...
        }
type Component = ComponentClass ()

data ButtonClass a
    = ButtonClass {
        , buttonText :: Text
        , btSubclass :: a
        ...
        }
type Button = ComponentClass (ButtonClass ())

This is what I would do if I thought a simple hierachy fit my problem
domain well (but not necessarily to mirror the OO in another language).
However, this strategy does not work well if you need to track deep
hierarchies, or multiple superclasses (such as in Python).

Another disadvantage is that you cannot deconstruct an arbitrary
ComponentClass a into its subtype a. You can only do this if you
know the type of a in advance. However, you could use dynamic typing
to fix this issue, such as via Typable.

Note that you could also represent the hierachy in the up direction as well:

data ButtonClass a
    = ButtonClass {
          btText :: Text
        , btSuperClass :: a
        }
type Button = ButtonClass ComponentClass

You could then build each object "down the class hierarchy" until you
discovered the most concrete type to wrap it up in.

Also note that Gtk2Hs uses a
typeclassing strategy similar to your second one to represent the class
hierachy of Gtk. Perhaps a look at its source code would be edicational,
or even just browsing its documentation on Hackage.

Finally, the Exception hierachy implemented in GHC is quite interesting, though I don't think it would help you as much as either of the above---it's quite flat by comparison.


Implementation of the Stack data structure using a linked list in C++ I'm currently working on implementing overloaded operators for simplifying pop() and push() operations. Check out the //how can avoid this? comments. I've had some troubles understanding how to return a certain value without causing memory leaks due to unassigned free space: so far the only dirty and inelegant idea I've had was to create a temporary empty value of T type, so in case the top pointer contained nullptr I could return this empty value.

But how can I avoid this? Or is this actually necessary in a template type for a function with return statements?

#include <iostream>
#include <memory>
#include <string>
#include <vector>

template <typename V> using ptr = std::shared_ptr<V>;

template <typename T>
class Stack
{
private:

    class Node
    {
    private:

        T data;
        ptr<Node> next;

    public:
        Node(T _data):data{_data}, next{} {}
        T getData(){
            return data;
        }

        ptr<Node>& getNext(){
            return this->next;
        }

    };

    ptr<Node> top;

public:

    Stack() : top{} {}
    Stack(const std::shared_ptr<std::vector<T>>& vec) : top{} {
        for (auto vElement : *vec){
            push(vElement);
        }
    }

    void push(T _data){
        auto _tmp = std::make_shared<Node>(_data);
        if(top!= nullptr){
            _tmp->getNext() = top;
        }
        top = _tmp;
    }

    T pop(){

        T forSafety{}; //how can I avoid this?
        if(isEmpty()) return forSafety;
        else {
            auto _tmp = top;
            top = top->getNext();
            return _tmp->getData();
        }
    }

    T peek(){

        T forSafety{}; //how can I avoid this?
        if(!isEmpty()){
            return top->getData();
        } else return forSafety;
    }

    bool isEmpty(){
        if(top==nullptr) return true;
        else return false;
    }

    void display(){
        auto _tmp = top;
        while(_tmp!=nullptr){
            std::cout << _tmp->getData() << std::endl;
            _tmp = _tmp->getNext();
        }
    }
};

int main(int argc, char** argv)
{
    std::shared_ptr<std::vector<std::string>> names{new std::vector<std::string>{"Julia","Bob","Angela","fSociety"}};
    auto s = std::make_shared<Stack<std::string>>(names);
    s->display();
    std::cout << s->peek() << std::endl;

    while(!s->isEmpty()){
        std::cout << "Popping -> " << s->pop() << std::endl;
    }

    s->display();
    s->peek();

    return 0;
}

template <typename V> using ptr = std::shared_ptr<V>;

Class specific details like aliases should be kept in the class or wrapped in a namespace to prevent global namespace pollution.

Why std::shared_ptr? std::shared_ptr is typically reserved for shared-ownership resource management and that has a cost associated to it.  They are twice the size of a raw pointer as the std::shared_ptr object stores both the raw pointer to the resource and a raw pointer to the reference count.  Reference counting must also be atomic to handle multiple reader/writers, resulting in slower increments and decrements.  If you want to use a smart pointer, C++ provides std::unique_ptr for exclusive-resource management and should typically be the default smart pointer unless you need std::shared_ptr.

    class Node {
    // ...
    }

Node contains some payload data.  We can read data through getData().  How do we write to data?  Why return a copy of data?  Since your Node class is an internal implementation detail that isn't used outside of your Stack, consider making Node an aggregate.

    struct Node {
        T data;
        ptr<Node> next;
    }

    Stack() : top{} {}

If your constructors are initializing data members with constant initialization values, explicitly initialize your data members using in-class initializers.

    ptr<Node> top{};        // default initializes top to nullptr
public:
    Stack() = default;      // don't suppress compiler generated def-ctor
    Stack(const std::shared_ptr<std::vector<T>>& vec) {
        // ...
    }

    ptr<Node> top;
public:
    // omitted destructor

On destruction, smart pointers are great because they clean themselves up.  Unfortunately, recursive destructions are not optimized.  If you had a deep linear hierarchy of smart pointers, there is going to come a point where you overflow the stack due to too many destruction calls.  If you used a smart pointer for a stack, you still need to implement a destructor which requires the rule of five.  The rule of five says that if you provide any of the copy assignment/constructor, move assignment/constructor, or destructor special member functions, you must provide the others.

T pop() {
    T forSafety{}; //how can I avoid this?

The problem with returning from pop is what do you return?  Returning by value is inefficient for expensive types.  Your other option is to return by reference, but pop is a destructive operation that would result in returning a dangling pointer.  A third option is to separate the concerns.  Make pop a non-value returning function that simply removes the top element.  peek already provides access to the top element.

T peek(){
    T forSafety{}; //how can I avoid this?
    if(!isEmpty()){
        return top->getData();
    } else return forSafety;
}

Member functions that inspect but do not mutate the object should be marked with a const suffix.  Mutations can happen internally through direct access and externally through references, so marking functions as const (read-only) prevents mutation.  Strive for const-correctness.

Return by value is inefficient and doesn't allow for external mutation of the value.  As stated in the review of pop, you don't want to return a dangling pointer.  The simplest solution is to delegate construction of a static variable to be referenced.

   T& peek() {
        if (top) {
            return top->getData();
        }
        return failed_range_check();
    }

    const T& peek() const {
        if (top) {
            return top->getData();
        }
        return failed_range_check();
    }

private:
    T& failed_range_check() {
        static T placeholder{};
        return placeholder;
    }

It works but it's still terrible.  The callee has no idea an error occurred and the function returns a value that looks to be valid.  A better solution is to use one of the many error handling techniques that already exist.  The simplest is to throw an exception.

    T& peek() {
        return do_peek();
    }

    const T& peek() const {
        return do_peek();
    }

private:
    T& do_peek() {
        if (!top) {
            throw std::range_error("Stack<>::peek(): Cannot peek empty stack.");
        }
        return top->getData();
    }

Other options include using typesafe std::error_condition with containers like Alexandrescu's Expected<T>, variant<T>, or classic struct/std::tuple result types.  If you don't care about the error type, you could use an optional<T>.

    if(top==nullptr) return true;
    else return false;

You don't have to explicitly compare to nullptr as the bool conversion for nullptr results in false.

  4.12 Boolean conversions [conv.bool]

  \$^1\$ A prvalue of arithmetic, unscoped enumeration, pointer, or pointer to member type can be converted to a prvalue of type bool. A zero value, null pointer value, or null member pointer value is converted to false; any other value is converted to true. For direct-initialization (8.5), a prvalue of type std::nullptr_t can be converted to a prvalue of type bool; the resulting value is false.

Continue to strive for const-correctness.

    bool isEmpty() const {
        return !top;
    }

void display(){
    auto _tmp = top;
    while(_tmp!=nullptr){
        std::cout << _tmp->getData() << std::endl;
        _tmp = _tmp->getNext();
    }
}

Stacks only provide access to the last element inserted, so a function that displays the content of the stack is unnecessary.  If you need to verify data is being entered and stored correctly, use testing and a debugger.

Be aware that <iostream> statically initializes the standard streams (std::cin, std::cout, std::cerr).  Rather than directly including <iostream> in headers and incurring the initialization costs on users.  Consider including <iosfwd> and pass streams through referencing parameters.

Be aware that

std::cout << std::endl;

is equivalent to

std::cout << '\n' << std::flush;

The call to std::flush may have an effect on stream efficiency.  Rather than researching which streams behave poorly with flushing, explicitly state your intent in code that you want a new line.

std::cout << '\n';                 // Explicit EOL char.
std::cout << std::flush;           // Explicit flush the stream.
std::cout << '\n' << std::flush;   // Explicit both

When there is an obvious loop variable, prefer for loops over while loops.

void display(std::ostream& out) {
    for( auto& curr = top; curr; curr = curr->next) {
        out << curr->getData() << '\n';
    }
}

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

main is a special function.  The standard allows main to take 2 forms.

  3.6.1 Main function [basic.start.main]

  An implementation shall allow both

  \$^{(2.1)}\$ — a function of () returning int and

  \$^{(2.2)}\$ — a function of (int, pointer to pointer to char) returning int

If you do not plan to use the parameters, explicitly convey your intent by omitting them.

int main() {

std::shared_ptr<std::vector<std::string>> names{new std::vector<std::string>{"Julia","Bob","Angela","fSociety"}};
auto s = std::make_shared<Stack<std::string>>(names);

Don't use smart pointers or dynamic allocation when you do not need it.

auto names = std::vector<std::string>{"Julia","Bob","Angela","fSociety"};
auto s = Stack<std::string>(names);        // conv ctor needs to be updated
s.display();
// ...

  How is this designed so far?

You are missing some basic methods.

std::size_t size() const;
T& emplace(Args...); (requires Node emplacement support)
void push(T&&);
void swap(T&);

As for design, applying the adaptor pattern would have been a much better choice.  A stack provides specialized access (last-in, first-out) that can be mapped to an underlying container.  push does an insert on one of the ends. top provides direct access to the same end.  pop removes the element at that end.  std::stack is designed this way.  The underlying container could be any sequence container that provides the required interface (push_back, pop_back, back).

The best part of composing with an existing container class is that it naturally provides the rule of zero.  Your adaptor class has nothing to do with ownership as it simply maps its interface on the underlying container.


String Calculator kata in Haskell In order to practice some Haskell, I tried this kata, where the task is to add some numeric inputs that are delimited by commas or newlines.

I have the below solution in Haskell which passes my tests, but could anyone offer any improvements? It feels a bit messy to me so feedback on how to make it more functional (my experience is pretty much all OO until now) would be great!

Known failings:

This does not try to be smart about invalid syntax so the pattern matching is not exhaustive - would the best solution here be to use a try/either type? This seemed like it would add quite a bit of noise at this stage so for now I have left this. It also does not fail on negative numbers for the same reason.

Code:

module Calculator where

import Data.List.Split

defaultDelimiters = [",", "\n"]

readInt :: String -> Int
readInt = read

splitAtDelims :: ([String], String) -> [String]
splitAtDelims (delims, body) = foldr splitEachOnSubStr [body] delims
    where
        splitOnSubStr = split . dropBlanks . dropDelims . onSublist
        splitEachOnSubStr = concatMap . splitOnSubStr

parseDelim :: String -> String -> (String, String)
parseDelim (']':xs) delim = (delim, xs)
parseDelim (x:xs) delim = parseDelim xs (delim++[x])

parseCustomDelimiters :: [String] -> String -> ([String], String)
parseCustomDelimiters [] (delim:'\n':body) = ([[delim]], body)
parseCustomDelimiters delims ('\n':rest)   = (delims, rest)
parseCustomDelimiters delims ('[':rest)    = parseCustomDelimiters newDelims remainingText
    where
        parsed = parseDelim rest ""
        newDelims = delims ++ [fst parsed]
        remainingText = snd parsed

parse :: String -> ([String], String)
parse ('/':'/':text)    = parseCustomDelimiters [] text
parse body              = (defaultDelimiters, body)

add :: String -> Int
add = sum . filter (< 1000) . map readInt . splitAtDelims . parse

I'd suggest to accompany the code with comments. Not just for the purpose of review, but also for your own. After a few weeks it's hard to remember all the details, and what seems to be clear now will be hard to read later. Especially in this case when all arguments are of String or [String], it'd be very helpful to document them (see Function arguments).

Some more thoughts: The code isn't that easy to read. One reason for that I see
is that your functions often entangle together several goals. Extracting common
functionality into smaller helper functions would help.

Specific comments:

parse :: String -> ([String], String)
parse ('/':'/':text)    = parseCustomDelimiters [] text
parse body              = (defaultDelimiters, body)

The name is basically meaningless - what does it parse? Why do you call the
string text in one place and body in another?

parseDelim :: String -> String -> (String, String)
parseDelim (']':xs) delim = (delim, xs)
parseDelim (x:xs) delim = parseDelim xs (delim++[x])

This basically is "read until a specific character and return both parts". And
the second argument is redundant, it's only an implementation detail, and
obscures how the function should be used.  I'd create a helper function like
readUntil :: Char -> String -> (String, String), and implement it using
break.
And then just parseDelim = readUntil ']'.

splitAtDelims :: ([String], String) -> [String]
splitAtDelims (delims, body) = foldr splitEachOnSubStr [body] delims
    where
        splitOnSubStr = split . dropBlanks . dropDelims . onSublist
        splitEachOnSubStr = concatMap . splitOnSubStr

The type is quite uncommon. I'd rather use

splitAtDelims :: [String] -> String -> [String]

and then use uncurry splitAtDelims when needed.

parseCustomDelimiters :: [String] -> String -> ([String], String)
parseCustomDelimiters [] (delim:'\n':body) = ([[delim]], body)
parseCustomDelimiters delims ('\n':rest)   = (delims, rest)
parseCustomDelimiters delims ('[':rest)    = parseCustomDelimiters newDelims remainingText
    where
        parsed = parseDelim rest ""
        newDelims = delims ++ [fst parsed]
        remainingText = snd parsed

Again, the first argument is just an implementation detail and makes the
function both harder to read and harder to use. Rather you should define it for
example as:

parseCustomDelimiters :: String -> ([String], String)
parseCustomDelimiters = loop []
  where
    loop =  -- your original definition

Also you use both names body and rest for the same purpose, which is very
confusing. You could also use readUntil from before to simplify this, as
apparently new-line terminates processing the input. So you could just split the
body on '\n' and parse the delimiters from the first part.

Notice that when parsing delimiters, you match on the starting marker "//" in
a different function than the ending marker "\n". It's much easier to read
when such related parts are together, in one function. So you could write
another helper function, like

readBetween :: String -> String -> (String, String)

where the first and the second argument are the starting and ending makers. Then
you could write readBetween "//" "\n" text and parse the returned content.

I'd strongly encourage you to explore ReadP, which is available in base, or some other parser (like Parsec AttoParsec). The code will be then much more readable!

You can also go without such a full-featured parser, and instead implement your own, tiny one. Already you can see in your types that the core structure of a parsing function (that doesn't handle errors or multiple possibilities) is

String -> (a, String)

That is, we parse some expected value of type a and return the rest of the string. A parser is then just an encapsulation of such an idea, like newtype Parser a = Parser (String -> (a, String)) and common operations on them.

As mentioned in the comments, you didn't specify any tests, even though this was a TDD exercise. Including them would be definitely helpful. And a great way to learn about property-based testing! Testing is also extremely valuable for refactorings, to make sure you don't introduce a regression.

I hope this helps!


Optimising multiple string splits and concatenations I'm writing a program in Unity and am using a TextMeshProUGUI object (named history) to hold a list of messages to act as a terminal/console.

The problem I am having is that I am printing up to hundreds of lines a second, which can be detrimental to performance when I'm doing many string splits and then updating the text variable of the TextMeshProUGUI object, (which in turn gets rendered) every single frame.

My initial implementation looked like this:

private const int MAX_HISTORY = 64;  // the maximum number of lines to store
public TextMeshProUGUI history;      // value is set in Unity editor
private int inputs = 0;              // the size of the history object

public void Print(params string[] msgs)
{
    // add all messages to the history
    foreach (string msg in msgs)
    {
        if (msg == null)
            continue;
        inputs++;
        history.text += msg + '\n';
    }
    // and now reduce the size
    if (inputs > MAX_HISTORY)
    {
        string[] lines = history.text.Split('\n');
        history.text = "";
        for (int i = inputs - MAX_HISTORY; i < lines.Length; i++)
        {
            history.text += lines[i] + '\n';
        }
        inputs = MAX_HISTORY;
    }
}

and I have managed to reduce it to this:

public void Print(params string[] msgs)
{
    foreach (string msg in msgs)
    {
        if (msg == null)
            continue;
        inputs++;
        history.text += msg + '\n';
    }
    if (inputs > MAX_HISTORY)
    {
        history.text =
            history.text.Split(new char[] { '\n' }, inputs - MAX_HISTORY)[inputs - MAX_HISTORY - 1];
        inputs = MAX_HISTORY;
    }
}

But I am still concerned with the line in the if-statement with the large split function. I would appreciate it if somebody who has experience with C# and string manipulation could review this and tell me if my optimisation is (a) correct and (b) good.

Edit:

Changing the value of TextMeshProUGUI.text causes a layout and vertex recalculation which has poor performance so I've also added a StringBuilder to handle the concatenation and splits before setting the text value.

public void Print(params string[] msgs)
{
    StringBuilder text = new StringBuilder(history.text);
    foreach (string msg in msgs)
    {
        if (msg == null)
            continue;
        inputs++;
        text.AppendLine(msg);
    }
    while (inputs > MAX_HISTORY)
    {
        text.Remove(0, Convert.ToString(text).Split('\n').FirstOrDefault().Length + 1);
        inputs = MAX_HISTORY;
    }
    history.text = text.ToString();
}

I would use generic list to store messages history and Linq extension methods. If I understood correctly I would append new messages and crop to predefined maximum size. Could you test this solution if it has sufficient performance?

private List<string> _history = new List<string>();

public void Print(params string[] msgs)
{
    _history.AddRange(msgs.Where(m => !string.IsNullOrWhiteSpace(m)));
    _history = _history.Skip(Math.Max(0, _history.Count - MAX_HISTORY)).ToList();
    history.text = string.Join(Environment.NewLine, _history);
}

If you need messages ordered from newest to latest, you can add Reverse() method:

_history = _history.Skip(Math.Max(0, _history.Count - MAX_HISTORY)).Reverse().ToList();


Testing file IO errors in python Below is a simple class with two methods: the first calls open() to read in a file. The second calls the first method and does error handling.

There is also a corresponding test class that tests the first methods returns data as expected and the other methods test the error handling of potential exceptions thrown by open.

Firstly, I would like to know if these tests are written in the appropriate manner using python3, mocking and patching as I am new to the language and especially to testing.

I also have a query to why mocking an exception to be thrown cannot be handled like the rest of the tests (see last test method)

fileMethods.py

# -*- coding: utf-8 -*-

def readconfig(myfile):
    try:
        filecontent = readfile(myfile)
        #   manipulation of filecontent
    except FileNotFoundError:
        filecontent = "FileNotFoundError"
    except PermissionError:
        filecontent = "PermissionError"
    return filecontent

def readfile(myfile):
    with open(myfile, 'r') as file:
        filecontent = file.read()
    file.close()
    return filecontent

fileMethods_test.py

# -*- coding: utf-8 -*-

import io
import unittest
import unittest.mock as mock
import packageName.fileMethods

class TestIO(unittest.TestCase):

    def test_readfile_returns_string(self):
        mock_file_content = io.StringIO('Content in file')
        with mock.patch('packageName.fileMethods.open', return_value=mock_file_content, create=True):
            result = packageName.fileMethods.readfile('file_to_readin')
        self.assertEqual('Content in file', result)

    def test_readconfig_returns_content(self):
        with mock.patch('packageName.fileMethods.readfile', return_value="string", create=True):
            result = packageName.fileMethods.readconfig('file_to_readin')
        self.assertEqual('string', result)

    def test_readconfig_handles_FileNotFoundError(self):
        with mock.patch('packageName.fileMethods.readfile', side_effect=FileNotFoundError, create=True):
            result = packageName.fileMethods.readconfig('file_to_readin')
        self.assertEqual('FileNotFoundError', result)

    def test_readconfig_handles_PermissionError(self):
        with mock.patch('packageName.fileMethods.readfile', side_effect=PermissionError, create=True):
            result = packageName.fileMethods.readconfig('file_to_readin')
        self.assertEqual('PermissionError', result)

    # Does not work
    def test_readconfig_throws_OSError(self):
        with mock.patch('packageName.fileMethods.readfile', side_effect=OSError, create=True):
            self.assertRaises(OSError, packageName.fileMethods.readconfig('file_to_readin'))

The design of the readconfig() function is seriously problematic.  It makes no sense to convert caught exceptions into data, as if the file literally contained the string "FileNotFoundError" or "PermissionError".  If you don't have a good way to handle an exception, the right thing to do is let it propagate.

The readfile() function should be simpler:

def read_file(path):
    with open(path) as file:
        return file.read()

To note:

Since "read file" is two words in English, it should not be written as a compound word.
myfile could be better named.  It's a filesystem path, not a file handle.  Also, who is "me" in this case?
The 'r' mode is the default.
You don't need to call file.close() on an already closed file.  The whole point of the with block is that the file is automatically closed when exiting the block.


Selecting employees in certain departments using LINQ I am trying to select the DepartmentName from Employee Table based on where condition. Is any way to simplify the code?

List<Department> lstdepartment=(from i in _context.Department
                                select i).ToList();

    List<Employee>lstEmployee=(from e in _context.Employee
                            where e.EmployeeId=EmployeeId
                            select new EmployeeInfo
                            {
                                EmployeeId = e.EmployeeId,
                                FirstName = e.FirstName,
                                LastName = e.LastName,
                                MiddleName = e.MiddleName,
                                //Need code simplifiaction
                                DepartmentName=lstdepartment.Where(w=>w.DepartmentId==e.DepartmentId)
                                .Select(x=>x.DepartmentName).SingleOrDefault();
                            }).ToList();

lstdepartment

We don't use Hungarian notation with prefixes like lst in C#. It's just departments

This is notepad coding but you can use a join to get the matching deparment

var results =
    from e in _context.Employee
    join d in _context.Department on e.DepartmentId equals d.DepartmentId
    where e.EmployeeId = EmployeeId
    select new EmployeeInfo
    {
        EmployeeId = e.EmployeeId,
        FirstName = e.FirstName,
        LastName = e.LastName,
        MiddleName = e.MiddleName,
        DepartmentName= d.DepartmentName
    }).ToList();

although this is not yet exactly what you have because this will yield only exact matches. If you want deparment-name to be null then you need a left join and I think this should work:

var results =
    from e in _context.Employee
    join d in _context.Department on e.DepartmentId equals d.DepartmentId into ds
    from d in ds.DefaultIfEmpty()
    where e.EmployeeId = EmployeeId
    select new EmployeeInfo
    {
        EmployeeId = e.EmployeeId,
        FirstName = e.FirstName,
        LastName = e.LastName,
        MiddleName = e.MiddleName,
        DepartmentName= d?.DepartmentName
    }).ToList();


Navigation scroll bar So I'm trying to code a navigation scroll bar like that looks like this:

This is the code:

<!DOCTYPE html>

How does it look? Should I remove the scroll bar? If so, how do I do that?

You have the scroll always on because you set the overflow-x:scroll; in the first div. Change it to hidden and it should never show itself again.

<div style="width:100%;height:75px;overflow:hidden;">

You have to remember that if you will hide it in some cases the content may not be visible (too small browser window width or to many elements if you plan to add more). I don't know what you want to do with it but you should consider some kind of dynamic scrolling or width adjustment.


Generating all subarrays from an array of Integer I want to generate all the subarrays from an array of integer as below:

func subarrays(_ arr: [Int]) -> [[Int]]{
    var result = [[Int]]()
    guard arr.count >= 1 else { return [arr] }
    for i in 0..<arr.count {
        for res in result {
            var resi = res
            resi.append(arr[i])
            result.append(resi)
        }

        result.append([ arr[i] ])
    }
    result.append([])
    return result
}

subarrays([1,2]) //{},{1,2},{1},{2}
subarrays([1,2,3]) //{} {1}{2}{3}{1,2}{2,3}{1,3}{1,2,3}

I have alternative implementations, but really want to know if it is acceptable to use

for res in result

while adding to the array, or whether this would be seen as unusual practice in any way.

To answer your immediate question: Yes, it is fine to mutate an array while enumerating its elements. The reason is that

for res in result { ... }

is equivalent to

var iter = result.makeIterator()
while let res = iter.next() {
    // ...
}

and that arrays are value types in Swift: Mutating result inside the loop creates a new value and does not affect the original array that the iterator is using. See Remove element from collection during iteration with forEach on Stack Overflow for more details.

There are some possible simplifications. Here

var resi = res
resi.append(arr[i])
result.append(resi)

is is not necessary to declare another array variable, this can be shortened to

result.append(res + [arr[i]])

A better name for the res variable might be subarray or subset.

Instead of looping over the array indices

for i in 0..<arr.count {
    // ... do something with `arr[i]` ...
}

you can enumerate the array (as you already do in the inner loop)

for elem in arr {
    // ... do something with `elem` ...
}

The

guard arr.count >= 1 else { return [arr] }

is not needed. Your algorithm already does “the right thing” for an empty array.

So this is what we have so far:

func subarrays(_ arr: [Int]) -> [[Int]] {
    var result = [[Int]]()
    for elem in arr {
        for subarray in result {
            result.append(subarray + [elem])
        }
        result.append([elem])
    }
    result.append([])
    return result
}

It becomes a bit simpler if result is not initialized with an empty list, but with a list containing the empty subset. Adding the array element itself, and the empty set at the end of the function is no longer necessary:

func subarrays(_ arr: [Int]) -> [[Int]] {
    var result: [[Int]] = [[]]
    for elem in arr {
        for subarray in result {
            result.append(subarray + [elem])
        }
    }
    return result
}

(The subset are returned in a different order now, though.)

The inner loop could be replaced by a map  operation:

func subarrays(_ arr: [Int]) -> [[Int]] {
    var result: [[Int]] = [[]]
    for elem in arr {
        result.append(contentsOf: result.map { $0 + [elem] })
    }
    return result
}

or even the entire function body replaced by a single expression:

func subarrays(_ arr: [Int]) -> [[Int]] {
    return arr.reduce(into: [[]], { (result, elem) in
        result.append(contentsOf: result.map { $0 + [elem] })
    })
}

but that is a matter of taste – is not necessarily easier to read.

There is nothing particular with respect to integers in the algorithm, the function can be made generic to take arrays of any element type – or even arbitrary sequences.

Finally I would call it subsets(of:), which is what the function computes:

func subsets<S: Sequence>(of seq: S) -> [[S.Element]] {
    var result: [[S.Element]] = [[]]
    for elem in seq {
        for subset in result {
            result.append(subset + [elem])
        }
    }
    return result
}


Project Euler #7 10001st prime in C++ By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can
  see that the 6th prime is 13.

  What is the 10 001st prime number?

How can I optimize this code?

#include <iostream>

bool is_prime(int num)
{
    for (int i = 2; i <= num/2; ++i)
    {
        if (num % i == 0)
        {
            return false;
        }
    }
    return true;
}

int main()
{
    int count = 2;
    for (int i = 5; ; i = i+2)
    {
        if (is_prime(i))
        {
            count++;
        }
        if (count == 10001)
        {
            std::cout << i;
            return 0;
        }
    }
}

Use better algorithms: Sieve_of_Eratosthenes

You used a brute force algorithm. But even this can be highly improved.

You increment by 2 each loop.

    for (int i = 5; ; i = i+2) {

So you have noticed that all even numbers are not prime. You can improve on this. By incrementing by 2 then 4 then 2 then 4. This removes all multiples of 2 and 3 automatically.

    int inc = 2;
    for (int i = 5; ; i += inc, inc = 6 - inc) {

The brute force check runs up to num/2

    for (int i = 2; i <= num/2; ++i)

Actually you can do better than that you only need to run up to the sqrt(num) anything larger than this will not divide into num exactly.

    int limit = sqrt(num);
    for (int i = 2; i <= limit; ++i)

Actually we can take this a step further. There is no need to divide by every number lower than num. Any number that is divisible by a prime is already checked by a prime smaller than it.

For example there is no need to check 4. You already checked 2 and all numbers divisible by 4 are also divisible by 2, so need to do that check. In fact you can skip all numbers that are not prime.

bool is_prime(int num)
{
    int limit = sqrt(num);
    for (auto const& prime: primes) {
    {
        if (prime > limit) {
            return true;
        }
        if (num % prime == 0) {
            return false;
        }
    }
    return true;
}


Converting data when output std container via ostream_iterator I have a strongly typed enum and std::vector of this type.

enum class Colors { red, green, blue };
std::vector v = { Colors::blue, Colors::red };

I trying to output v to standard output via std::copy.

std::copy(v.begin(), v.end(), std::ostream_iterator<Colors>(std::cout, " ");

Compilation failed because compiler do not know how to print Colors. I can define operator << for colors, but it looks too excessive for me. Then try print int values of v elements.

std::copy(v.begin(), v.end()
  , std::ostream_iterator<std::underlying_type<Colors>::type>>(std::cout, " ");

Compilation failed because it is prohibited to print Colors to std::ostream_iterator<int>. Ok. Then std::transform can help.

std::transform(v.begin(), v.end()
,   std::ostream_iterator<std::underlying_type<Colors>::type>(std::cout, " ")
,   [&](Colors &c) -> std::underlying_type<Colors>::type
    {
        return std::underlying_type<Colors>::type(c);
    }
);

However, this lambda looks ugly. Is there neater way to copy or transform vector of Colors? Maybe boost has some way?

Of course, a loop can print, but I'd prefer to avoid explicit loop.

for(auto c : v)
{
    std::cout << std::underlying_type<Colors>::type(c) << std::endl;
}

I have a lot of different enum classes in my project. All of them I need to print out. I don't like idea to create operator << for each one individually. However, if I try to  make templated operator << then I've got a global redefinition of this operator. And this is not what I actually want.

template <typename T>
std::ostream &operator << (std::ostream &_os, const T &_t) {
    _os << typename std::underlying_type<T>::type(_t);
    return _os;
}

It seems to me that if you want to insert an object into a stream, the "right" way to do it is normally to define an operator<< for that type. That goes for strongly typed enums just as much as it does for class/struct objects.

#include <iostream>
#include <vector>
#include <utility>
#include <iterator>

enum class colors { red, green, blue };
std::vector<colors> v = { colors::blue, colors::red };

std::ostream &operator<<(std::ostream &os, colors const &c) {
    return os << std::underlying_type<colors>::type(c);
}

int main() {
    std:copy(v.begin(), v.end(), std::ostream_iterator<colors>(std::cout, "\n"));
}

That seems simple and straightforward enough that I'd be at least a little surprised to see a substantially cleaner/simpler solution (at least in C++ as it's currently defined; obviously, it's open to argument that some other language or some future definition of C++ could make things cleaner).

Using something like std::transform puts the burden in the wrong place. It requires that all code that uses the type in question contain and encode "knowledge" of the internals of the enumeration. What you generally want is to contain the complexity and the internals of the type in one place, so the rest of the code can just work with its external interface. An external interface of stream << object is simple and straightforward. Forcing the rest of the world to work with an interface of stream << std::underlying_type<colors>::type(c); anything but straightforward or simple. It's ugly and nasty.


Iterate over list and valid email address I have the following code:
(emailList type is List<string>)

//Add All emails
foreach (var mail in emailList)
{
    var isEmail = Regex.IsMatch(mail.Trim(), @"\A(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?)\Z", RegexOptions.IgnoreCase);

    if (isEmail)
    {
        email.BccRecipients.Add(mail.Trim());
        sbInfoLog.Append($"Email: {mail.Trim()}\n");
    }
    else
    {
        sbAlertLog.Append($"Not valid email address: {mail}\n");
    }
}

I'm looking for another way to:

To validate the email address
To Iterate over the list (Maybe with ForEach etc..)

Any suggestions?

This is a method I've used without any problems so far.

bool IsValidEmail(string email)
{
    try
    {
        var addr = new System.Net.Mail.MailAddress(email);
        return true;
    }
    catch
    {
        return false;
    }
}

To check the validity and handle each case you could do this.

var emailChecklist = emails.Select(x =>
    new { IsValid = IsValidEmail(x.Trim()), Address = x });

foreach(var email in emailChecklist)
{
    if (email.IsValid)
    {
        email.BccRecipients.Add(email.Address.Trim());
        sbInfoLog.Append($"Email: {email.Address.Trim()}\n");
    }
    else
    {
        sbAlertLog.Append($"Not valid email address: {email.Address}\n");
    }
}

I left the trim in the loop because I noticed that you didn't trim on the invalid email log. I assumed that you were trying to preserve the original value.


Remove useless whitespace from styled string I have a big string resource (basically the "about" text of the application) which contains styles (such as <b>, <a> etc.). The string resource is written on multiple lines, like this:

<string name="about_text"><big><b>About</b></big>\n
Lorem ipsum dolor sit amet and all that stuff...\n
\n
More stuff.</string>

Now, just like in HTML, Android treats actual new lines (not the special \n) as spaces, so the text ends up looking something like this:

About
  Lorem ipsum dolor sit
amet and all that
stuff...

  More stuff.

Which looks pretty stupid. Now, I have two options:

Write the whole thing on one line. I did not go with this because:

It would be a pretty big line.
The text needs to be translated, and putting it all on one line would give the translators a great headache.

Remove the unneeded whitespace programatically.

I went with the second solution, but I'm not sure my implementation is optimal:

CharSequence aboutText = getText(R.string.about_text);

SpannableStringBuilder ssb = new SpannableStringBuilder(aboutText);
for (int i=0; i < ssb.length()-1; i++) {
    if (ssb.charAt(i) == '\n' && ssb.charAt(i+1) == ' ') {
        ssb.replace(i+1, i+2, "");
    }
}

this.aboutText = (TextView) findViewById(R.id.about_text);

this.aboutText.setText(ssb);

It seems very hackish, but I could not find a better way. Is there a better way?

The answer is to put the newlines at the start of each line, directly before the text.

<string name="about_text"><big><b>About</b></big>
    \nLorem ipsum dolor sit amet and all that stuff...
    \n\nMore stuff.
</string>

Therefore all the redundant whitespace appears at the end of each line (e.g. after "About") where it doesn't affect the appearance of your text.


Rewriting C# implementation of Snake class in F# This is my C# class, which takes 2.6s:

class Snake
{
    readonly Symbol _mySymbol;
    readonly int _multiplier;
    readonly int _hit;
    readonly int _payout;
    readonly Snake _bestSnake;

    public Symbol MySymbol { get { return _mySymbol; } }
    public int Multiplier { get { return _multiplier; } }
    public int Hit { get { return _hit; } }
    public int Payout { get { return _payout; } }

    public Snake BestSnake { get { return _bestSnake; } }

    public Snake(Apple myApple)
    {
        this._mySymbol = myApple.MySymbol;
        this._multiplier = myApple.Multiplier;
        this._hit = 1;
        this._payout = _mySymbol.Payout_[0] * (_multiplier == 0 ? 1 : _multiplier);
        this._bestSnake = this;
    }

    Snake(Symbol mySymbol, int multiplier, int hit)
    {
        this._mySymbol = mySymbol;
        this._multiplier = multiplier;
        this._hit = hit;
        this._payout = _mySymbol.Payout_[hit - 1] * (multiplier == 0 ? 1 : multiplier);
        this._bestSnake = this;
    }

    Snake(Symbol mySymbol, int multiplier, int hit, Snake bestRecentSnake)
        : this(mySymbol, multiplier, hit)
    {
        if (_payout < bestRecentSnake._payout)
        {
            this._bestSnake = bestRecentSnake;
        }
        else if (_payout == bestRecentSnake._payout)
        {
            if (bestRecentSnake._mySymbol.Type == 1 && mySymbol.Type != 1)
            {
                this._bestSnake = bestRecentSnake;
            }
        }
    }

    public Snake Eat(Apple nextApple)
    {
        var resultSymbol = Symbol.Add(_mySymbol, nextApple.MySymbol);
        if (resultSymbol == null)
            return null;

        var newSnake = new Snake(resultSymbol, Math.Max(_multiplier, nextApple.Multiplier), Hit + 1, _bestSnake);

        return newSnake;
    }

This is my F# class, which takes 5s:

type Snake (mySymbol: Symbol, multiplier, hit, best: Option<Snake>) =

let payout (mySymbol: Symbol) hit multiplier =
    match multiplier with
    | 0 -> mySymbol.Payout_.[hit-1]
    | a -> mySymbol.Payout_.[hit-1] * a

new (myApple: Apple) = Snake (myApple.MySymbol, myApple.Multiplier, 1, None)

member this.BestSnake =
    match best with
    | Some a when this.Payout < a.Payout -> a
    | Some a when this.Payout = a.Payout && a.MySymbol.Type = 1 && mySymbol.Type <> 1 -> a
    | _ -> this

member this.MySymbol = mySymbol
member this.Payout = payout mySymbol hit multiplier
member this.Multiplier = multiplier
member this.Hit = hit

member this.Eat (myApple: Apple) =
    let tmp = Symbol.Add mySymbol myApple.MySymbol
    match tmp with
    | None -> Operators.Unchecked.defaultof<Snake>
    | Some a -> Snake (a, (max multiplier myApple.Multiplier), hit+1, Some this.BestSnake)

There are a few small differences in your translation. I would start with as literal a translation possible, then optimize/make idiomatic from there. If done incrementally it should be easy to spot performance problems as they occur. Here's a starting point:

(* stub *)
[<AllowNullLiteral>]
type Symbol =
  member x.Payout = [|0|]
  member x.Type = 0
  static member Add(sym1, sym2) : Symbol = null

(* stub *)
type Apple =
  abstract MySymbol : Symbol
  abstract Multiplier : int

type private BestSnake =
  | This
  | RecentSnake of Snake

and [<AllowNullLiteral>] Snake private (mySymbol: Symbol, multiplier, hit, bestSnake) as this =
  let payout = mySymbol.Payout.[hit - 1] * (match multiplier with 0 -> 1 | m -> m)
  let bestSnake =
    match bestSnake with
    | This -> this
    | RecentSnake bestRecentSnake ->
      if payout < bestRecentSnake.Payout ||
        (payout = bestRecentSnake.Payout && bestRecentSnake.MySymbol.Type = 1 && mySymbol.Type <> 1)
      then bestRecentSnake
      else null

  member val MySymbol : Symbol = mySymbol
  member val Multiplier = multiplier
  member val Hit = hit
  member val BestSnake = bestSnake
  member val Payout = payout

  new (mySymbol, multiplier, hit, bestRecentSnake) = Snake(mySymbol, multiplier, hit, RecentSnake bestRecentSnake)
  new (mySymbol, multiplier, hit) = Snake(mySymbol, multiplier, hit, This)
  new (myApple: Apple) = Snake(myApple.MySymbol, myApple.Multiplier, 1, This)

  member this.Eat(nextApple: Apple) =
    match Symbol.Add(this.MySymbol, nextApple.MySymbol) with
    | null -> null
    | resultSymbol -> Snake(resultSymbol, max this.Multiplier nextApple.Multiplier, this.Hit + 1, this.BestSnake)

The performance of this should be on par with C#.


Square root implementations The question was:

  Write a function that calculates the root of a given number.

Answer A = I wrote it for integers. just run up to half of the number
Answer B+C = are for double, using Newton's law.

I just implemented this in C#.

They all work and were tested.Please comment me about anything from not well written code, complexity, style, or give a better solution.

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

namespace ConsoleApplication2
{
    public class FindSquareRoot
    {
        public FindSquareRoot()
        {
            int numberToFind = 49;
            int res = SquareRoot(numberToFind);
            Console.WriteLine(res);

            double doubleNumberToFind = 157.0;
            double resDouble = SquareRoot(doubleNumberToFind);
            Console.WriteLine(resDouble);
            resDouble = SquareRootBetter(doubleNumberToFind);
            Console.WriteLine(resDouble);
        }

        public int SquareRoot(int number)
        {
            int half = number / 2;
            int counter = 0;
            while (counter < half)
            {
                if (Math.Pow(counter, 2.0) == number)
                {
                    return counter;
                }
                counter++;
            }
            return half;
        }

        public double SquareRoot(double number)
        {
            double precision = 10e-8;
            double low = 0.0;
            double high = 0.0;
            double prev = 0.0;
            double mid = number / 2.0;

            while(Math.Abs(mid - prev) > precision )
            {
                if(Math.Pow(mid,2) > number)
                {
                    high = mid;
                }else
                {
                    low = mid;
                }
                prev = mid;
                mid = (low+high)/ 2.0;
            }
            return mid;
        }
        public double SquareRootBetter(double number)
        {
            double precision = 10e-8;
            double prev = 0.0;
            double mid = number;
            while(Math.Abs(mid - prev ) > precision)
            {
                prev = mid;
                mid = (mid +(number/ mid))/2.0;
            }
            return mid;
        }
    }
}

SquareRoot(int)

returns half if the integer is not a perfect square. A very dubious decision. I'd consider returning a best integer approximation, or throwing an exception.
uses a linear search. A binary search would be quite faster. A Newton-Raphson (yes it works with integers) would be even factor.

An error approximation is better done as Math.abs(number - Math.pow(candidate_square_root, 2))


At most one LocalDate per month in a list I'm implementing argument check in a method where one parameter is a List of LocalDate objects (from Joda-Time). I want to make sure that List contains at most one LocalDate per month.

public List<LocalDate> generateAllDates(List<LocalDate> firstYearDates, ...) { }

Thus, for example

Arrays.asList(
    new LocalDate(2012, 3, 25),
    new LocalDate(2012, 4, 30),
    new LocalDate(2013, 3, 26)
)

would be an illegal argument because there are two dates whose month is March.

Currently I have something like this (making use of Guava's Multimap):

Multimap<Integer, LocalDate> monthToDates = HashMultimap.create();
for (LocalDate date : firstYearDates) {
    monthToDates.put(date.getMonthOfYear(), date);
}
for (Integer i : monthToDates.keySet()) {
    if (monthToDates.get(i).size() > 1) {
        throw new IllegalArgumentException(
            "firstYearDates must contain at most one entry per month");
    }
}

Question is, how would you simplify that? Using any API features of JDK 1.6, Guava or Joda-Time is fine.

Bit late but there is Guava built-in method for that - Maps.uniqueIndex - given:

List<LocalDate> dates = Arrays.asList(
     new LocalDate(2012,3,25), new LocalDate(2012,4,30), new LocalDate(2013,3,26);

with JDK 8:

Maps.uniqueIndex(dates, LocalDate::getMonthOfYear);

or with JDK <7:

Maps.uniqueIndex(dates, new Function<LocalDate, Integer>() {
    @Override
    public Integer apply(final LocalDate date) {
      return date.getMonthOfYear();
    }
});

It will throw

java.lang.IllegalArgumentException: duplicate key: 3

in this example.


Getting current timezone My goal is to precisely calculate the current timezone using C++ standard library, represented as the difference (in seconds) between GMT time and local time.

Example: Italy is GMT+1 so my expected output is -3600. This offset represents the difference between GMT time (i.e. 12:00) and italian time (i.e. 13:00), ignoring daylight savings.

My actual code looks like this and it seems to work:

long timezone = 0;

bool _tzset() {
   std::time_t now = std::time(NULL);
   std::time_t local = std::mktime(std::localtime(&now));
   std::time_t gmt = std::mktime(std::gmtime(&now));
   timezone = static_cast<long> (gmt - local);
   return true;
}

From my understanding, what I'm doing here is getting the local time (ignoring DST) and the GMT time (ignoring DST) and just returning the difference.

It looks pretty straightforward but is it correct? Do you see any flaws?

EDIT: I was not clear about my real goal here. I am working on a Win32 framework that has to be ported to the Universal Windows Platform. The problem is that this framework uses a lot of CRT functions that are not supported anymore in the UWP. As you can see, _tzset() is one of them (also it looks like that the timezone variable is defined globally in a Win32 environment). In other words what I'm doing here is giving an alternative implementation to these unsupported functions. I can't change the signature of them.

Headers
You're missing #include <ctime>.
Naming
Identifiers beginning with underscore are reserved for the implementation, so _tzset() is a risky choice, and may result in undefined behaviour.
Interface
What does the return value mean?  Although declared to return bool, it never returns a false value, so it's unclear how the result is intended to be used.  void may be a better choice, but I think it's more reasonable to return an arithmetic result (and thus eliminate the use of the global timezone).
It is likely that we might want to find this information for an arbitrary time value, rather than only for "now", so I suggest adding a defaulted parameter to pass in the time to be considered.
Assumptions about std::time_t
This code assumes that time_t is a signed type; if it's unsigned, then the subtraction gmt - local may overflow.  It also assumes that time_t counts in units of seconds; that is not required by the C++ standard (though it is a common implementation choice, and is perhaps required for POSIX compliance).  Instead of using arithmetic subtraction, it is portable to use std::difftime() - it is safe if time_t is unsigned, and it is defined to return its results in seconds.
Assumptions about std::mktime()
This code assumes that std::mktime() ignores the tm_isdst field of std::tm.  In my experience, implementations do not ignore it, meaning that local and gmt values are always equal in time zones such as Europe/London.
Failure around DST transitions
Even if corrected to account for tm_isdst, I'm finding it hard to reason whether the code is correct shortly before (or shortly after) a daylight-savings transition (in either direction).  I'd expect to see four unit-tests (in a specific timezone) that verify the behaviour, but there are no tests provided at all.
Alternative approach
Instead of (ab)using std::mktime() in this way, it's far safer and more transparent to use std::time_put() with a format string of %z to obtain the offset from UTC in ISO-8601 format (±HHMM) and convert that string into seconds (taking care to apply the sign to both the hours and the minutes):
#include <ctime>
#include <iomanip>
#include <sstream>

static constexpr time_t const NULL_TIME = -1;

// returns difference in seconds from UTC at given time
// or at current time if not specified
long tz_offset(time_t when = NULL_TIME)
{
    if (when == NULL_TIME)
        when = std::time(nullptr);
    auto const tm = *std::localtime(&when);
    std::ostringstream os;
    os << std::put_time(&tm, "%z");
    std::string s = os.str();
    // s is in ISO 8601 format: "±HHMM"
    int h = std::stoi(s.substr(0,3), nullptr, 10);
    int m = std::stoi(s[0]+s.substr(3), nullptr, 10);

    return h * 3600 + m * 60;
}

#include <iostream>
int main()
{
    std::cout << tz_offset() << std::endl;
}


Detect if string contains more than 1 period This function returns true if the string contains zero or one periods, and returns false if the string contains more than one period.

bool isStringValid(std::string str){
    std::size_t pos = str.find('.', 0);
    if(pos != std::string::npos){
        pos = str.find('.', pos + 1);
        if(pos != std::string::npos){
            std::cout << "\nError: Multiple decimal marks\n";
            return false;
        }
    }
    return true;
}

For example:

If str is 1.519, it returns true;
If str is 159, it returns true;
If str is 1.541.1, it returns false;
If str is 1.1.1.1, it returns false, and so on.

I am using the Boost C++ libraries elsewhere in the program, if that's useful in any way. This code isn't specific for finding the period . character, it should work with any character, but this is the use case for me.

Can this be done more efficiently?

Optimize for maintainability first

Is the complexity of your function really needed? The functionality can be implemented in one line using only standard-library algorithms.

bool
is_valid(const std::string& str)
{
  return (std::count(std::begin(str), std::end(str), '.') <= 1);
}

Let's see how this will compare against your version. I hope we can agree that for readability, it is a clear win. Considering performance:

If the string is valid, both algorithms will iterate over the entire string exactly once.
If the string is not valid (contains more than one .), your algorithm will short-circuit and immediately return while std::count will continue iterating until the end of the string.

But do you really expect that the case of strings that

are invalid,
very long and
contain the second . near the beginning

is frequent enough to optimize for? It certainly won't buy you anything in the worst-case scenario. Even if you think it is, I would strongly encourage you to benchmark first. You might very well be surprised.

Don't make copies you don't need

Conceptually, your function is read-only. In order to answer whether a string has a certain property, we don''t have to modify it. Copying strings can be expensive so that operation might well dwarf any other improvement you make. (The iteration is \$\mathcal{O}(n)\$ but copying is also \$\mathcal{O}(n)\$ plus a substantial cost for allocating (and later deallocating) the buffer. Plus potentially touching cold memory.)

The corollary of this is of course to make your function take its parameter as a const reference (as I did in the alternative above).

Don't mix concerns

Your function is a predicate for strings. Predicates should be pure. Producing output shouldn't be any of that function's business. Let the caller of your function decide what they want to do with the result. (To illustrate, think how awkward it would be if std::string::find would write to standard output whenever it found something. Leave alone the performance implications of doing I/O like this.)

Avoid deeply nested control structures

You could improve the readability of your code by reducing the levels of nested structures by returning early. For example, if you start by

const auto pos = str.find('.', 0);
if (pos == std::string::npos)
  return true;

then you can flush the remainder of the code to the left. I also find this style more readable because I don't have to check whether the function will actually do anything else after that point.


Writing filter with one or more comparison values (defn ret-match-row
  "Finds a row in in-seqB that matches in-valA and in-seq-keyB"
  [in-valA in-seq-keyB in-seqB]
  (filter #(= in-valA (in-seq-keyB %1)) in-seqB))

I want to expand this function, so that it still must always accept one comparison key and that n more comparison keys could be passed to the function if need be.

The only way I can think of to do this is create a non # anonymous function that would handle the comparison of multiple arguments. I do not know Clojure macros well enough to know if this would be an appropriate place to use a macro.

Minimally I would rewrite the function's argument signature like this:

(defn ret-match-row
  "Finds a row in in-seqB that matches in-valA and other optional vals."
  [match-keyA in-seq-keyB in-seqB & optional-match-keys]

Any thoughts, pointers, or guidance would be appreciated.

Here is some dummied up real data (dummied up to protected sensitive information)

666/0010|RET|xyz-ab-caba|SMITH|JOHN|HEALTHCO|377.94|20140702|201407| 2014-07-01

I need to filter on the ssn field (xyz-...) and the health description as an AND condition. I've dummied up the health description as HEALTHCO, but, in reality, the health description includes words like SPOUSE, SINGLE, and so on and is much more robust.

If I'm understanding your question right, you want to write a function that will take a call like this:

(matching-entries entries :ssn "xyz-ab-caba" :health-desc "HEALTHCO")

and it will return a seq of entries with matching data for the given keys:

("666/0010|RET|xyz-ab-caba|SMITH|JOHN|HEALTHCO|377.94|20140702|201407| 2014-07-01")

(in this case, only one match was returned)

The most idiomatic syntax I can think of for a function like this would be something like this:

; notes:
;
; 1) rows has to be the first argument, as everything after '&' is assumed to be
;    a key-value combination
;
; 2) This is assuming that each row is represented as a map, so that you can easily
;    grab the value for a given comparison key. If your rows are simply strings,
;    then you'll need to convert them to maps first, perhaps by writing a helper
;    function that splits the string on `|` and zips your comparison keys with the
;    data in each cell., e.g. (zipmap [:foo :bar :ssn ...]
;                                     (clojure.string/split #"|" row))

(defn matching-entries [entries & queries]
  (letfn [(match? [entry]
            (every? (fn [[k v]] (= v (k entry)))
                    (partition 2 queries)))]
    (filter match? entries)))

(partition 2 queries) takes arguments like :ssn "xyz-ab-caba" :fname "JOHN" and turns it into a list of key-value "queries", i.e. '((:ssn "xyz-ab-caba") (:fname "JOHN")). Then every? checks to make sure that each one of these queries is satisfied by a given entry. Finally, we use filter to find all the entries that have the desired values for the given keys.


Seekable HTTP Range Stream I read around for a while and surprisingly didn't find a fitting solution. I need a .NET Stream that internally issues HTTP Range requests when it seeks. There are huge files server-side for which I only need small amounts of data in a very random-access way. The consumer of the custom stream (beyond my control) issues lots of tiny, sequential reads, so the stream has to do caching. I have the following, and it seems to work quite well, but it's long and it's irritating my sense of "this should be easier in .NET".

Are there any suggestions to improve or shrink this?

class PartialHTTPStream : Stream, IDisposable
{
    Stream stream;
    WebResponse resp;
    int cacheRemaining = 0;
    const int cachelen = 1024;

    public string Url { get; private set; }
    public override bool CanRead { get { return true; } }
    public override bool CanWrite { get { return false; } }
    public override bool CanSeek { get { return true; } }

    long position = 0;
    public override long Position
    {
        get { return position; }
        set
        {
            long delta = value - position;
            if (delta == 0)
                return;
            if (delta > 0 && delta < cacheRemaining)
            {
                Console.WriteLine("Seeking in cache");
                byte[] dummy = new byte[delta];
                cacheRemaining -= (int)delta;
                while (delta > 0)
                {
                    int nread = stream.Read(dummy, 0, (int)delta);
                    if (nread == 0) throw new IOException();
                    delta -= nread;
                }
            }
            else cacheRemaining = 0;
            position = value;
            Console.WriteLine("Seek {0}", value);
        }
    }

    long? length;
    public override long Length
    {
        get
        {
            if (length == null)
            {
                HttpWebRequest request = HttpWebRequest.CreateHttp(Url);
                request.Method = "HEAD";
                length = request.GetResponse().ContentLength;
            }
            return length.Value;
        }
    }

    public PartialHTTPStream(string Url) { this.Url = Url; }

    public override void SetLength(long value)
    { throw new NotImplementedException(); }

    public override int Read(byte[] buffer, int offset, int count)
    {
        if (cacheRemaining == 0)
        {
            Console.WriteLine("Cache miss");
            if (stream != null)
            {
                stream.Close();
                resp.Close();
            }
            HttpWebRequest req = HttpWebRequest.CreateHttp(Url);
            cacheRemaining = (int)Math.Min(Length - Position, Math.Max(count, cachelen));
            req.AddRange(Position, Position + cacheRemaining - 1);
            resp = req.GetResponse();
            stream = resp.GetResponseStream();
        }

        count = Math.Min(buffer.Length - offset, Math.Min(cacheRemaining, count));
        Console.WriteLine("Read {0} @ {1}", count, Position);

        int nread = stream.Read(buffer, offset, count);
        position += nread;
        cacheRemaining -= nread;
        return nread;
    }

    public override void Write(byte[] buffer, int offset, int count)
    { throw new NotImplementedException(); }

    public override long Seek(long pos, SeekOrigin origin)
    {
        switch (origin)
        {
            case SeekOrigin.End:
                Position = Length + pos;
                break;
            case SeekOrigin.Begin:
                Position = pos;
                break;
            case SeekOrigin.Current:
                Position += pos;
                break;
        }
        return Position;
    }

    public override void Flush() { }

    new void Dispose()
    {
        base.Dispose();
        if (stream != null)
        {
            stream.Dispose();
            stream = null;
        }
        if (resp != null)
        {
            resp.Dispose();
            resp = null;
        }
    }
}

Position.Set

This

long delta = value - position;
if (delta == 0)
    return;

isn't understandable at first glance. You should switch it to

if (position == value) { return; }
long delta = value - position;

You declare long delta for the above substraction but later on you will at least 2 times cast it to int. It would be better to do it the other way.

if (position == value) { return;}
int delta = (int)(value - position);

Potential "bug"

Assume the user of this class will call either Seek() or Position.Set() your code will throw an exception because stream == null.

You should extract the initializing/opening of the stream to a separate method which you call from the constructor, and if needed (cacheRemaining == 0) from the Read() method.

private void OpenStream(int count = 0)
{
    if (stream != null) { resp.Close(); }

    HttpWebRequest req = HttpWebRequest.CreateHttp(Url);

    cacheRemaining = (int)Math.Min(Length - Position, Math.Max(count, cachelen));
    req.AddRange(Position, Position + cacheRemaining - 1);

    resp = req.GetResponse();
    stream = resp.GetResponseStream();

}

Stream.Close()

The call to Stream.Close is not neccessary if you are closing the WebResponse as this is automatically closing the underlaying stream.

Naming

Shortening variable names to resp does not help for readability. Simply name it response.

Variables should be named using camelCase casing which you do except for cachelen which should be cacheLen or cacheLength.

General

What I don't see in your code is the caching. You are using cacheRemaining and cachelen but you don't cache anything.

The Read() method shows unexpected behaviour. Passing in an array with Length == 10 and an offset == 5 then with count == 6 this should throw an exception.

Using more of the built-in features of .NET

For sure that can be done, but as there is no seekable Network or HTTP stream you need to write some amount of code, but I don't see where this should matter, as you only do this once.

What would I do different to your implementation ?

copy the responsestream to a MemoryStream to take advantage of using the Seek() method.
overwrite the Close() method instead of implementing IDisposable as the Stream's Dispose() method only calls the Close() method.
make the cacheLength adjustable, at least by using a constructor parameter.
Ensure that the behaviour of my implementation is the same as of the abstract Stream class

Each method/property should throw an ObjectDisposedException if the stream is disposed
Read() method

If buffer is null throw an ArgumentNullException
If offset or count is negativ throw an ArgumentOutOfRangeException
If buffer.Length - offset < count throw an ArgumentException

But as always code does tell more than just words

class PartialHTTPStream : Stream
{
    private readonly Int64 cacheLength = 1024;
    private const Int32 noDataAvaiable = 0;
    private MemoryStream stream = null;
    private Int64 currentChunkNumber = -1;
    private Int64? length;
    private Boolean isDisposed = false;

    public PartialHTTPStream(String url)
        : this(url, 1024) { }

    public PartialHTTPStream(String url, Int64 cacheLength)
    {
        if (cacheLength > 0) { this.cacheLength = cacheLength; }
        Url = url;
    }

    public String Url { get; private set; }

    public override Boolean CanRead
    {
        get
        {
            EnsureNotDisposed();
            return true;
        }
    }

    public override Boolean CanWrite
    {
        get
        {
            EnsureNotDisposed();
            return false;
        }
    }

    public override Boolean CanSeek
    {
        get
        {
            EnsureNotDisposed();
            return true;
        }
    }

    public override Int64 Length
    {
        get
        {
            EnsureNotDisposed();
            if (length == null)
            {
                HttpWebRequest request = HttpWebRequest.CreateHttp(Url);
                request.Method = "HEAD";
                length = request.GetResponse().ContentLength;
            }
            return length.Value;
        }
    }

    public override Int64 Position
    {
        get
        {
            EnsureNotDisposed();
            Int64 streamPosition = (stream != null) ? stream.Position : 0;
            Int64 position = (currentChunkNumber != -1) ? currentChunkNumber * cacheLength : 0;

            return position + streamPosition;
        }
        set
        {
            EnsureNotDisposed();
            EnsurePositiv(value, "Position");
            Seek(value);
        }
    }

    public override Int64 Seek(Int64 offset, SeekOrigin origin)
    {
        EnsureNotDisposed();
        switch (origin)
        {
            case SeekOrigin.Begin:
                break;
            case SeekOrigin.Current:
                offset = Position + offset;
                break;
            default:
                offset = Length + offset;
                break;
        }

        return Seek(offset);
    }

    private Int64 Seek(Int64 offset)
    {
        Int64 chunkNumber = offset / cacheLength;

        if (currentChunkNumber != chunkNumber)
        {
            ReadChunk(chunkNumber);
            currentChunkNumber = chunkNumber;
        }

        offset = offset - currentChunkNumber * cacheLength;

        stream.Seek(offset, SeekOrigin.Begin);

        return Position;
    }

    private void ReadNextChunk()
    {
        currentChunkNumber += 1;
        ReadChunk(currentChunkNumber);
    }

    private void ReadChunk(Int64 chunkNumberToRead)
    {
        Int64 rangeStart = chunkNumberToRead * cacheLength;

        if (rangeStart > Length) { return; }

        Int64 rangeEnd = rangeStart + cacheLength - 1;
        if (rangeStart + cacheLength > Length)
        {
            rangeEnd = Length - 1;
        }

        if (stream != null) { stream.Close(); }
        stream = new MemoryStream((int)cacheLength);

        HttpWebRequest request = HttpWebRequest.CreateHttp(Url);
        request.AddRange(rangeStart, rangeEnd);

        using (WebResponse response = request.GetResponse())
        {
            response.GetResponseStream().CopyTo(stream);
        }

        stream.Position = 0;
    }

    public override void Close()
    {
        EnsureNotDisposed();

        base.Close();
        if (stream != null) { stream.Close(); }
        isDisposed = true;
    }

    public override Int32 Read(Byte[] buffer, Int32 offset, Int32 count)
    {
        EnsureNotDisposed();

        EnsureNotNull(buffer, "buffer");
        EnsurePositiv(offset, "offset");
        EnsurePositiv(count, "count");

        if (buffer.Length - offset < count) { throw new ArgumentException("count"); }

        if (stream == null) { ReadNextChunk(); }

        if (Position >= Length) { return noDataAvaiable; }

        if (Position + count > Length)
        {
            count = (Int32)(Length - Position);
        }

        Int32 bytesRead = stream.Read(buffer, offset, count);
        Int32 totalBytesRead = bytesRead;
        count -= bytesRead;

        while (count > noDataAvaiable)
        {
            ReadNextChunk();
            offset = offset + bytesRead;
            bytesRead = stream.Read(buffer, offset, count);
            count -= bytesRead;
            totalBytesRead = totalBytesRead + bytesRead;
        }

        return totalBytesRead;

    }

    public override void SetLength(Int64 value)
    {
        EnsureNotDisposed();
        throw new NotImplementedException();
    }

    public override void Write(Byte[] buffer, Int32 offset, Int32 count)
    {
        EnsureNotDisposed();
        throw new NotImplementedException();
    }

    public override void Flush()
    {
        EnsureNotDisposed();
    }

    private void EnsureNotNull(Object obj, String name)
    {
        if (obj != null) { return; }
        throw new ArgumentNullException(name);
    }
    private void EnsureNotDisposed()
    {
        if (!isDisposed) { return; }
        throw new ObjectDisposedException("PartialHTTPStream");
    }
    private void EnsurePositiv(Int32 value, String name)
    {
        if (value > -1) { return; }
        throw new ArgumentOutOfRangeException(name);
    }
    private void EnsurePositiv(Int64 value, String name)
    {
        if (value > -1) { return; }
        throw new ArgumentOutOfRangeException(name);
    }
    private void EnsureNegativ(Int64 value, String name)
    {
        if (value < 0) { return; }
        throw new ArgumentOutOfRangeException(name);
    }
}


Navigating over a square spiral I recently found adventofcode, but when I solved Day 3 in Python, I noticed that my code isn't looking very nice. The challenge is about navigating a hypothetical memory laid out in a square spiral:

  You come across an experimental new kind of memory stored on an infinite two-dimensional grid.

  Each square on the grid is allocated in a spiral pattern starting at a location marked 1 and then counting up while spiraling outward. For example, the first few squares are allocated like this:

17  16  15  14  13
18   5   4   3  12
19   6   1   2  11
20   7   8   9  10
21  22  23---> ...

  Given an input, the program should print:

  The Manhattan distance of the cell with that address to the cell with address 1 (justified by the only access port being at address 1)

  For example:

  Data from square 1 is carried 0 steps, since it's at the access port.
  Data from square 12 is carried 3 steps, such as: down, left, left.
  Data from square 23 is carried only 2 steps: up twice.
  Data from square 1024 must be carried 31 steps.

  The first value written to memory larger than the input during a "stress test".

  As a stress test on the system, the programs here clear the grid and then store the value 1 in square 1. Then, in the same allocation order as shown above, they store the sum of the values in all adjacent squares, including diagonals.

  Square 1 starts with the value 1.
  Square 2 has only one adjacent filled square (with value 1), so it also stores 1.
  Square 3 has both of the above squares as neighbors and stores the sum of their values, 2.
  Square 4 has all three of the aforementioned squares as neighbors and stores the sum of their values, 4.
  Square 5 only has the first and fourth squares as neighbors, so it gets the value 5.

47  142  133  122   59
304    5    4    2   57
330   10    1    1   54
351   11   23   25   26
362  747  806--->   ...

I'm particularly looking for tips on reducing the code repetition in the coords function. Should I use list comprehension to sum the values? It seems quite long. Finally, is there a more Pythonic way to include a special case when a key isn't found?

from math import sqrt, floor
import itertools

# Given an address in the spiral memory, return the coordinates of that memory cell,
# where x grows when moving right, and y grows when moving down.

def coords(address):
# For a spiral of side length n, the bottom right element will have an address of n^2
# Starting from that element, the spiral goes one cell right, and then n cells up,
# n+1 cells left, n+1 cells down and n+2 cells right

# Start in the bottom right corner of the largest spiral that does not contain
# the address we are looking for.
    n = floor(sqrt(address))

    if n % 2 == 0:
        n -= 1

# r - the radius of the spiral of side length n
    r = (n - 1) // 2
    address -= n ** 2

    if address == 0:
        return r, r
    address -= 1

    if address < n:
        return r + 1, r - address
    address -= n

    if address < n + 1:
        return r + 1 - address, r - n
    address -= n + 1

    if address < n + 1:
        return r - n, r - n + address
    address -= n + 1

    if address < n + 2:
        return r - n + address, r + 1
    raise Exception("This shouldn't ever happen")

def distance(x, y):
    return abs(x) + abs(y)

inp = int(input())
print(distance(*coords(inp)))

spiral = { (0, 0): 1 }
for address in itertools.count(2):
    x, y = coords(address)
    value = sum([spiral[(x + dx, y + dy)] for dx, dy in itertools.product([-1, 0, 1],
      repeat = 2) if (x + dx, y + dy) in spiral])

    spiral[(x, y)] = value

    if value > inp:
        print(value)
        break

Note: I had to add the blockquotes in an edit because SE considers heuristics about how well an answer is formatted an absolute truth... I think something should be done about it. If I am going as far as figuring out how to unbind an event listener that doesn't let me post, it should be clear that I am certain about my formatting.

I wrote my answer in c++, it might help you. On first look, our solutions are quite similar, at least from an idea standpoint.

//
// Created by Felix Crazzolara on 09.12.17.
//

#include <cstdlib>
#include <iostream>

using namespace std;

int main (int argc, char* argv[]) {
    int position = 277678;

    // Find spiral radius
    int n = 1;
    while (position > n*n)
         n += 2;
    // Find side
    int side = 4;
    int pos = n*n;
    while (side > 1 && position < (pos - n + 1)) {
        pos -= n + 1;
        --side;
    }
    int middle_point = pos - (n - 1) / 2;
    int dist = abs(position - middle_point);
    dist += (n - 1) / 2;
    cout << dist << endl;
}

Explanation:
The highest number in every spiral is the bottom right corner, that has the value \$n^2\$ if we denote the length of the sides of a spiral with \$n\$.

If a spiral has side length \$n\$ obviously the next spiral has side length \$n' = n + 2\$.

Let's assume that our point is in the spiral with side length \$n\$. There are 4 corners. Our point got to be somewhere, so we want to find a corner that is greater-equal our point. Once we got the "smallest" corner being greater-equal then our point, we know, that our point must lay on the side given by the found corner and the next smaller corner.

When starring a bit on the first numbers of the memory layout, once you'll remark that the way to go the origin is given by the way to go to the closest point being vertical/horizontal in respect to the origin, being on the current spiral, plus the way from that point to the origin.

How these ideas fit together in code, I'll leave up to you to think about.


Validation rules with support for immediate termination of processing I am picking up a code smell from this code and I am not sure of the best way to rid myself of it.

I have a validation class that has a function that returns a list of IValidationRule<T> implementations (RuleOne, RuleTwo, RuleThree).

public interface IValidationRule<T>
{
    ValidationRuleResult Validate(T instance);
}

public override ValidationRules<Order> AllRules()
=> new ValidationRules<Order>
{
    Rules =
    {
        RuleOne(),
        RuleTwo(),
        new StopProcessingIfInvalidCheckpoint<Order>(),
        RuleThree()
    }
};

As the rules are iterated through, the IValidationRule<T>.Validate method is called on them.  They will return their results which are added to a collection.

I implemented a class that inherits from IValidationRule<T> called StopProcessingIfInvalidCheckpoint which will cease the processing of any rules following it if any of the rules before it fail.
I felt this was a clean and visual way to kick out of the validation routine early.

What I don't like is that the StopProcessingIfInvalidCheckpoint class has to be passed the same generic as all the other rules around it.  The Validate method on this class is never even called and thus, it does not matter that it knows it's generic is an Order.

Below is the extension method I use to validate a list of rules and you can see where I kick out if I come across the StopProcessingIfInvalidCheckpoint class:

public static ValidationRuleResult Validate<T>(
    this IEnumerable<IValidationRule<T>> validationRules,
    T instance
)
{
    var validationResults = new List<ValidationResult>();

    foreach (var vr in validationRules)
    {
        if (vr is StopProcessingIfInvalidCheckpoint<T> && validationResults.Any()) { break; }
        var result = vr.Validate(instance);
        if (result.IsValid) { continue; }
        validationResults.AddRange(result.ValidationResults);
    }

    var res = new ValidationRuleResult
    {
        ValidationResults = validationResults,
        IsValid = !validationResults.Any()
    };
    return res;
}

How can I improve this code?

You shouldn't bind the implementation to any particular type in this case the StopProcessingIfInvalidCheckpoint.

I think it would be better to drop this class and instead add a new property IsFinal to the interface. You could check if the validation fails and IsFinal is set to true then you stop processing other rules.

If you want to get rid of the generic argument but at same time you want to keep it for other implementations then add another abstraction layer. Make the interface non-generic and add an abstract class that is derived from the non-generic interface:

interface IValidationRule
{
    ValidationRuleResult Validate(object instance);
    bool IsFinal { get; }
}

abstract class ValidationRule<T> : IValidationRule
{
    public abstract ValidationRuleResult Validate(object instance);
    public bool IsFinal { get; set; }
}

class FooValidationRule : ValidationRule<int>
{
    public override ValidationRuleResult Validate(object instance)
    {
        ... do something with (T)instance;
    }
}

and

foreach (var vr in validationRules)
{
    var result = vr.Validate(instance);
    if (!result.IsValid && vr.IsFinal)
    {
        break;
    }
    validationResults.AddRange(result.ValidationResults);
}

so

Rules =
{
    RuleOne(),
    RuleTwo() { IsFinal = true },
    RuleThree()
}