XOR linked list implementation I am trying to resolve below question in order to prepare for an interview xD

An XOR linked list is a more memory efficient doubly linked list.
Instead of each node holding next and prev fields, it holds a field
named both, which is an XOR of the next node and the previous node.
Implement an XOR linked list; it has an add(element) which adds the
element to the end, and a get(index) which returns the node at
index.

Please suggest how this implementation can be improved. Thank you in prior.
#include <stdio.h>
#include <stdlib.h>

typedef struct {
    int value;
    long both;

} StNode;
typedef StNode *pStNode;

pStNode add(pStNode lastNode, int value)
{
    pStNode newNode = (StNode *) malloc(sizeof(StNode));
    newNode->value = value;

    //[both]=value of previous node pointer if it is last node
    newNode->both = (long)lastNode;

    //calculating previous node [both] value
    lastNode->both = (long)newNode ^ lastNode->both;

    return newNode;
}

pStNode get(pStNode headNode, int index)
{
    pStNode prevNode;
    pStNode currNode;
    long tmp;

    //special handling
    //case: cur=1, prev=0
    //we have set previous node of head node value to be 0 manually
    currNode = (StNode *) ((headNode->both) ^ 0);
    prevNode = headNode;

    //skim through linked list
    for(int i=2; i<=index; i++)
    {
        tmp = (long)prevNode;
        prevNode = currNode;
        currNode = (StNode *)(currNode->both ^ tmp);
    }

    return currNode;
}

int main() {

    //I named first node as headNode, and last node as tailNode
    //create head node with both=0 since there is no previous node to it
    pStNode headNode = (StNode *) malloc(sizeof(StNode));
    headNode->both = 0;
    headNode->value = 2;

    //assign pointers
    pStNode tailNode = headNode;

    //lets add 10 nodes after head, and assign values
    for(int i=3; i<13; i++)
    {
        tailNode = add(tailNode, i);
    }

    //get node value where index=3
    pStNode iNode = get(headNode, 3);
    printf( "result: %d\n",  iNode->value);

    return 0;
}

long both;

Using a long for storing a cast pointer can be both wasteful and too little. Just use the dedicated uintptr_t. Even if your implementation might not be fully up to C99 (MS greets), it probably has <stdint.h> and the typedef.

pStNode newNode = (StNode *) malloc(sizeof(StNode));

The above line demonstrates three bad ideas:

More pieces to keep track of means more complexity, which is bad. Thus hiding pointers behind typedefs is a bad idea, unless that pointer-type is never dereferenced, and is thus the one true type used.

Casting a void* is superfluous and error-prone.
See "Do I cast the result of malloc?
".

Don't use sizeof(TYPE) unless unavoidable. Getting it right even if the types aren't obscured is error-prone. Just use sizeof *pointer.

pStNode get(pStNode headNode, int index)

There is a type uniquely suited as an index: size_t. Alternatively, if you want something signed, you might be interested in ptrdiff_t. Though yes, if your use-case guarantees few enough nodes, int might work.

You really should encapsulate your list somehow. Currently, you are creating a one-off in main().
Also, cleanup should be possible without terminating the program.

If you use at least C99, main() has an implicit return 0;.


Listing sessions within folders in API results I have this code with nested foreach loops that is getting out of hand.

Is there a better and/or cleaner way of doing this?

Basically, what it's doing, is take the results from an API call, and then looping through each layer.

I check to make sure that each layers Results.Length is greater than zero before I start the next loop.

Anyway, here's my crazy ugly mess of code:

parentFolderResponse = GetFolders(parentFolder, sessionMgr, SessionManagementAuth, pagination);

        if (parentFolderResponse.Results.Length > 0)
        {
            foreach (Folder folder in parentFolderResponse.Results)
            {
                Console.WriteLine("\n + Class Of =  " + folder.Name);

                ListFoldersResponse childFolderResponse = GetFolders(folder.Id, sessionMgr, SessionManagementAuth, pagination);
                if (childFolderResponse.Results.Length > 0)
                {
                    foreach (Folder childFolder in childFolderResponse.Results)
                    {
                        Console.WriteLine("\n \t Course Name =  " + childFolder.Name);
                        ListSessionsResponse courseSessionResponse = GetPresentations(sessionMgr, SessionManagementAuth, pagination, childFolder.Id);
                        if (courseSessionResponse.Results.Length > 0)
                        {
                            foreach (Session session in courseSessionResponse.Results)
                            {
                                Console.WriteLine("n \t \t Session Name = " + session.Name);
                            }
                        } else {
                            Console.WriteLine("\n No sessions found!");
                        }
                    }
                } else {
                    Console.WriteLine("\n No course folder(s) found!");
                }

            }
        } else {
            Console.WriteLine("\n No class of folder(s) found!");
        }

If anyone has any suggestions, I'd love to hear them.

There's no reason to check if there are any items in a collection before iterating over it.  If there are no items, iterating over it won't do anything.  So that alone removes half of the indentation, which makes the code fairly manageable.

parentFolderResponse = GetFolders(parentFolder, sessionMgr, SessionManagementAuth, pagination);

foreach (Folder folder in parentFolderResponse.Results)
{
    Console.WriteLine("\n + Class Of =  " + folder.Name);

    ListFoldersResponse childFolderResponse = GetFolders(folder.Id, sessionMgr, SessionManagementAuth, pagination);
    foreach (Folder childFolder in childFolderResponse.Results)
    {
        Console.WriteLine("\n \t Course Name =  " + childFolder.Name);
        ListSessionsResponse courseSessionResponse = GetPresentations(sessionMgr, SessionManagementAuth, pagination, childFolder.Id);

        foreach (Session session in courseSessionResponse.Results)
        {
            Console.WriteLine("n \t \t Session Name = " + session.Name);
        }
        if (courseSessionResponse.Results.Length == 0)
        {
            Console.WriteLine("\n No sessions found!");
        }
    }
    if (childFolderResponse.Results.Length == 0)
    {
        Console.WriteLine("\n No course folder(s) found!");
    }
}
if (parentFolderResponse.Results.Length == 0)
{
    Console.WriteLine("\n No class of folder(s) found!");
}

You could also consider breaking the code up into several methods, to cover each of the logical objects being printed, rather than doing the whole thing all together.  This is more useful if you're doing more stuff at each level, rather than just a conditional and unconditional console write at each tier, but it helps remove indenting, and is more useful the more tiers there are:

parentFolderResponse = GetFolders(parentFolder, sessionMgr, SessionManagementAuth, pagination);

foreach (Folder folder in parentFolderResponse.Results)
{
    PrintClass(folder);
}
if (parentFolderResponse.Results.Length == 0)
{
    Console.WriteLine("\n No class of folder(s) found!");
}

void PrintClass(Folder folder)
{
    Console.WriteLine("\n + Class Of =  " + folder.Name);

    ListFoldersResponse childFolderResponse = GetFolders(folder.Id, sessionMgr, SessionManagementAuth, pagination);
    foreach (Folder childFolder in childFolderResponse.Results)
    {
        PrintCourse(childFolder);
    }
    if (childFolderResponse.Results.Length == 0)
    {
        Console.WriteLine("\n No course folder(s) found!");
    }
}

void PrintCourse(Folder childFolder)
{
    Console.WriteLine("\n \t Course Name =  " + childFolder.Name);
    ListSessionsResponse courseSessionResponse = GetPresentations(sessionMgr, SessionManagementAuth, pagination, childFolder.Id);

    foreach (Session session in courseSessionResponse.Results)
    {
        Console.WriteLine("n \t \t Session Name = " + session.Name);
    }
    if (courseSessionResponse.Results.Length == 0)
    {
        Console.WriteLine("\n No sessions found!");
    }
}

Pulling out the printing of sessions seemed...not useful.


Thread pool implementation As a kind of test of some newer features of C++ I implemented a thread pool. I'm aware that there are probably libraries out there that already have thread pools implemented but this was just as a small test for myself. I'm looking for opinions on my implementation as well as suggestions on how to improve it if possible.

The current intention of it is just to have a simple way to add functions that should be executed to the pool without having to worry about the overhead of starting a new thread each time.

Here's the class:

typedef std::function<void(void)> work_function;

class ThreadPool {
  private:
    bool _running;
    std::queue<work_function> _work_queue;
    std::atomic<size_t> _num_work;
    std::mutex _mutex;
    std::vector<std::thread> _threads;
  public:
    ThreadPool(size_t num_threads = std::thread::hardware_concurrency());
    virtual ~ThreadPool();
    void addWork(work_function work);
    void clearWork();
    void wait();
};

An here's the implementation:

ThreadPool::ThreadPool(size_t num_threads) :
    _running(true), _num_work(0) {
  auto thread_loop = [&](size_t id) {
    while (_running) {
      _mutex.lock();
      if (!_work_queue.empty()) {
        auto work = _work_queue.front();
        _work_queue.pop();
        _mutex.unlock();
        work();
        _num_work--;
      } else {
        _mutex.unlock();
        std::this_thread::yield();
      }
    }
  };
  _threads.reserve(num_threads);
  for (size_t i = 0; i < num_threads; i++) {
    _threads.push_back(std::thread(thread_loop, i));
  }
}

ThreadPool::~ThreadPool() {
  _running = false;
  for (std::thread& t : _threads) {
    t.join();
  }
}

void ThreadPool::addWork(work_function work) {
  _mutex.lock();
  _work_queue.push(work);
  _num_work++;
  _mutex.unlock();
}

void ThreadPool::clearWork() {
  std::queue<work_function> empty;
  _mutex.lock();
  _num_work -= _work_queue.size();
  std::swap(_work_queue, empty);
  _mutex.unlock();
}

void ThreadPool::wait() {
  while (_num_work.load() > 0) {
    std::this_thread::yield();
  }
}

Please excuse my brevity, I'm typing on a phone.

Naming

The rules regarding use of leading underscore in identifiers are a bit complex (https://stackoverflow.com/a/228797/2498188). Although you do not have undefined behavior, I do generally recommend that one avoids leading underscore on identifiers as not everyone knows the rules that well.

Consider using std::packaged_task

While your code can be used with any freestanding function without arguments and no return type, by using packaged task you can use any function with bind and you can get a std::future for the return value or synchronisation of the result. It would make your code more generally usable.

Synchronisation

The value _num_work is never used outside of a mutex look and does thus not have to be atomic, in fact you do not need it at all as you can just use the size() method on the queue.

Edit: I see now that it counts the number of tasks including the ones "in flight", I don't really see how this is useful if you don't provide a method for separately querying the number in flight and the number queued.

Effectivity

Note that you do call yield in your thread loop, this doesn't make the thread wait, it only informs the OS that some other thread can run in its place. If there is no other thread to run, it will run again at the top of your loop essentially pegging all of your CPUs at 100% even without any tasks in the queue. You need to use a std::condition_variable to block your worker threads when there is no work to do, and wake them when work arrives.

Desired methods

You're missing don't methods that are typically desirable, like for example the ability to terminate the thread pool with and without waiting for all jobs to complete.

Finally as a reference I offer up my pool in a previous question: Thread pool worker implementation


Most common letter in string Problem: Write a method that takes in a string. Your method should return the most common letter in the array, and a count of how many
  times it appears.

Note: I solved it with Ruby, but the solution will be almost the same with  other OOP language which have arrays and hashes.

Solution:

Split the string into an array of its characters/letters.
Put each char in a hash as key and starting value = 1.
When you find a character again, increase its value by 1.
Iterate your hash to find the most common character.

My solution works, but, is there a simpler & better way to do it?

What are the disadvantages of my solution?

def most_common_letter(string)
    hash = {}
    arr = string.split("")
    i = 0
    ge = "" # the char which occurs the most times in string
    max = 0 # number of times ge occurs in string
    while i < arr.length
        e = arr[i]
        if hash.has_key? e
            hash[e] += 1
        else
            hash[e] = 1
        end
        i = i + 1
    end

    # Find most common character
    hash.each do |k,v|
        if(v > max)
            max = v
            ge = k
        end
    end
    return ge.to_s + ":" + max.to_s
end

#Test
puts most_common_letter("abca")
puts most_common_letter("g")

while i < arr.length

In your loop, we are only accessing the current element and not doing any index arithmetic.  Use arr.each{}.

    if hash.has_key? e
        hash[e] += 1
    else
        hash[e] = 1
    end

When we encounter a key, we just want to increment by 1.  If a key doesn't exist, use the default value and increment by 1.  You can initialize your hash to have a default value of 0 by hash = Hash.new(0).

# Find most common character
hash.each do |k,v|
    if(v > max)
        max = v
        ge = k
    end
end

This can simply be written using hash.max_by{} and it does the work for you of returning a key-value pair.

Putting it all together, you get

def most_common_letter(string)
  hash = Hash.new(0)
  string.split('').each{ |ch| hash[ch] += 1 }
  hash.max_by{ |ch,count| count }
end

And if you want to clean it up so its a single chained statement.

def most_common_letter(string)
  string.split('')
        .each_with_object(Hash.new(0)) { |ch,hash| hash[ch] += 1 }
        .max_by{ |ch,count| count }
end

  What are the disadvantages of my solution ?

The biggest issue is that it only returns 1 letter/count pair.  What happens if you call most_common_letter("aabb").  Your function returns the pair that is first encountered that sets max {'a', 2} rather than {{'a', 2}, {'b', 2}}.  If you want to return all of the max pairs, build your count hash, find the max value, then return all elements from your hash that has that max value using Hash#select{}.


Splitting a string by spaces without space-splitting elements in double-quotes I need to split a command like:

r'  "C:\Program Files (x86)\myeditor" "$FILEPATH" -n$LINENO "c:\Program Files" -f$FILENAME -aArg2'`

into:

['"C:\\Program Files (x86)\\myeditor"',
 '"$FILEPATH"',
 '-n$LINENO',
 '"c:\\Program Files"',
 '-f$FILENAME',
 '-aArg2']

That is, I want to split by spaces, but avoid splitting the elements in double-quotes.

I have this code:

import re

s = r'  "C:\Program Files (x86)\myeditor" "$FILEPATH" -n$LINENO "c:\Program Files" -f$FILENAME -aArg2'

start = end = 0
split = []

for elem in re.findall('".*?"', s):
    end = s.find(elem)
    split.append(s[start:end])
    split.append(elem)
    start = end + len(elem)

split.extend(s[start:].split())
split = [elem.strip() for elem in split]
split = list(filter(None, split))

It works, but I'm wondering if there's some more elegant/shorter/more readable way to do that in Python(3) ?

The best way to do what you want with the standard library would be shlex.split():

>>> import shlex
>>> s = r'  "C:\Program Files (x86)\myeditor" "$FILEPATH" -n$LINENO "c:\Program Files" -f$FILENAME -aArg2'
>>> shlex.split(s)
['C:\\Program Files (x86)\\myeditor', '$FILEPATH', '-n$LINENO', 'c:\\Program Files', '-f$FILENAME', '-aArg2']

Note that the quotes are not retained.


Most efficient way to find spatial order from a list of tuples (Python) I have a circle-growth algorithm (line-growth with closed links) where new points are added between existing points at each iteration.

The linkage information of each point is stored as a tuple in a list. That list is updated iteratively.

QUESTIONS:

What would be the most efficient way to return the spatial order of these points as a list ?
Do I need to compute the whole order at each iteration or is there a way to cumulatively insert the new points in a orderly manner into that list ?

All I could come up with is the following:

tuples = [(1, 4), (2, 5), (3, 6), (1, 6), (0, 7), (3, 7), (0, 8), (2, 8), (5, 9), (4, 9)]

starting_tuple = [e for e in tuples if e[0] == 0][0]
## note: 'starting_tuple' could be either (0, 7) or (0, 8), starting direction doesn't matter

order = [starting_tuple[0], starting_tuple[1]]
## order will always start from point 0

idx = tuples.index(starting_tuple)
## index of the starting tuple

def findNext():
    global idx
    for i, e in enumerate(tuples):
        if order[-1] in e and i != idx:
            ind = e.index(order[-1])
            c = 0 if ind == 1 else 1
            order.append(e[c])
            idx = tuples.index(e)

for i in range(len(tuples)/2):
    findNext()

print order

It is working but it is neither elegant (non pythonic) nor efficient.
It seems to me that a recursive algorithm may be more suitable but unfortunately I don't know how to implement such solution.

Also, please note that I'm using Python 2 and can only have access to full python packages (no numpy).

This question has also been posted on SO.

No need for recursion. You may want to first convert the tuples to a dict to make it more readable. Then iterate over the dict to construct an ordered list.

In terms of efficiency (or time / space complexity), your code is \$O(n^3)\$ in time and \$O(1)\$ in auxiliary space. Note that idx = tuples.index(e) is not necessary at all, since tuples.index(e) == i. Making use of this would allow your code to be \$O(n^2)\$ in time. The most time-efficient solution is \$O(n)\$, which is also the time complexity of the proposed solution involving a dict. However, the auxiliary space complexity of that solution is \$O(n)\$ -- inferior to your original approach.

If you want to update the order after obtaining a new tuples list, you can keep the dict and iterate over the new tuples, comparing with values in the dict to see if there is any change. However, the efficiency of this approach would probably be in most cases worse than constructing a new dict from scratch.

from collections import defaultdict

def tuples_to_neighbors_dict(tuples):
  """
  Covert `tuples` to a dict mapping each point to a list of its neighbors.
  """
  neighbors = defaultdict(list)

  for (a,b) in tuples:
    neighbors[a].append(b)
    neighbors[b].append(a)

  return neighbors

def tuples_to_order(tuples, start=0):
  """
  Covert `tuples` to a list of points.
  """
  neighbors = tuples_to_neighbors_dict(tuples)
  order = []

  prev = None
  current = start

  while current != start or prev is None:
    # add the current value to the list
    order.append(current)

    # move to the next -- pick the neighbor which we haven't visited yet
    neigh = neighbors[current]
    new = neigh[1] if neigh[0] == prev else neigh[0]
    prev = current
    current = new

  return order

EDIT   I just now looked at the SO question and noticed that one answer is almost identical to mine


Convert IEnumerable to CSV I have an IEnumerable of a view model that I need to convert to CSV and send back as a file response. I have written it out quite manually and I'm just seeking code review and suggestions on a more elegant solution, or alternatively if this looks acceptable, confirmation of that:

    public ActionResult ExportBasicReads(string nmi, DateTime fromDate, DateTime toDate)
    {
        var reads = _dataService.GetBasicReads(nmi, fromDate, toDate)
                    .Where(r => r.CurrentReadQuality != ReadQuality.Estimate)
                    .OrderBy(r => r.CurrentReadDate)
                    .ThenBy(r => r.DeviceKey)
                    .ThenBy(r => r.ValidationCode);

        StringBuilder csv = new StringBuilder();

        csv.AppendLine("Read Date,Device : Suffix,Read,Quality,Tran Code,Service Order,Previous Read Date,Previous Read,Previous Quality," +
            "Previous Tran Code,Dial Diff,Quantity,UOM,Dial Factor,Direction,Validation,From Date,To Date,Kwh,Avg Daily Load,Profile Name," +
            "Profle Area,Reading Timestamp,Update Date,ChangedBy,ChangeType,ChangeComment");

        reads.ForEach(x => csv.AppendLine(
                string.Format("{0},{1} : {2},{3},{4},{5},{6},{7},{8},{9},{10},{11},{12},{13},{14},{15},{16},{17},{18},{19},{20},{21},{22}," +
                "{23},{24},{25},{26},{27}",
                    x.CurrentReadDate,
                    x.DeviceKey,
                    x.Suffix,
                    x.CurrentReadRawValue,
                    x.CurrentReadQualityDesc,
                    x.CurrentReadTransactionCode,
                    x.CurrentServiceOrder,
                    x.PreviousReadDate,
                    x.PreviousReadRawValue,
                    x.PreviousReadQualityDesc,
                    x.PreviousReadTransactionCode,
                    x.DialDifference,
                    x.Quantity,
                    x.UOMDesc,
                    x.Multiplier,
                    x.DirectionDesc,
                    x.ValidationDesc,
                    x.EffectiveFromDateTime,
                    x.EffectiveToDateTime,
                    x.Kwh,
                    x.AverageDailyLoad,
                    x.ProfileName,
                    x.ProfileArea,
                    x.EffectiveFromDateTime,
                    x.UpdateTimestamp,
                    x.ChangedBy,
                    x.ChangeType,
                    x.ChangeComment
                )
            ));

        return File(Encoding.ASCII.GetBytes(csv.ToString()), "text/csv", "BasicReadsExport.csv");
    }

I modified the formatting part for better readability and maintainability, at the cost of lower performance:

var columns = new Dictionary<string, Func<Reads, object>>
{
    ["Read Date"] = x => x.CurrentReadDate,
    ["Device"] = x => x.DeviceKey,

    ["Suffix"] = x => x.Suffix,
    ["Read"] = x => x.CurrentReadRawValue,
    // add the rest of headers/property selectors below
};
//var format = "{0},{1} : {2},{3},{4},{5},{6},{7},{8},{9},{10},{11},{12},{13},{14},{15},{16},{17},{18},{19},{20},{21},{22},{23},{24},{25},{26},{27}";
var format = "{0}{1} : " + string.Join(",", columns.Select((_, i) => $"{{{i}}}").Skip(2));

var csv = new StringBuilder()
    .AppendFormat(format, columns.Keys.ToArray())
    .AppendLine();
foreach (var read in reads)
{
    var values = columns.Values.Select(selector => selector(read));
    csv
        .AppendFormat(format, values.ToArray())
        .AppendLine();
}

The advantage of this approach is that you can add/move/remove the columns without having to worry about updating the header and the format, and if they are kept in sync or not.

Note:

You can also revert to the constant format if it becomes more complex.
The property selector can be chained if you need special format on individual property, like: x => x.CurrentReadDate.ToString("yyyyMMddThhmm")


X86_64 implementation of STRCHR using NASM As this will eventually become part of an operating system I'm developing, there is no specific adherence to any particular ABI, even though this is being developed and tested on Ubuntu 16.04. My OS will however adopt a SYSTEM V paradigm insomuch as passing values by register, but they will be passed in the one most suitable to the algorithm.
One of my methodologies is to utilize as many bits of the register as possible, especially when 16 or less are significant. I have found too, that in a lot of cases this can simplify calling process. It does however complicate things a little ie:
    mov     rdi, Pointer_to_String
    mov     ecx, 784 << 8 | '.'
    call    strlen

This would traverse the buffer pointed to by RDI for the first occurence of period for a maximum of 784 characters. This snippet had evolved from just scanning for NULL for a maximum of FFFFFFFFH bytes, to scanning for any of the values (0 - FF), to traversing buffer for specified number of bytes.
Question
Do you think I've achived the maximum utility with the least amount of code.
 ; Determine length, including terminating character EOS. Result may include
 ; VT100 escape sequences.

 ;        ENTER: RDI = Pointer to ASCII string.
 ;               RCX   Bits 31 - 08 = Max chars to scan (1 - 1.67e7)
 ;                          07 - 00 = Terminating character (0 - FF)

 ;        LEAVE: RAX = Pointer to next string (optional).

 ;        FLAGS:  ZF = Terminating character found, NZ otherwise (overrun).
 ;                DF = Unmodified incase it was already set.

strlen:  pushf                       ; Preserve DF (Direction flag)
         push    rcx                 ; Preserve registers used by proc so
         push    rdi                 ; its non-destructive except for RAX.

         mov      al, cl             ; Byte to scan for in AL.
         shr     ecx, 8              ; Shift max count into bits 23 - 00

         std                         ; Auto decrement.
         repnz   scasb               ; Scan for AL or until ECX = 0
         mov     rax, rdi            ; Return pointer to EOS + 1

         pop     rdi                 ; Original pointer from proglogue
         jz      .exit               ; ZF indicates EOS was found
         mov     rax, rdi            ; RAX = RDI, NULL string

    .exit:
         pop     rcx
         popf                        ; Restore direction flag.
         ret

Bug #1
You set the DF flag, so your string operation is going to run backwards.  You should clear the DF flag to scan your string in the forwards direction.
Bug #2
If you reach the byte limit, your function will return the original string pointer in RAX.  The comment says it should return NULL.  Perhaps you meant xor rax, rax instead of mov rax, rdi?
Bug #3
According to the comments, your function should return with ZF set according to the result of the scan.  However, since you do pushf and then popf, the ZF result is wiped out by the popf.
Your own ABI
I would suggest following the standard X64 ABI rather than inventing a new ABI per function.  For one thing, you will be able to use high level languages to interface with your assembly routines.  Another thing is that it would be hard for you to remember details such as "when I call strlen, ZF set means that the terminating character was found".
Don't call it strlen
Your function is closer to strchr or strnchr than strlen.  Any reasonable person would expect a function called strlen to return a length rather than a pointer.


Parse.com simple query: login and display account balance I was recently at a hackathon and saw quite a few people using Parse, so I decided to check it out and read some guides online. Can someone take a look at my code? I followed Parse documentation so it should be fairly readable. I'm trying to get the balance for the user when they log in and display it.

Parse.User.logIn("user", "pass", {
  success: function(user) {
    query.find({
      success: function(results) {
        results[0].save({ key: value }, {
          success: function(result) {

            var arr = result.balances;
            var balance = sumArray(arr);
            var charge = result.charge;

            if(balance < charge)
                document.write("Insufficient funds. Must have " + balance + charge + " balance left");
            else if(balance == charge)
                document.write("Oops!");
                document.write("Sorry, balance cannot reach zero");

            var mssg = "";
            var today = formatDate(new Date());

            for(var i=0; i<arr.length; i++) {
                mssg += arr[i] + " ,";
            }
            document.write(today - 1 + 1);
            document.write(mssg);

          },
          error: function(result, error) {
            console.log(error);
          }
        });
      },
      error: function(error) {
        console.log(error);
      }
    });
  },
  error: function(user, error) {
    console.log(error);
  }
});

function sumArray(arr) {
    var S = 0;
    for(var i=0; i<arr.length; i++){
        S += arr[i];
    }
    return S;
}

$(document).ready(function () {
    $("#dialog-confirm").dialog({
        autoOpen: false,
        height: 'auto',
        width: '400px',
        modal: true,
        buttons: {
            Cancel: function () {
                $(this).dialog("close");
            }
        }
    });

    $(function () {
        $("#form").datepicker({
            maxDate: '-18y'
        });
        $("#form").mask("99/99/9999", { placeholder: "mm/dd/yyyy" });
    })
});

function formatDate(date) {
    var d = new Date(date);
    var day = '' + d.getDate();

    return day;
}

Promises

Promises are the next big thing in JavaScript. You can read more about them here. Luckily, Parse supports Promises and you should make good use of them.
Essentially, Promises will replace the messy callback pyramid structure you have here

Parse.User.logIn("user", "pass", {
  success: function(user) {
    ...
      success: function(results) {
        ...
          success: function(result) {
            ...
          },
          error: function(result, error) {
            // error
          }
        });
      },
      error: function(error) {
        // error
      }
    });
  },
  error: function(user, error) {
    // error
  }
});

Promises use the then function, which can be given callbacks to be called when the promise is fulfilled or has failed. Promises can also propogate errors, not calling any callbacks until an error handler is encountered. So, your code can be re-written as:

Parse.User.logIn("user", "pass").then(function(user) {
  return query.find();
}).then(function(results) {
  // ...
}).then(function(result) {
  // ...
}, function(error) {
  // error
});

One thing to remember that a successful promise does not imply the Parse Query itself was successful. A successful promise simply means your query was successfully executed, regardless of what value was returned by the query. So, make sure to check the return value from Parse:

.then(function(result)){   // Promise succeeded
  if(!result){             // check if return is not null
    // do something
  }
})

 Duplicated event handling

You have this

$(document).ready(function () {

and then you also have this

$(function () {

Why? Just put all the content inside the .ready function and you are good to go

Beware of Javascript string concatenation

This is one of the most naive mistakes:

document.write("Insufficient funds. Must have " + balance + charge + " balance left");

Let's assume balance=10 and charge=20. You expect it to print :

Insufficient funds. Must have 30 balance left

What it will actually print is:

Insufficient funds. Must have 1020 balance left

String concatenation in javascript is not well-defined when types get mixed. In your case string + int types are getting mixed. What you need to do instead is:

document.write("Insufficient funds. Must have " + (balance + charge) + " balance left");

Notice the parantheses besides the int types.

Mixing logic and view

You are mixing your logic. Look here

document.write(mssg);

Code that is rendering the view such as document.write should be handled in a separate function. It becomes very messy and hard to debug when you combine your logic-side and view-side code in one place. Read up on MVC (Model-View-Controller).

Illusion of binding

Take a look here why dropping brackets and misleading indentation can have unforeseen consequences

if(balance < charge)
  document.write("Insufficient funds. Must have " + b + ch + " balance left");
else if(balance  == charge)
  document.write("Oops!");
  document.write("Sorry, balance cannot reach zero");

At what times do you think "Sorry, balance cannot reach zero" message will be printed? Will it be printed only when balance == charge? Unfortunately, no! It will be printed every single time!

Why? Because your indentation leads to you believe that the 2nd document.write statement is bound to the else clause, when infact it isn't. Dropping brackets might make code look more clean & readable, but make sure you don't let the indentation give you false illusion of binding.

Avoid unnecessary string creation

In Javascript, strings are immutable. When you do something like this

var mssg = "";
for(var i=0; i<arr.length; i++) {
    mssg += arr[i] + " ,";
}

it isn't "modifying" the mssg string, but instead creating a new string on every iteration. For large iterations, say a million, you will definitely have a performance hit. Other languages like C# solve this problem by using a StringBuilder class, but for Javascript you can use the join() method instead

var mssg = arr.join(", ");

Incorrect code

document.write(today - 1 + 1);

Why are you doing this? What is the purpose of -1 + 1?
To me, it seems like you are trying to convert the date to an int. But you wouldn't need to do that if you hadn't done this earlier

var day = '' + d.getDate();

Don't add the empty string to the date in the first place. Just leave it as this

var day = d.getDate();

 Underscore.js

Naive summation operations such as

function sumArray(arr) {
    var S = 0;
    for(var i=0; i<arr.length; a++){
        S += arr[i];
    }
    return S;
}

can be replaced by powerful helper functions provided by the Underscore.js library. Using reduce() function, we can re-write it as follows:

// return sum of all elements of an array
var sum = _.reduce(arr, function(memo, num){ return memo+ num; }, 0);
return sum;

Lacking comments

Please always comment your code.


Is this the best approch to combine 2 Interfaces? I have a use case where I want to use both HttpRequestInterceptor and HttpResponseInterceptor in one class.

I'm asking if the following would be a good solution and what are the potential problems?

public interface HttpRequestResponseInterceptor extends HttpRequestInterceptor, HttpResponseInterceptor {}

Then I implement this interface:

public class AuthenticatingResponseInterceptor implements HttpRequestResponseInterceptor{

  private static final String TAG = AuthenticatingResponseInterceptor.class.getSimpleName();

  @Inject
  private Authenticator authenticator;
  @Inject
  private CookieStore cookieStore;
  @Inject
  private RequestCache requestCache;

  @Override
  public void process(HttpResponse httpResponse, HttpContext httpContext) throws HttpException, IOException {
    //Implementation
  }

  @Override
  public void process(HttpRequest request, HttpContext context) throws HttpException, IOException {
    //Implementation
  }
}

The part I am unsure about it is when I pass this into 2 separate methods:

@Inject
private HttpRequestResponseInterceptor interceptor;

httpClient.addResponseInterceptor(interceptor);
httpClient.addRequestInterceptor(interceptor);

I should probably point out that the HttpRequestResponseInterceptor is a singleton. Is this a good solution?

You don't necessarily need to have another interface that extends the two you want to combine, you just have to say that he class implements both of them.  One java class can implement as many interfaces as you desire.

Problems may arise if you have any shared data problems, but by saying you are implementing the interface you are guaranteeing that the class has the specified functions.

import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class MyClass{

    public interface Combined extends Runnable, Callable<Object> {}

public class Mixed implements Runnable, Callable<Object> {

    @Override
    public Mixed call() throws Exception {
        System.out.println("Called Mixed");
        return this;
    }

    @Override
    public void run() {
        System.out.println("Ran Mixed");
    }

}

public class Single implements Combined{
    @Override
    public Single call() throws Exception {
        System.out.println("Called Single");
        return this;
    }

    @Override
    public void run() {
        System.out.println("Ran Single");
    }
}

/**
 * @param args
 */
public static void main(String[] args) {
    MyClass top = new MyClass();
    Mixed mixed = top.new Mixed();
    Single single = top.new Single();
    ExecutorService es = Executors.newFixedThreadPool(1);

    es.execute(mixed);
    es.execute(single);

    es.submit((Callable<Object>) mixed);
    es.submit((Callable<Object>) single);

}

}

Output is:

Ran Mixed
Ran Single
Called Mixed
Called Single


Compile-time list in C++ Everything is evaluated in compile-time. You will need a C++14 compiler. The compilation time is quite long, and with a bigger list input, you'll get some error messages like constexpr evaluation hit maximum step limit.

#include <iostream>
#include <limits>
#include <initializer_list>

template<typename T>
class List
{
  template<typename T2>
  friend std::ostream &operator<<(std::ostream &, const List<T2> &);
public:
  constexpr List();
  constexpr List(std::initializer_list<T>);
  constexpr T head() const;
  constexpr List<T> tail() const;
  constexpr List<T> add(T) const;
  constexpr List<T> merge(List<T>) const;
  constexpr List<T> reverse() const;
  template<typename Filter>
  constexpr List<T> filter(Filter) const;
  constexpr List<T> sort() const;
  constexpr T sum() const;
private:
  int length;
  T array[std::numeric_limits<int>::max() >> 2];
};

template<typename T>
constexpr List<T>::List()
: length {0}
, array {0}
{
}

template<typename T>
constexpr List<T>::List(std::initializer_list<T> l)
: length {static_cast<int>(l.size())}
, array {0}
{
  for (auto it = l.begin(); it != l.end(); ++it)
  {
    array[it - l.begin()] = *it;
  }
}

template<typename T>
constexpr T List<T>::head() const
{
  return array[0];
}

template<typename T>
constexpr List<T> List<T>::tail() const
{
  List<T> l;
  l.length = length - 1;
  for (int i = 0; i < l.length; ++i)
  {
    l.array[i] = array[i + 1];
  }
  return l;
}

template<typename T>
constexpr List<T> List<T>::add(T t) const
{
  List<T> l {*this};
  l.array[l.length++] = t;
  return l;
}

template<typename T>
constexpr List<T> List<T>::merge(List<T> l) const
{
  for (int i = l.length - 1; i >= 0; --i)
  {
    l.array[i + length] = l.array[i];
  }
  for (int i = 0; i < length; ++i)
  {
    l.array[i] = array[i];
  }
  l.length += length;
  return l;
}

template<typename T>
constexpr List<T> List<T>::reverse() const
{
  List<T> l;
  l.length = length;
  for (int i = 0; i < l.length; ++i)
  {
    l.array[i] = array[length - i - 1];
  }
  return l;
}

template<typename T>
template<typename Filter>
constexpr List<T> List<T>::filter(Filter f) const
{
  List<T> l;
  for (int i {0}; i < length; ++i)
  {
    if (f(array[i]))
    {
      l = l.add(array[i]);
    }
  }
  return l;
}

template<typename T>
struct LT
{
  T pivot;
  constexpr bool operator()(T t) const
  {
    return t < pivot;
  }
};

template<typename T>
struct GE
{
  T pivot;
  constexpr bool operator()(T t) const
  {
    return t >= pivot;
  }
};

template<typename T>
constexpr List<T> List<T>::sort() const
{
  if (length == 0)
  {
    return *this;
  }
  return tail().filter(LT<T> {head()}).sort().add(head())
  .merge(tail().filter(GE<T> {head()}).sort());
}

template<typename T>
constexpr T List<T>::sum() const
{
  if (length == 0)
  {
    return T {};
  }
  return head() + tail().sum();
}

template<typename T>
std::ostream &operator<<(std::ostream &os, const List<T> &l)
{
  os << '{';
  for (int i {0}; i < l.length - 1; ++i)
  {
    os << l.array[i] << ", ";
  }
  return os << l.array[l.length - 1] << '}';
}

inline constexpr List<int> range(int a, int b, int c = 1)
{
  List<int> l;
  while (a < b)
  {
    l = l.add(a);
    a += c;
  }
  return l;
}

int main()
{
  constexpr std::size_t n = range(0, 300).reverse().sort().sum();
  std::cout << n << std::endl;
}

I confess to not being entirely clear as to the purpose of this code.  It seems to me that any list known at compile time could more easily and more correctly handled by other forms of preprocessing.  With that said, though, here are some comments on this code.

Think about real machines

The code for the List class currently includes this data member:

  T array[std::numeric_limits<int>::max() >> 2];

On my machine, this attempts to allocate 2GB on the stack for an int-based List. That's a rather large amount of memory to attempt to allocate for every List!  Perhaps this could be trimmed to some reasonable value.

Reduce the number of requirements on template types

The code requires a number of operations on the underlying T type.  It requires both < and >= but that requirement can be relaxed somewhat by defining GE's operator like this:

constexpr bool operator()(T t) const {
    return !(t < pivot);
}

Pass const references where practical

Rather than passing by value, it would probably make more sense in the general case to pass by const reference.  So for example, the previous function could be written like this instead:

constexpr bool operator()(const T &t) const {
    return !(t < pivot);
}

In fact, one of the only places that can't be treated this way is the argument to merge() which must be passed by value.

Try it with a non-primitive type

Whenver you're creating templated code, carefully consider just how it might be used and what requirements it may demand of the underlying type.  One technique I use is to wrap a primitive type inside a goofy minimalistic wrapper for testing:

template <typename T>
class Goofy
{
public:
    constexpr Goofy(T n = 0) : num(n) { }
    constexpr Goofy(const Goofy &g2) : num(g2.num) { }
    constexpr Goofy &operator=(const Goofy &g2) { num = g2.num; return *this; }
    constexpr Goofy &operator+=(const Goofy &g2) { num += g2.num; return *this; }
    constexpr bool operator<(const Goofy &g2) const { return num < g2.num; }

    friend std::ostream &operator<<(std::ostream &out, const Goofy &g2) {
        return out << g2.num;
    }
private:
    T num;
};

template <typename T>
constexpr Goofy<T> operator+(const Goofy<T> &g1, const Goofy<T> &g2)
{
    Goofy<T> result(g1);
    result += g2;
    return result;
}

This represents a minimal interface that can then be tested with your template:

int main()
{
  constexpr List<Goofy<float>> list{0, 5, 8, 13, 1, 7};
  std::cout << list << std::endl;
  std::cout << list.reverse() << std::endl;
  std::cout << list.sort() << std::endl;
  constexpr auto m = list.sum();
  std::cout << m << std::endl;
}

Sample output

{0, 5, 8, 13, 1, 7}
{7, 1, 13, 8, 5, 0}
{0, 1, 5, 7, 8, 13}
34


The largest odd number (follow-up code using nested functions) Ask user to input 10 integers and then print the largest odd number that was entered. If no odd number was entered, print a message to that effect.

This is my follow-up code on feedback from Find the largest odd number (follow-up code)

This time I'm trying to achieve the same result with nested functions. I need your comments please. Also, please note that I'm working through a book and it's meant to be used with Python 2.x

def find_largest_odd():
    numbers_entered = []

    def get_ten_ints():
        while len(numbers_entered) < 10:
            try:
                number = int(raw_input('Enter an integer: '))
                numbers_entered.append(number)
            except ValueError:
                print 'That was not an integer!'

    get_ten_ints()

    try:
        largest_odd_number = max(item for item in numbers_entered if item % 2)
        return 'The largest odd number entered was {}.'.format(largest_odd_number)
    except ValueError:
        return 'No odd number was entered.'

Since your first attempt, you are indeed showing progress, and it's kind of fun to watch. When entering into the realm of using functions the ball game changes a little, and you need to start thinking of segregation of duties and possible whether you can reuse one or more of the functions.

Some more or less general cases, addressed toward your code:

Make functions general – Allow for the input function to enter an arbitrary number of integers, i.e. rename/repurpose the get_ten_ints() function
Be wary of global variables – Do not use global variables if they can be avoided. This also applies to using functions and variables defined within the function (i.e. the numbered_entries variable)
Try avoiding magic numbers – In your code the 10 stands out as kind of a magic number, which do affect the running of your code, but where does it come from? If you want to have such numbers, they are better declared as constants in top of the file (or function)
Don't use nested functions, unless there is a real good reason – To me, I don't see why the get_ten_ints() is a nested function. I would propagate it to the top level of your code
Start documenting your code – A common practice is to use docstrings to document your function, so that it is clearer when you return to your code what it was supposed to do
Separate presentation from business logic – Decide if a function is supposed to present stuff or return stuff. Your original code had find_largest_odd() both find this number, and present it. In such a small functions that could be OK, but it's better to make it a clean function returning the number, and then letting the caller decide what it wants to do with the number, i.e. print it or use it for other calculations
Introducing main() function – The larger the code base the more interesting is it to introduce a pattern which reveals a common entry point to your code. The __name__ == '__main__' test ensures that if you run python on your file you get the execution defined, but still allowing for the file to be imported as a module giving access to your functions
Normalize your print statements – I'm not sure according to which standard, but it's quite normal (both in python 2.x and 3.y) to use constructs like print("my text: {}".format(some_variable))

Applying all of these principles you could end up with something like:

def get_ints(number_of_ints):
    """Read from input exactly number_of_ints numbers, and verify that they
       actually are ints. Return the list of these numbers"""
    numbers_entered = []
    while len(numbers_entered) < number_of_ints:
        try:
            number = int(raw_input("Enter an integer: "))
            numbers_entered.append(number)
        except ValueError:
            print("That was not an integer!")

    return numbers_entered

def find_largest_odd(numbers):
    """From a given list, find the largest odd number. Return None if no odd
       number is found."""
    try:
        largest_odd_number = max(item for item in numbers if item % 2)
        return largest_odd_number
    except ValueError:
        return None

def main():
    """Read 10 integers into a list, and find the largest odd number"""
    my_numbers = get_ints(10)
    the_number = find_largest_odd(my_numbers)
    if the_number != None:
        print("Largest odd number in list: {}".format(the_number))
    else:
        print("No odd number in list")

if __name__ == '__main__':
    main()


Handling bill cancellation I'm trying to refactor the working code below which intends to handle bill cancellation, but doesn't seem "linear" enough.
open FSharpPlus
open FSharpPlus.Data

type BillCancellationReason = | A | B
type OrderBillingCommand = | A | B
type OrderBillingEvent = | A | B
type CommandError = | A | B

type BillCancellationRequest =
    { BillSequence: int32
      Reason: BillCancellationReason
      Comment: string }

type BillCancellationsRequestPart =
    { OrderId: int32
      BillSequence: int32
      Reason: BillCancellationReason
      Comment: string }

type BillCancellationsRequest = BillCancellationsRequestPart list

module Validation =
    // Validation<'Failure list, 'Success> list -> Validation<'Failure list, 'Success list>
    let cumulativeFoldList (validations: Validation<'Failure list, 'Success> list):
        Validation<'Failure list, 'Success list> =
        traverse id validations

// BillCancellationsRequestPart -> Validation<CommandError list, (int32 * OrderBillingCommand)>
let toCommand (requestPart: BillCancellationsRequestPart) =
    Unchecked.defaultof<Validation<CommandError list, (int32 * OrderBillingCommand)>>

// (int32 * OrderBillingCommand) list -> Async<Result<OrderBillingEvent list, CommandError list>>
let processValidCommands (commands: (int32 * OrderBillingCommand) list) =
    Unchecked.defaultof<Async<Result<OrderBillingEvent list, CommandError list>>>

// BillCancellationsRequest -> Async<Result<OrderBillingEvent list,CommandError list>>
let cancelBills (request: BillCancellationsRequest) =
    async {
        match request |> List.map toCommand |> Validation.cumulativeFoldList with
        | Success commands -> return! processValidCommands commands
        | Failure failure -> return Error failure
    }

In particular this last bit:
// BillCancellationsRequest -> Async<Result<OrderBillingEvent list,CommandError list>>
let cancelBills (request: BillCancellationsRequest) =
    async {
        match request |> List.map toCommand |> Validation.cumulativeFoldList with
        | Success commands -> return! processValidCommands commands
        | Failure failure -> return Error failure
    }

I tried to come up with a new module but I found myself rolling a lot more code:

module Result =
    // ('T -> 'U) -> ('V -> 'W) -> Result<'T, 'V> -> Result<'U, 'W>
    let bimap okMapping errorMapping result =
        result
        |> Result.map okMapping
        |> Result.mapError errorMapping

    // Result<Async<'T>, 'U> -> Async<Result<'T, 'U>>
    let awaitOnOk result =
        async {
            match result with
            | Ok ok ->
                let! awaitedOk = ok
                return Ok awaitedOk
            | Error error ->
                return Error error
        }

// BillCancellationsRequest -> Async<Result<OrderBillingEvent list,CommandError list>>
let cancelBills (request: BillCancellationsRequest) =
    request
    |> List.map toCommand
    |> Validation.cumulativeFoldList
    |> Validation.toResult
    |> Result.map processValidCommands
    |> Result.awaitOnOk
    |> Async.map Result.flatten

I'm wondering if there is like a better and more concise approach to achieve that?
Note: Unchecked.defaultof<...> has been used just for the sake of making the code above to compile.

To be honest, your original implementation of cancelBills looks pretty good - it's very focused and is fairly easy to understand what it's doing even without much context.
If you really want to write the function as a single chain of functions then your alternative version is most of the way there. I'd consider replacing the last three functions in the chain with a single Result.asyncBind function to simplify it somewhat, i.e.
module Result  =
    let asyncBind (asyncBinder: 'T -> Async<Result<'U, 'TError>>) (result: Result<'T, 'TError>) =
        match result with
        | Result.Ok value -> asyncBinder value
        | Result.Error error -> async { return Result.Error error }

This is effectively just an async equivalent of the built-in Result.bind, so shouldn't add much overhead when reading the code and should be fairly re-usable if you're piping Result and Async around a lot. That being said, if you're not going to be re-using Result.asyncBind (or your equivalent function chain) then just stick with your original implementation of cancelBills!


Http Request response time within C# application I have this method :

private string GetCheckResultFor(string url)
        {
            Stopwatch reveil = new Stopwatch();
            reveil.Start();
            HttpWebResponse response = null;
            url = Regex.Replace(url, @"\s+", "");
            if (!url.StartsWith("http") && !url.StartsWith("https")) url = "http://" + url;
            string res = "";
            try
            {
                HttpWebRequest request = (HttpWebRequest)HttpWebRequest.Create(url);
                request.Method = "GET";
                response = (HttpWebResponse)request.GetResponse();
                if (response.ResponseUri.ToString() == url) res = "No Action";
                else res = "Redirect to " + response.ResponseUri.ToString();
            }
            catch (WebException e)
            {
                if (e.Status == WebExceptionStatus.ProtocolError)
                {
                    response = (HttpWebResponse)e.Response;
                    res = "Errorcode: " + (int)response.StatusCode;
                }
                else
                {
                    res = "Error: " + e.Status;
                }
            }
            finally
            {
                if (response != null)
                {
                    response.Close();
                }

            }
            reveil.Stop();
            Debug.WriteLine("For this url : " + url + " = " + reveil.ElapsedMilliseconds + " ms");
            return res;
        }

It takes between 200 and 700 ms. I know it depends on internet connection speed but I need to improve it because I only need the reponse url. I have Three possiblities :

Reponse Url = request Url ==> No action
Redirect to another url
Request Error

So How can I edit this snippet to reduce method response time ?

I found that a webclient with custom settings gave me the fastest result.
Personally I was building a framework for web crawling & scraping, where you want it to do the requests quickly.

Might be an alternative to check out.

public class DefaultWebClient : WebClient {
    public DefaultWebClient() {
        ServicePointManager.ServerCertificateValidationCallback += (sender, certificate, chain, sslPolicyErrors) => true;
        ServicePointManager.DefaultConnectionLimit = Int32.MaxValue;
        ServicePointManager.Expect100Continue = false;
    }

    protected override WebRequest GetWebRequest(Uri address) {
        var request = base.GetWebRequest(address) as HttpWebRequest;

        if (request != null) {
            //This might work in your case, it didnt do the trick for me.
            //request.Method = WebRequestMethods.Http.Head;
            request.Proxy = null;
            request.Timeout = 5000;
            request.AutomaticDecompression = DecompressionMethods.Deflate | DecompressionMethods.GZip;
        }

        return request;
    }
}

This, in combination with retrieving the response url should do the trick.

    protected override WebResponse GetWebResponse(WebRequest request) {
        var response = base.GetWebResponse(request);
        if (response != null) {
            var responseUri = response.ResponseUri;
            // Redirect found.
        }
        else {
            // No Action.
        }

        return response;
    }

If you do a lot of requests to get the redirects, it might be a good idea to raise it as an event.


Is this index selection useless? I'm reviewing some code and I came across this:

// Select the last parameter in the list
if (parametersListBox.SelectedIndex > -1)
{
    parametersListBox.SetSelected(parametersListBox.SelectedIndex, true);
}

ParametersListBox being a C# ListBox. Despite the fact that the comment is wrong, does this actually do something or is this just useless code?

The check is needed, because SetSelected() would throw an ArgumentOutOfRangeException.

See reference source: http://referencesource.microsoft.com/System.Windows.Forms/a.html#fcae9a622a947f54 or read the documentation where it clearly states

Condition for ArgumentOutOfRangeException

  The specified index was outside the range of valid values.


Ruby command-line app that outputs from JSON file I made this application that runs fine, but I feel the code is sloppy as there are many if statements. I'm a beginner so I'm not sure how to improve it, at least not without messing everything up. Any suggestions?

#!/usr/bin/ruby

require 'thor'
require 'json'

class ListTrends < Thor

  desc "list_trends json", "list out trends from JSON file"
  option :api_key, :aliases => "--api-key", :type => :string, :required => true
  option :format, :type => :string, :desc => "one line format"
  option :no_country_code, :aliases => "--no-country-code", :desc => "remove country code", :type => :boolean
  def list_trends(keyword=nil)

    json = File.read('trends_available.json')
    trends_hash = JSON.parse(json)
    re = /^(?=.*[a-zA-Z])(?=.*[0-9]).{8,}$/
    keyword = keyword.to_s.downcase

    if re.match(options[:api_key])

      trends_hash.each do |trend|
        if trend["country"].downcase.include?(keyword)
          if options[:format]
            output = trend.values[0..2]
            output.delete_at(1) if options[:no_country_code]
            puts output.join(" ")
          else
            # Complete output
            trend.each do |k, v|
              if v.is_a?(Hash)
                v.each do |i, j| puts "Trend location type #{i}: #{j}" end
              else
                puts "Trend #{k}: #{v}"
              end
            end # trend.each do
            puts ""
          end # if options[:format]
        end # if trend["country"]
      end # trends_hash.each

    else
      puts "Invalid API Key, operation abort..."
    end # if re.match

  end # list_trends
end

ListTrends.start(ARGV)

The way to deal with nested if's is generally to move code into methods. Another solutions is to use an 'early return'. (if !valid b; return instead of if valid then a else b)

I would write this something like:

#!/usr/bin/ruby

require 'thor'
require 'json'

class ListTrends < Thor

  VALID_API_KEY_RE = /^(?=.*[a-zA-Z])(?=.*[0-9]).{8,}$/

  desc   'list_trends json', 'list out trends from JSON file'
  option :api_key, aliases: '--api-key', type: :string, required: true
  option :format, type: :string, desc: 'one line format'
  option :no_country_code, aliases: '--no-country-code', desc: 'remove country code', type :boolean

  def list_trends(keyword=nil)

    # Check your options before reading the file
    if !VALID_API_KEY_RE.match(options[:api_key])
      puts "Invalid API Key, operation abort..."
      exit(255)
    end

    json        = File.read('trends_available.json')
    trends_hash = JSON.parse(json)
    keyword     = keyword.to_s.downcase

    trends_hash.each do |trend|
      process_trend(trend)
    end
  end

  private

  def process_trend(trend)
    return unless trend["country"].downcase.include?(keyword)

    if options[:format]
      output_formatted_trend(trend)
    else
      output_complete_trend(trend)
    end
  end

  def output_formatted_trend(trend)
    output = trend.values[0..2]
    output.delete_at(1) if options[:no_country_code]
    puts output.join(' ')
  end

  def output_complete_trend(trend)
    trend.each do |k, v|
      if v.is_a?(Hash)
        v.each do |i, j| puts "Trend location type #{i}: #{j}" end
      else
        puts "Trend #{k}: #{v}"
      end
      puts ''
    end
  end

end

ListTrends.start(ARGV)

Another advantage being that most people can only understand a few lines of code at a time, short methods also make it easier to keep track of variable scope and giving them meaningful names makes it easier to understand code without needing comments.


play_card() method in an Exploding Kittens card game I wrote a library for card games.  To demonstrate how it can be used, I also wrote an Exploding Kittens card game as an example.

I'm using CodeClimate to determine cyclomatic complexity. This method in the Exploding Kittens game has complexity too high (6):

def play_card(self, card: KittenCard, player: Player = None, target: Player = None):
    if card.selfcast and player is None:
        raise Exception("You must pass a player who owns the card!")
    if card.targetable and target is None:
        raise Exception("You must pass a target!")
    if not self.ask_for_nope():
        card.effect(player, target)
    else:
        print("Card was noped :(")

My question is, how would you reduce it? It seems really readable and maintainable now, but I want to get rid of the Codeclimate issue. Should I be splitting it up to two functions or does it defeat its purpose and makes the code less maintainable?

(This is a new version of the answer based on feedback from iScrE4m who asked the question. Feel free to consul the history for the original, wrong answer.)

Readability

First, it's confusing to write code like this:

if condition:
    do_stuff()
if other_condition:
    do_other_stuff()

Because it can be mistaken with the same code using elif where only one branch is executed. And indeed, that's how I read it first. The solution is simple: add a new line between the two conditions. That's a matter of personal preference though, and some developers prefer to write compactly as you did.

What's not captured by cyclomatic complexity?

Nevertheless, it's difficult to reason about this function, simply because you're using ifs and no elsifs. So we have to think about all options: for each if that's not an elif, what happens if both conditions are true? And I don't know if card.selfcast and player is None, card.targetable and target is None and not self.ask_for_nope() can be all true or not. I'm lucky here because you're raising an exception in the first two so I can infer that you consider them exclusive, but that's more reasoning that I would have liked to make.

However, cyclomatic complexity, as defined by Thomas J. McCabe in his 1976 paper, and used by modern tools, does not capture this information. He says in the beginning of section 2 ("A complexity measure"):

  The complexity measure approach we will take is to measure and control the number of paths through a program. This approach, however, immediately raises the following nasty problem: "Any program with a backward branch potentially has an infinite number of paths." Although it is possible to define a set of algebraic
  expressions that give the total number of possible paths through a (structured) program (1), using the total number of paths has been found to be impractical. Because of this the complexity measure developed here is defined in terms of basic paths - that when taken in combination will generate every possible path.

(1) The appendix explains how it can be done.

It's not entirely clear why it's considered impractical, but I guess it makes it harder to compare programs even if does not capture the issue here. (When McCabe wrote his paper in 1976, Fortran did not have ELSE IF: it only appeared in Fortran 77, but it already had a case satement, like the switch statement in C).

Leaving cyclomatic complexity aside, what are all the paths the original code can take? Two options per if, so that's 2*2*2, ie. 8. Now, using elsif, you only have 4 options, all of them with only one statement. Much better, right?

What's captured by cyclomatic complexity?

One thing that cyclomatic complexity does capture is that "complicated" conditions count more than simple ones. So when you write if card.selfcast and player is None, this actually counts as two ifs. This is why you reach 6: 5 ifs + 1. If you want to stay within the limit, you can simply encapsulate the conditions in their own functions, which would lower the complexity to 4.

Solving your immediate issue

I'm not sure encapsulating the conditions is worth it. In this case, consider just raising the limit: 6 is quite low, and I've seen 10 used elsewhere successfully (maybe because this is the number McCabe used in his original paper, even if he says it's arbitrary). This is just at tool, and if it does not help, then stop using it. :)


Iterating through one map and one multimap and printing the vlaue of common keys I've got one multimap: mm1 and a map: mm2. The size of mm1 is usually expected to be smaller than mm2. So I begin by iterating through mm1, and for each key in mm1 that also exists in mm2, will print the values of them. e.g. in the following example, first I come up with 2 in mm1, which also exists in mm2; therefor I print "aa" and "bb" from mm1 and "b" from mm2. If we encounter any key in mm1 that cannot be found in mm2, the whole process will be stopped. Example:

(in each pair, number is key, string is value)

mm1:

  2, "aa"
  2, "bb"
  4, "cc"
  4, "dd"
  5, "ee"

mm2:

  1, "a"
  2, "b"
  3, "c"
  4, "d"
  6, "e"

output should be:

  aa
  bb
  b
  cc
  dd
  d

For the following input, 4 is not found in the second map and therefore we stop printing values after 2 (even though 5 exists in both of them, it's not printed).

mm1:

  2, "aa"
  2, "bb"
  4, "cc"
  4, "dd"
  5, "ee"

mm2:

  1, "a"
  2, "b"
  3, "c"
  5, "d"

output should be:

  aa
  bb
  b

And here is my code:

#include <map>
#include <string>
#include <iostream>

using namespace std;

int main()
{
    multimap<int,string> mm1;
    mm1.insert(make_pair(2, "aa"));
    mm1.insert(make_pair(2, "bb"));
    mm1.insert(make_pair(4, "cc"));
    mm1.insert(make_pair(4, "dd"));
    mm1.insert(make_pair(5, "ee"));

    map<int,string> mm2;
    mm2.insert(make_pair(1, "a"));
    mm2.insert(make_pair(2, "b"));
    mm2.insert(make_pair(3, "c"));
    mm2.insert(make_pair(4, "d"));
    mm2.insert(make_pair(5, "e"));

    typedef multimap<int,string>::iterator Iter;

    pair<Iter, Iter> range1(mm1.begin(), mm1.begin());
    Iter next1 = mm1.begin();
    map<int,string>::iterator next2 = mm2.begin();

    range1 = mm1.equal_range(next1->first);
    while (!(range1.first == mm1.end() && range1.second == mm1.end()))
    {
        next2 = mm2.find(next1->first);
        if(next2 == mm2.end()) break;
        for (Iter itr = range1.first;itr!=range1.second;itr++)
        {
            cout << itr->second << endl;
        }
        cout << next2->second << endl;
        next1 = range1.second;
        next2++;
        if(next1 == mm1.end() || next2 == mm2.end())
            break;
        range1 = mm1.equal_range(next1->first);
    }
    return 0;
}

My intention was to iterate over each map only once. Any advice on improving the performance and size of this program would be appreciated!

You can merge both maps to a "list". That should give you O(n) complexity solution where n is a total number of elements in both maps.
map elements are sorted from lower to higher, so simple iteration would yield an ordered list.
To merge maps, you need to iterate both of the maps, instead of insertion to a list, you can just output values to the screen.


Show all records when current user is admin I have the following. How best to refactor it:

  def self.search(search, organization_id, is_admin)
    if is_admin == false
      where("record_no LIKE ?", "%#{search}%").
        where("organization_id = ?", organization_id)
    else
      where("record_no LIKE ?", "%#{search}%")
    end
  end

Basically this means, I want it to show all records without the organization_id when user is an admin.

About your code:

The argument name search is confusing (the same as the method), use instead the value it stands for: record_no.
"record_no LIKE ?". This is perfectly fine, but, if you used Arel instead, the query would work for all database.
if is_admin == false -> if !is_admin or unless is_admin. Shorter, more declarative, idiomatic.
DRY your code by reusing the repeated scope.
organization_id: Since you using anORM, you usually send objects as arguments, not their IDs. Does it make sense for you code?
where("organization_id = ?", organization_id). Use a hash instead.

I'd write:

def self.search(record_no, organization_id, is_admin)
  with_record_no = where(arel_table[:record_no].matches("%#{record_no}%"))
  if is_admin
    with_record_no
  else
    with_record_no.where(organization_id: organization_id)
  end
end


A more efficient way to make JQL (Jira Query Language) So my code here takes some parameters and then creates an SQLesque type of string (JQL).

function makeJQL(status, project, priority, description, summary){
    var arr = new Array();
    // Could put a null check here if all parametres are undefined
        if(status.length > 0){
            jql = "status="+'"'+status+'"';
            arr.push(jql)
        }
        if(project.length > 0){
            jql = "project="+'"'+project+'"';
            arr.push(jql)
        }
        if(priority.length > 0){
            jql = "priority="+'"'+priority+'"';
            arr.push(jql)
        }
        if(description.length > 0){
            jql = "description="+'"'+description+'"';
            arr.push(jql)
        }
        if(summary.length > 0){
            jql = "summary="+'"'+ summary +'"';
            arr.push(jql)
        }
        var jql = "";
        for(var i = 0; i < arr.length; i++){
            jql = jql + arr[i]+ ' AND '
        }
        jql = jql.substring(0, jql.length - 5);
        return jql
}

The only thing is, is that there is a lot of code repetition, but I don't know how to overcome this really. I also haven't put a null check in because if the function is called, there has to be parameters passed.

Instead of passing parameters to this function you can pass a single object and iterate through it.

// instead of this
makeJQL("status", "project", "priority", "description", "summary");

// Try something like this
makeJQL({
    status: "status",
    project: "project",
    priority: "priority"
});

And to itinerate the object passed to the function you can use Object.keys():

makeJQL: function(jqlObject){
    var arr = new Array();
    Object.keys(jqlObject).forEach(function(key) {
          array.push(key + "="+'"'+ jqlObject[key] +'"');
    });
}

Other good practice would be implement a function for each request in your application, creating a makeSomethingJQL will increase the readability of your code.


takeWhile, groupBy and group higher order functions I'm trying to implement few higher order functions from Haskell in C++. Below are the definitions of takeWhile, groupBy, and group.

I would appreciate it if someone can review and comment if the implementation takes care of all scenarios. Should I be using universal refs for parameters to these functions? Please let me know of any other comments, too. Please assume C++14 standard and above. Also, if there are existing implementations that I can use to study and refer to, please point me to them too.

#include <vector>
#include <utility>

template <typename fwd_iter, typename predicate>
auto takeWhile(fwd_iter first, fwd_iter last, predicate p) -> std::pair<fwd_iter, fwd_iter>
{
    auto till = (first);

    for (; till != last; till++) {
        if (!p(*till)) {
            break;
        }
    }

    return { first, till };
}

// something similar to std::greater or std::less.. is there something for equality??
struct equal {
    template <typename T>
    constexpr bool operator()(const T &lhs, const T &rhs) const
    {
        return lhs == rhs;
    }
};

// predicate type should be : typename *fwd_iter -> typename *fwd_iter  -> bool)
template <typename fwd_iter, typename predicate,
    typename container = std::vector<std::pair<fwd_iter, fwd_iter>>>
auto groupBy(fwd_iter first, fwd_iter last, predicate pred)
{
    auto current = first;
    container ret;

    while (current != last) {
        auto t = takeWhile(current, last, [&](auto x) {return pred(*current, x); });
        current = t.second;
        ret.emplace_back(t);
    }

    return ret;
}

template <typename fwd_iter>
auto group (fwd_iter first, fwd_iter last)
{
    return groupBy(first, last, equal());
}

struct equal is kind of useless. There's std::equal_to, which does the same thing.
You can make the design more flexible (and more standard library-like) by passing an output iterator to the groupBy and group function instead of creating a new container inside the function. It's useful if the user wants to write pairs that represent groups to an existing container or to a C-style array.


Populating and saving checkboxlist items to the database I have inherited an application that has code to save and populate values in an ASP.NET checkboxlist control. The existing code is very lengthy and does not appear to be efficient to me. I plan on rewriting this application and have complete control over the database design.

All of the checkboxlist items are stored in an SQL table as bit fields. Each row of checkboxlist values are assigned to a user ID. Once again, my goal is to find a more efficient way to save and populate selected checkboxlist items with less code. I'm open to changing the table design, using Linq or lambda if it helps.  I'm new to ASP.NET but there must be a better way of doing this.

Current table design
UserID  AA  BC  AC
1       1    1  0
2       0    1  1
3       1    1  0
4       1    1  1

'Code to populate selected checkboxlist items(there are a total of 22 bit values)

Dim CodeData as datarow
CodeData = CodeDataTable.Rows(0)
For Each li As ListItem In Me.cblCodes.Items
                    If li.Text = "AA" Then
                       If Not CodeData.IsOneNull Then
                          If CodeData.One = True Then
                               li.Selected = True
                        End If
                    End If
                End If
                  If li.Text = "BC" Then
                    If Not CodeData.IsTwoNull Then
                        If CodeData.Two = True Then
                            li.Selected = True
                        End If
                    End If
                End If
         If li.Text = "AC" Then
                    If Not CodeData.IsTwoNull Then
                        If CodeData.Two = True Then
                            li.Selected = True
                        End If
                    End If
                End If
            ......

Next

'Code to save checkboxlist values
 Dim AA As Boolean = False
 Dim BC As Boolean = False
 Dim AC As Boolean = False
 Dim AD As Boolean = False

 AA = Me.cblCodes.Items.FindByText("AA").Selected()
 BC = Me.cblCodes.Items.FindByText("BC").Selected
 AC = Me.cblCodes.Items.FindByText("AC").Selected
 AD = Me.cblCodes.Items.FindByText("AD").Selected

 CodeTableAdapter.UpdateCodes(UserID,AA,BC,AC,AD.......)

I think it's reasonable to look at whether a single column design is possible - it certainly makes life easier:

For Each DataRow dr in CodeDataTable.Rows
    Dim li as ListItem = Me.cblCodes.Items.FindByText(CType(dr["Text"], String))
    li.Selected = True
Next

You could alternatively do a two column design, with a Text varchar and a Value bit:

For Each DataRow dr in CodeDataTable.Rows
    Dim li as ListItem = Me.cblCodes.Items.FindByText(CType(dr["Text"], String))
    li.Selected = CType(dr["Value"], Boolean)
Next

But even the wide table could be done with less repetitive code by using DataTable.Columns to get the names of the columns:

Dim codeData as DataRow = CodeDataTable.Rows(0)
For Each DataColumn dc in CodeDataTable.Columns
    Dim li as ListItem = Me.cblCodes.Items.FindByText(dc.Name)
    If Not codeData.IsNull(dc) AndAlso CType(codeData[dc], Boolean) Then
        li.Selected = True
    End If
Next


Simple C++ object pool I am studying computer science and just for the sake of practise I thought of implementing an object pool.

This is my first implementation, it is simple and works correctly, but I would love any thoughts on it. Currently, the memory is allocated on the stack, but I am thinking of using policy based design to allow heap allocation as well.

Here is the code of the pool:

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

/**
 * Generic class for object pools.
 */
template <class T, int SZ, class Initialiser, class Releaser> class StackObjectPool {
public:
    using pointer = std::unique_ptr<T,std::function<void(T*)> >;
    using PoolType = StackObjectPool<T,SZ,Initialiser, Releaser>;

    StackObjectPool() {}
    StackObjectPool(const PoolType& orig) = delete;
    ~StackObjectPool() {}

    /**
     * Acquires an object not being currently used
     * @return pointer to the acquired object
     * @throw std::out_of_range if all the objects inside the pool are being used
     */
    pointer acquire() {
        unsigned int index = 0; // look for the first free object
        while(m_occupied_registry[index]) ++index;
        if(index >= SZ) throw std::out_of_range("Pool exceeded its size");
        m_occupied_registry[index] = true; // mark it as currently in use
        m_initialiser(&m_objects[index]); // initialise it

        //return an unique_ptr that calls release when reset
        return pointer(&m_objects[index], [this](T* element)->void{release(element);});
    }

    friend std::ostream& operator<<(std::ostream& ostream, const PoolType& pool) {
        for(unsigned int index = 0; index < SZ; ++index) {
            std::cout<<(pool.m_occupied_registry[index]?"[occupied] ":"[free]\n");
            if(pool.m_occupied_registry[index]) {
                std::cout<<pool.m_objects[index]<<"\n";
            }
        }
        return ostream;
    }
private:

    void release(T* element) {
        unsigned int index = 0;
        while(&m_objects[index] != element) ++index;
        m_occupied_registry[index] = false; // mark the released element as free
        m_releaser(element); // call release functor
    }

    Initialiser m_initialiser;
    Releaser m_releaser;
    bool m_occupied_registry[SZ] {0};
    T m_objects[SZ];
};

And here is a Test class and a simple main which I have used to test the pool:

class Test {
public:
    Test():m_a(0),m_b('0'){
        std::cout<<"Construct default Test"<<*this<<"\n";
    }
    Test(int a, char b): m_a(a), m_b(b) {
        std::cout<<"Construct Test"<<*this<<"\n";
    }
    Test(const Test& orig): m_a(orig.m_a),m_b(orig.m_b) {
        std::cout<<"Construct cpy Test"<<*this<<"\n";
    }
    ~Test() {
        std::cout<<"Delete Test"<<*this<<"\n";
    }
    friend std::ostream& operator<<(std::ostream& ostream, const Test& test) {
        ostream<<"["<<test.m_a<<",'"<<test.m_b<<"']";
        return ostream;
    }
private:
    int m_a;
    char m_b;
};

struct Initialiser {
    void operator() (Test* element) {
        std::cout<<"Initialising Test"<<*element<<"\n";
    }
};

struct Releaser {
    void operator() (Test* element) {
        std::cout<<"Releasing Test"<<*element<<"\n";
    }
};

int main(int argc, char** argv) {
    using TestPool = StackObjectPool<Test,5, Initialiser, Releaser>;
    TestPool pool;
    TestPool::pointer pt1, pt2,pt3,pt4,pt5;

    std::cout<<pool;

    pt1= pool.acquire();
    std::cout<<"Aquired pt:"<<*pt1<<"\n";
    //pt1.reset();

    std::cout<<pool;

    pt2 = pool.acquire();
    std::cout<<"Aquired pt:"<<*pt2<<"\n";
    pt2.reset();

    std::cout<<pool;

    pt3 = pool.acquire();
    std::cout<<"Aquired pt:"<<*pt3<<"\n";
    //pt3.reset();

    std::cout<<pool;
    return 0;
}

You might change:

void release(T* element, unsigned int index) {
    if (&m_objects[index] != element) {
            std::cerr << "no match index and element\n";
            return; // or exit(1);
    }
    m_occupied_registry[index] = false; // mark the released element as free
    m_releaser(element); // call release functor
}

and at the acquire  change the return line:

    return pointer(&m_objects[index], [this,index](T* element)->void{release(element,index);});

The above will improve the efficiency of the release operation, as you don't need to search for the element.


Bootstrap pagination Here is my bootstrap pagination code. Maybe it can be simplified.

const pagination = {
  pre: 0,
  curr: 1,
  max: 6,
  list: null,
  cn: '',
  swap() {
    pagination.list.children[pagination.pre].classList.remove(pagination.cn);
    pagination.list.children[pagination.curr].classList.add(pagination.cn);
  },
  changeBy(n) {
    pagination.pre = pagination.curr;
    pagination.curr += n;
    if (pagination.curr < 1) {
      pagination.pre = 0;
      pagination.curr = 1;
    } else if (pagination.curr > pagination.max) {
      pagination.pre = pagination.max - 1;
      pagination.curr = pagination.max;
    }
    pagination.swap();
  },
  prev() {
    pagination.changeBy(-1);
  },
  next() {
    pagination.changeBy(1);
  },
  activate(i) {
    return () => {
      pagination.pre = pagination.curr;
      pagination.curr = i;
      pagination.swap();
    }
  },
  setHandler() {
    pagination.list.children[0].addEventListener("click", pagination.prev);
    pagination.list.children[pagination.max + 1].addEventListener("click", pagination.next);
    for (let i = 1; i < (pagination.max + 1); i += 1) {
      pagination.list.children[i].onclick = pagination.activate(i);
    }
  },
  init(list, cn) {
    pagination.list = list;
    pagination.cn = cn;
    pagination.setHandler();
  }
}

let list = document.querySelector(".pagination"),
  cn = "active",
  init = pagination.init(list, cn);
<link href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" rel="stylesheet">

        &laquo;

1

2

3

4

5

6

        &raquo;

Could you give more information on what the point of this is? All it does is change the class of the links. What about the actual pagination itself, which i assume would use AJAX? This code should probably know about that in order to react to errors.

Wrapping the code in an object like this isn't very useful like this. For one, the functions inside the containing object all refer to its variable name, so it breaks if it's changed. Also it looks like a reusable class allowing to have multiple paginations in one HTML document, however it's not.

You have the "max size" redundantly hard coded twice - once in the JavaScript and once as the number of elements in the HTML. How do you know that the pagination has exactly 6 pages?

The same basic problem with the original "current" element and the "active" class: It's hard coded in both the JS and the HTML. With the class name it even looks as if you could change it, because it's an initialization parameter, but changing it breaks the design, because its hard coded in the HTML.

Generally you have some unnecessarily abbreviated variable names, cn being the worst.


Socket wrapper class I've created this thin wrapper class for a socket project that I'm working on.  Can someone review my class and give me some points on if I'm on right track?

#include <iostream>
#include <WinSock2.h>

const int MAX_RECV_LEN = 8096;
const int MAX_MSG_LEN= 1024;
const int PORT_NUM =  1200;

class Sokect
{

private:
    Sokect(){}
    void setSocketID(int socketFb){socketID = socketFb;}

    int portNum;
    int socketID;
    int blockFlag;
    int bindFlag;

public:
    Sokect(int);

    ~Sokect(){
        closesocket(socketID);
    }

    void setReuseAddr(int);
    void getReuseAddr(int);
    void setKeepAlive(int);
    void getKeepAlive(int);
    void setLingerOnOff(bool);
    void getLingerOnOff(bool);
    void setLingerSecond(int);
    void getLingerSecond(int);
    void setSocketBlockFlag(int);
    void getSocketBlockFlag(int);

    //size of send & Receive Buffer
    void  setSendBufferSize(int);
    void  getSendBufferSize(int);
    void  setReceiveBuffferSize(int);
    void  getReceiveBuffferSize(int);

    int getSocketID() {return socketID;}
    int getPortNum() {return portNum;}

    friend ostream& operator<<(ostream&,mySocket&);

    //get systems error
    void detectErrorOpenWinSocket(int*,string&);
    void detectErrorSetSocketOption(int*,string&);
    void detectErrorGetSocketOption(int*,string&);

};

There are at least a few things I think I'd do differently, at any rate.

const int MAX_RECV_LEN = 8096;
const int MAX_MSG_LEN= 1024;
const int PORT_NUM =  1200;

The ALL_CAPS convention was originally invented for macros. At least as far as I can tell, it was originally for function-like macros, to give a subtle warning that it might evaluate its argument more than once, so you needed to be careful about exactly how and where you applied it (especially to arguments that might have side effects).

It really doesn't make sense for a situation like this, so I'd prefer to avoid it. Regardless of the exact names you choose, it's probably better to avoid putting these variables in the global namespace any way. I'd consider wrapping the entire class in a namespace instead.

private:
    Sokect(){}

Assuming your compiler is recent enough to support it, I'd prefer to use =delete; to ensure that a ctor can't be accessed, rather than defining it as private.

[ ... ]

void setReuseAddr(int);
void getReuseAddr(int);
void setKeepAlive(int);
void getKeepAlive(int);
void setLingerOnOff(bool);
void getLingerOnOff(bool);
void setLingerSecond(int);
void getLingerSecond(int);
void setSocketBlockFlag(int);
void getSocketBlockFlag(int);

I think rather than embedding these directly into the socket itself, I'd create a separate socket_options (or something similar) struct to hold them. This would allow (for example) storing an entire set of options together, so you can set all the options for a particular socket at once.

//size of send & Receive Buffer
void  setSendBufferSize(int);
void  getSendBufferSize(int);
void  setReceiveBuffferSize(int);
void  getReceiveBuffferSize(int);

Likewise, I'd probably create a small class just to handle the buffering. Then the socket class could just contain two buffer instances, one for receiving and one for sending.

//get systems error
void detectErrorOpenWinSocket(int*,string&);
void detectErrorSetSocketOption(int*,string&);
void detectErrorGetSocketOption(int*,string&);

These seem to me like they merit at least a little more explanation of what they're actually supposed to be/do.


Running a block of code on every interval except the first iteration I am working with an encryption algorithm that looks like this

The key is a number stored in 4 bytes.
For each byte in the message, XOR it with the corresponding byte in the key
When you reach the end of the key, multiply the key by 13, and then continue looping

Translated, it looks like this

for (int i = 0; i < dataSize; i++) {

   // Increase the key as per the algorithm
   if (i > 0 && (i % 4 == 0)) {
      key *= 13
   }

   // Shift over i bytes, as per the algorithm
   data[i] ^= key >> (8 * (i % 4));
}

What I don't like is that check for i > 0, as it looks like wasteful processing. How can I modify this code so that I don't have that extra check that is basically redundant after the first iteration of the loop?

If the size of the data is 5 MB I don't want to run an extra 5 million or so i > 0 checks when i is always increasing.

Initialize the first byte, and then start the loop from 1.....

You may also want to do a bit-check instead of modulo, but they may be equally fast...

if (dataSize > 0) {
    data[0] ^= key;
}
for (int i = 1; i < dataSize; i++) {

   // Increase the key as per the algorithm
   if (i & 3 == 0) {
      key *= 13
   }

   // Shift over i bytes, as per the algorithm
   data[i] ^= key >> (8 * (i % 4));
}

EDIT:

I have seen the following done (translating from memory of a C program)... this has been known to be able to compile down to SIMD-using instructions on supporting compilers, etc.

for (int i = 0; i < dataSize - 3; i+=4) {

   // Shift over i bytes, as per the algorithm
   data[i + 0] ^= key; // >> (   0);
   data[i + 1] ^= key >> (   8);
   data[i + 2] ^= key >> (  16);
   data[i + 3] ^= key >> (  24);

   key *= 13

}

// handle up to 3 remaining bytes.....
for (int i = dataSize - (dataSize & 3); i < dataSize; i++) {
    data[i] ^= key >> (8 * (i & 3));
}


Sudoku solver in Ruby All the test cases pass except one of the test case which runs > 10 seconds. What can I do to make my solution faster?

def in_row(grid,i,k)
    grid[i].include? k
end

def in_col(grid,j,k)
    0.upto(8) do |i|
      return true if grid[i][j] == k
    end
    false
end

def in_grid(grid,i,j,k)
    x,y =0,0
    x = 3 if i > 2
    x = 6 if i > 5
    y = 3 if j > 2
    y = 6 if j > 5
    x.upto(x+2) do |xx|
      y.upto(y+2) do |yy|
        return true if grid[xx][yy] == k
      end
    end
    return false
end

def find_unassigned_location(grid)
  grid.each_with_index do |rows,i|
    rows.each_with_index do |cols,j|
      return [i,j] if cols == 0
    end
  end
  false
end
def sudoku_solve grid
    # If there is no unassigned location, we are done
    cell = find_unassigned_location(grid)
    return [true,grid] unless cell # No unassigned cell remaining
    i,j = cell[0],cell[1]
    1.upto(9) do |k|
      next if in_row(grid,i,k) || in_col(grid,j,k) || in_grid(grid,i,j,k)
      grid[i][j] = k
      return [true,grid] if sudoku_solve(grid)[0] == true  # Recursively solve
      grid[i][j] = 0 # Unassign location if recursive solution returns false
    end
    return [false,grid]
end
def print_grid (grid)
    grid.each do |rows|
      rows.each do |cols|
        print "#{cols} "
      end
      puts ''
    end
end

board = []
(0...9).each do |i|
  board << gets.split.map {|num| num.to_i}
end
solution = sudoku_solve(board)
print_grid(solution[1]) if solution[0]

# -- input
4 0 0 0 0 6 0 0 0
0 6 0 0 0 0 0 0 9
0 0 0 0 0 0 0 0 0
0 0 2 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0
0 0 3 0 6 0 0 2 0
1 0 0 0 0 0 9 0 0
8 0 0 0 0 5 0 0 0
0 0 0 0 0 0 0 0 5

Remember how you solve Sudoku boards manually - you write a list of allowed numbers into each cell, and whenever you add a new number somewhere to the grid, you update the other cell's lists accordingly.

So one obvious optimization is to implement the same kind of bookkeeping in your program, that will speed up the process heavily: instead of having a test like this

1.upto(9) do |k|
      next if in_row(grid,i,k) || in_col(grid,j,k) || in_grid(grid,i,j,k)

your program can just grab the allowed numbers from the cells's list, without the need of calling the functions in_row, in_col, and in_grid. Of course, the assignment of the number will need some additional time now for the bookkeeping, but if you do this right, I am sure the bookkeeping will pay off.

Moreover, such bookkeeping will allow some further optimizations: for example, when the list of allowed numbers of one cell becomes empty, you can stop the recursion immediately. The same is true when the union of all numbers of the "allowed numbers" lists of a row, a column or a 3x3 subgrid does not contain all values from 1 to 9 any more.

Note that for this approach, it will be beneficial to replace your grid array by a data structure or class which holds not only the cell values, but also the "allowed numbers" list of each cell, and supports adding and removing of numbers.

For removing a number, you will need to reset the grid including the internal "allowed number" lists to the state before the number was added. A simple implementation is to make a copy of the whole grid beforehand and replace the current grid by the copy afterwards, when the "move" was not accepted.


Hangman Game in Python-3 answer = input("what's the word")
answer_list = list(answer) #list version of the original word
presentation = []
for i in range(len(answer_list)):
    presentation.append("_") #makes a list that shows the progress of the player during the game

incorrect = 0 #number of allowed guesses
completion = False # condition for end game
while completion == False:
    attempt = input('guess')
    ind = 0 #index of the guess that appears in answer_list
    count = 0 #number of occurences of the guess in answer_list
    for x in answer_list: #searches for all occurences of the guess in answer_list and change presentation accordingly
        if x == attempt:
            num = answer_list.index(attempt)
            presentation[num] = attempt
            answer_list[num] = 0 #if there is an occurence, replace that occurence with 0 in answer_list
            count += 1
    if count>0:
        print ("Your guess is correct, there was/were {} matches in the word".format(count))
        print(presentation)
    elif count == 0:
        incorrect += 1

    if incorrect == 5:
        print("You lost")
        break

    if any(answer_list) == False: #since all 0 have a negative truthy value, we can use any() to check if any element has a truthy value
        print("Congrats, you got everything correct")
        completion = True
        break

I want to make clean up this hangman game and format it as 1 or 2 functions. How do I make it work? For example, initializing lists could be initialize() and from the declaration of incorrect to the end of the code can be play_hangman(). I am still very new to python.

Welcome to Code Review!
PEP-8
Since you're still new to python, it's a good idea to keep a window/tab open with PEP-8 loaded into it. It is mostly suggestions for coding style. Few things that can be picked up from the same:

Comparisons to singletons like None should always be done with is or is not, never the equality operators.
Comments should be complete sentences. The first word should be capitalized, unless it is an identifier that begins with a lower case letter (never alter the case of identifiers!).
PEP 257 describes good docstring conventions. Note that most importantly, the """ that ends a multiline docstring should be on a line by itself:

Functions
As you've already raised this point, splitting the code into individual functions is always a good practice.
if __name__ block
Put the execution logic of your script inside the if __name__ == "__main__" block. Read more about the details on why on stack overflow.
f-string
Python 3.x also introduced literal string interpolation, or more commonly used term: f-string. Read more about it here (PEP-498).
Type hinting
When writing functions, you can also make use of type hinting to provide a more readable flow of your code to anyone. This helps removing the manual labour of having to backtrack variable types etc. More details can be found in PEP-484.
Game logic
The initialisation of presentation array can be simplified to:
presentation = ["_"] * len(answer)

For every guessed character from the user, you keep looping over the answer_list, irrespective of validating whether the guess is correct or not.
Before the loop over each character of the correct word, you have setup ind = 0 which is never really used.
Your global while loop relies on the condition completion == False (which should ideally be completion is False) but you break out before the loop/condition really has a chance to do so for you, making the variable useless.

Rewrite
from typing import List

MAX_ATTEMPTS: int = 5

def get_answer() -> str:
    return input("What's the word? ")

def correct_guess(char_count: int, guessed_word: List[str]):
    print(f"Your guess is correct, there was/were {char_count} matches in the word.")
    print(" ".join(guessed_word))

def game_win(guessed_word: List[str]) -> bool:
    return "_" in guessed_word

def game():
    answer = get_answer()
    correct_letters = set(answer)
    guessed_word = ["_"] * len(answer)
    incorrect = 0
    while True:
        if incorrect >= MAX_ATTEMPTS:
            print("You lost!")
            break
        attempt = input("Guess: ")
        if attempt not in correct_letters:
            incorrect += 1
            continue
        char_count = 0
        for index, char in enumerate(answer):
            if char == attempt:
                guessed_word[index] = char
                char_count += 1
        correct_guess(char_count, guessed_word)
        if game_win(guessed_word):
            print("Congrats, you got everything correct")
            break

if __name__ == "__main__":
    game()