Inserting and deleting elements in a list In my code, I use fast method of reading input and a custom model of list. After reading the input into my 'list'.

The problem is that, when I test my code, most answers are correct, but for some INPUT my algorithm works slow. I unfortunately don't know the input data, but 6 from 12 tests finished with the wrong time. Maybe for some input data, some of the loops do not stop.

Where time is 1.01 or 2.01 -> wrong

  TASK

  Consider any indexed sequence of natural numbers C, which define the
  concept of the present position. Next, we introduce two operations on
  the elements of this sequence: If amount of sequence of natural
  numbers is even number ->

  R, remove the item 'c' on index POS + 1, then move the pointer POS on
  'c' elements to the right, otherways ->

  X, inserting element 'c' on the position (POS + 1) value 'c-1' from
  POS, and then moving the pointer on the POS 'c' elements to the right.

  INPUT:

  the number of times of repetition scheme of operations R and X, the
  sequence of natural numbers 'C'

  OUTPUT:

  numbers after operations from the position of index

  LIMITS

  The time complexity of O (tn), where 'n' is the length of 'C' and 't'
  - times of repetition operations R and X.

#include<iostream>
#include<stdlib.h>
#define gc getchar_unlocked

struct Element
{
    unsigned int c;
    Element* next;
    Element(unsigned int value)
    {
        c = value;
        next = NULL;
    }
};

void scan_integer(unsigned int* o )
{
    unsigned int c = gc();
    int x = 0;
    for( ; ((c<48 || c>57)); c = gc() );

    for( ;c>47 && c<58; c = gc() ) {
        x = (x << 1) + (x << 3) + c - 48;
    }
    *o = x;
}

int main()
{
    unsigned int rounds;
    scan_integer(&rounds);
    if(!feof(stdin))
    {
        unsigned int elements = 0;
        unsigned int tmp;
        scan_integer(&tmp);
        Element* first = new Element(tmp);
        elements++;
        Element* current = first;
        while(!feof(stdin))
        {
            scan_integer(&tmp);
            current->next = new Element(tmp);
            current = current->next;
            elements++;
        }
        current->next = first;
        current = first;
        for(int i = 0; i < rounds; i++)
        {
            if(elements == 0){
                fprintf(stdout, "%u", -1);
                break;
            }
            unsigned int value;
            if(current->c & 1)
            {
                value = current->c;
                Element* newC = new Element(value-1);
                newC->next = current->next;
                current->next = newC;
                elements++;
            }
            else
            {
                value = current->next->c;
                Element* toDelete = current->next;
                current->next = toDelete->next;
                delete toDelete;
                elements--;
            }

            if(elements > 0 && value != 0){
                for(int j = 0; j < value; j++){
                    current = current->next;
                }
            }
        }
        if (elements == 0) {
            fprintf(stdout, "%u", -1);
        }
        if(elements > 0)
        {
            first = current;
            fprintf(stdout, "%u", current->c);
            current = current->next;
            while(first != current)
            {
                fprintf(stdout, " %u", current->c);
                current = current->next;
            }
        }
    }else{
        return -1;
    }

    return 0;
}

Let's start with the code style, even though not asked explicitly.

It says C++, but apart from using a constructor on the struct  and using new/delete, that is pure C what you have written there.

Especially the C style input and output handling is overcomplicated, with regards to how much simpler the C++ interfaces are to use.

As for your implementation, you don't need a linked list. In fact, you don't need to store the input nor the output sequence at all, at least not for the algorithm itself.

Take a look at the problem carefully again, and try to think about how you can rephrase the operations, when do you need to consume input, and what the earliest point is, at which you can create output. Last but not least, how long do you even need to store each specific piece of information.

The first observation:
Whenever an operation has finished, the value at POS is no longer mutated, and no further insertions or deletions occur prior to POS.

The second observation:
During an operation at POS, no value other than POS has an effect on that operation. At most POS+1 is either removed or added in.

The third observation:
As all numbers are guaranteed to be natural numbers, even c - 1 > 0 must hold. Which means that in any case, POS+1 is always output in the same operation as which it is mutated in.

With that in mind, it's actually quite simple to solve this problem:

#include <iostream>
#include <vector>

int main()
{
    std::vector<int> input;
    int rounds = 0;
    bool even = false;

    // Collect the input
    std::cin >> rounds;
    int temp;
    while(std::cin >> temp) {
        input.push_back(temp);
    }

    even = (input.size() % 2) == 0;

    std::vector<int>::iterator it = input.begin();

    // Special handling for first element
    std::cout << *it;
    it++;

    for(int round = 0; round < rounds; round++) {
        // Number of steps to the right
        int steps = *it;
        if(even) {
            // Operation "R"
            // Skip one
            it++;
            // And pass through from input
            for(int i = 0; i < steps; i++, it++) {
                std::cout << " " << *it;
            }
        } else {
            // Operation "X"
            // Directly output `POS+1`
            std::cout <<  " " << *it - 1;
            // But pass through one less input in return
            for(int i = 0; i < steps - 1; i++, it++) {
                std::cout <<  " " << *it;
            }
        }
        even != even;
    }
    for(; it != input.end(); it++) {
        std::cout <<  " " << *it;
    }
}

It is unfortunately still necessary to store the entire input in memory, in order to count the elements. If it wasn't for that, you could just copy from input to output element wise.

But what's important: The data structure used is now a plain vector, which removes a lot of unnecessary stress on the memory system, as the storage method is a compact as it gets.

There are also no mutations to that data structure at all, it only serves to replay the input after counting all elements.

The program spends most of the time in list maintenance - allocating, deleting, and traversing the nodes. The solution is to not have the list in the first place.

You are only required to skip values to the right, therefore there is no point to keep the values at the left of the current index. Print them as necessary, and forget:

    while count < operations
        read a value n
        if n is even,
           read and print next n values
        else
           print (n - 1)
           read and print next (n - 1) values
        increment count

    read and print the rest


Program to Check If string is Palindrome (in C) Just wrote a program to check if a string is Palindrome. If that's true, return 1. Else, return 0. How can I improve it?

#include <stdio.h>
#include <string.h>
#include <ctype.h>

int palindrome(char *s)
{
    int i = 0, length = strlen(s) - 1, n = length;
    int boolean = 0;

    for (; i <= length && n >= 0; i++, n--) {
        if (toupper(s[i]) == toupper(s[n]))
            boolean = 1;
        else
            boolean = 0;
    }

    return boolean;
}

int main(void)
{
    char string[] = "Racecar";

    printf("%d", palindrome(string));

    return 0;
}

Bug
Given the input "racebar", the function returns true.  The problem is that your boolean variable is reset after each character pair, so you are only really testing the first character with the last.  I would do the following:

Get rid of the boolean variable and return false on the first mismatch.
Fix the loop iteration to only iterate through half the string instead of the whole string.
Mark the input parameter as const, since you do not modify the input string.  This allows your function to accept both const strings and non-const string as arguments.
In C99, you can declare loop variables inside the loop itself, which helps to reduce the scope of these variables to only where they are used.

Here is a rewrite showing all of the above:
int palindrome(const char *s)
{
    for (int i = 0, n = strlen(s) - 1; i < n; i++, n--) {
        if (toupper(s[i]) != toupper(s[n]))
            return 0;
    }

    return 1;
}

When you mentally check a word to see if it's a palindrome do you first compare the letters at each end and work inwards letter by letter or do you just read the word backwards and compare to the original word? I do the latter so modelling that you could just reverse the word and compare to the original.


Printing multiplication table to 12 × 12 Now, a while ago, I said in chat:

  I'm done with BF programming.

Except I guess I was lying. Here's a Brainfuck program I wrote, that prints out the multiplication tables up to 12. Fairly simple; what it does:

Loops through 1-12; let's say the current value we are at is x

Loop 12 times, adding x each time, and prints the result
Resets the number

The code:

Cell 0 is space
Cell 1 is newline
Cell 2 is outer counter
Cell 3 is outer number
Cell 4 is inner counter
Cell 5 is inner number
Cell 6 is copy workspace

>++++[<+++++ +++>-]                                 set cell 0 to space
+++++ +++++                                         set cell 1 to new line

>+++++ +++++ ++                                     set counter to 12
[                                                   while counter is not 0
    >+                                              increment number
    >+++++ +++++ ++                                 set inner counter to 12
    [                                               while inner counter is not 0
        <[>>+>+<<<-]>>>[<<<+>>>-]                   increment inner number

        >++++++++++<<[->+>-[>+>>]>[+[-<+>]>+>>]
        <<<<<<]>>[-]>>>++++++++++<[->-[>+>>]>
        [+[-<+>]>+>>]<<<<<]>[-]>>[>++++++
        [-<++++++++>]<.<<+>+>[-]]<[<[->-<]++++++
        [->++++++++<]>.[-]]<<++++++[-<++++++++>]
        <.[-]<<[-<+>]                               print number gotten off SO

        <<<<<<.                                     print space
        >>>>-                                       go to inner counter
    ]                                               end inner loop
    >[-]<                                           reset number
    <<<.                                            print new line
    >-                                              go to counter
]                                                   end outer loop

Compressed:

>++++[<++++++++>-]++++++++++>++++++++++++[>+>++++++++++++[<[>>+>+<<<-]>>>[<<<+>>>-]>+++++++
+++<<[->+>-[>+>>]>[+[-<+>]>+>>]<<<<<<]>>[-]>>>++++++++++<[->-[>+>>]>[+[-<+>]>+>>]<<<<<]>[-]
>>[>++++++[-<++++++++>]<.<<+>+>[-]]<[<[->-<]++++++[->++++++++<]>.[-]]<<++++++[-<++++++++>]<
.[-]<<[-<+>]<<<<<<.>>>>-]>[-]<<<<.>-]

Output:

1 2 3 4 5 6 7 8 9 10 11 12
2 4 6 8 10 12 14 16 18 20 22 24
3 6 9 12 15 18 21 24 27 30 33 36
4 8 12 16 20 24 28 32 36 40 44 48
5 10 15 20 25 30 35 40 45 50 55 60
6 12 18 24 30 36 42 48 54 60 66 72
7 14 21 28 35 42 49 56 63 70 77 84
8 16 24 32 40 48 56 64 72 80 88 96
9 18 27 36 45 54 63 72 81 90 99 108
10 20 30 40 50 60 70 80 90 100 110 120
11 22 33 44 55 66 77 88 99 110 121 132
12 24 36 48 60 72 84 96 108 120 132 144

Some notes:

The multiplication table is missing formatting. I realise that, and will add it some time later, if I can figure out how to compare two values.
I realise it doesn't work for multiplication tables of 16 or higher, due to BF cell sizes being only 8 bits, or up to 255. The value 16 * 16 would result in 256, setting the value of the cell to be 0. Is this unavoidable? Or is there a trick to it?

Concerns:

Does it make sense? (Considering it's Brainfuck, it shouldn't make sense...)
Do my "comments" make sense?
Are there redundant commands that I can remove/reduce?

The multiplication table is missing formatting. I realise that, and will add it some time later, if I can figure out how to compare two values.

Comparing two values isn't that complicated, once you know how to do it. It is however extremely inefficient if you do it over and over again. Your best bet would be to hook into the printing of the number and print spaces if there are no hundreds or tens.

I managed to do a little proof-of-concept for this, which I am sure can be optimized further. The idea is the following:

Before the loop that prints the number of 100's, set a temporary flag on another cell
If the loop is entered, reset the temporary flag at the end
Outside the loop, go to temporary flag and if it is set, print a space. Clear the flag and go back to the cell.

The unformatted and unoptimized version of this enhanced "print the number" code is as follows:

        >++++++++++<<[->+>-[>+>>]>[+[-<+>]>+>>]
        <<<<<<]>>[-]>>>++++++++++<[->-[>+>>]>
        [+[-<+>]>+>>]<<<<<]>[-]>>

>>>+<<<[

    [x>++++++[-<++++++++>]<.<<+>+>[-]]

>>>-<<<]>>>[++++++++++ ++++++++++ ++++++++++ + . [-]]<<<

<

>>>+<<<[
    [y<[->-<]++++++[->++++++++<]>.[-]]
>>>-<<<]>>>[++++++++++ ++++++++++ ++++++++++ + . [-]]<<<

<<++++++[-<++++++++>]
        <.[-]<<[-<+>]                               print number gotten off SO

I have marked the loop where the 100's are printed with x and the loop for the 10's with y, to make it easier for you to see the changes I did.

  I realise it doesn't work for multiplication tables of 16 or higher, due to BF cell sizes being only 8 bits, or up to 255. The value 16 * 16 would result in 256, setting the value of the cell to be 0. Is this unavoidable? Or is there a trick to it?

There are some different possible solutions to this:

Use a Brainfuck interpreter that supports higher values (Disclaimer: That's my project)
Print value of cell as number for any sized cell
Use bit-width conversions
Perform the multiplication differently. This would involve a completely different approach of storing each individual digit in its own cell and doing the multiplication on a more detailed level, by looping over the digits and multiplying. I won't go into much more detail about this as it would require a lot of work.

I am unfortunately only experienced with using a Brainfuck interpreter that supports higher values. I don't know at all how the Bit-width conversions work, or how you are gonna do the number printing then.

  Does it make sense? (Considering it's Brainfuck, it shouldn't make sense...)

  Do my "comments" make sense?

Surprisingly much sense, yes. For a Brainfuck developer your code is quite easy to follow and your comments are helpful.

  Are there redundant commands that I can remove/reduce?

There are two places in your code where I am, without doing over-analysis, able to find optimization possibilities: How you structure your memory, and how you print the numbers.

When structuring the memory, think about which places you need to go, and in which order. Currently you have positioned the space (value 32) at index 0 on the tape, and yet it is one of the most accessed values. Positioning that closer to the "inner number", will allow you to go to it more easily.

The printing of the number is a common algorithm found at Stack Overflow, which is fine. But I have realized that it is a bit inefficient for printing multiple numbers, as it goes from value 0 to value 48 (ASCII value for '0') multiple times, and resets to 0 every time. It would be better to initialize a '0' into a cell and re-use that cell multiple times, without resetting it.

I think I came up with a solution to printing values greater than 16*16. Is it possible to print values without spaces between them? If so I'm sure there's some way to make 16*16 come out to 25 and then without a space in-between 5 (or 2 and then 56) and do that with all values greater than 255.

As for your concerns:
1) It works so it must make sense
2) I don't see anything wrong with your comments
3) I don't see anything glaringly inefficient. Isn't that sort of the point of the language? To be really minimalist and efficient.


Check if two strings are anagrams I'm doing some practice questions from the book Cracking the coding interview and wanted to get some people to review my code for bugs and optimizations.

Question:

  Write a method to decide if two strings are anagrams or not.

/*
Time complexity: O(n^2)
Space complexity: O(n)
*/
bool IsAnagram(std::string str1, std::string str2)
{
    if(str1.length() != str2.length())
        return false;
    for(int i = 0; i < str1.length();i++)
    {
        bool found = false;
        int j = 0;
        while(!found && j < str2.length())
        {
            if(str1[i] == str2[j])
            {
                found = true;
                str2[j] = NULL;
            }
            j++;
        }
        if(!found)
            return false;
    }
    return true;
}

Since you only modify a copy of the second string, the first one should be a const reference:

bool IsAnagram(const std::string &str1, std::string str2)

Also, i and j should be of type size_t to match what they're compared with.

However, I think I'd do it like this:

bool IsAnagram2(std::string str1, std::string str2)
{
    std::sort(str1.begin(), str1.end());
    std::sort(str2.begin(), str2.end());
    return str1==str2;
}

Test program:

#include <iostream>
#include <string>
#include <algorithm>

#define SHOW(x) std::cout << # x " = " << x << '\n'

int main()
{
    std::cout << std::boolalpha;
    SHOW(IsAnagram("\0\0\0\0\0", "\0lehl"));
    SHOW(IsAnagram("hello", ""));
    SHOW(IsAnagram("\0\0\0\0\0", "olehl"));
    SHOW(IsAnagram("hello", "ole"));
    SHOW(IsAnagram("hello", "plehl"));
    SHOW(IsAnagram("hello", "hello"));
    SHOW(IsAnagram("hello", "12345"));
    SHOW(IsAnagram("hello", "Hello"));
    SHOW(IsAnagram("hello", "oellh"));
    SHOW(IsAnagram("hello", "olelh"));
    SHOW(IsAnagram("hello", "elelh"));
}

Program output

IsAnagram("\0\0\0\0\0", "\0lehl") = true
IsAnagram("hello", "") = false
IsAnagram("\0\0\0\0\0", "olehl") = false
IsAnagram("hello", "ole") = false
IsAnagram("hello", "plehl") = false
IsAnagram("hello", "hello") = true
IsAnagram("hello", "12345") = false
IsAnagram("hello", "Hello") = false
IsAnagram("hello", "oellh") = true
IsAnagram("hello", "olelh") = true
IsAnagram("hello", "elelh") = false

Time complexity here is n^2, whereas if we sort both the keys first, worst case would be n^2 and n^2 which is not really optimized, although it really is easy to write.

    bool isAnagram(string key1,string key2){
    int len=0;
    if(key.length()!=key1.length()){return false;}
    for (int i=0;i<key1.length();i++){
            for (int j=0;j<key1.length();j++){
                if(key1[i]==key2[j]){
                    len++;
                    key2[j] = ' ';//to deal with the duplicates
                    break;
                }

            }
        }
if (len==key.length()){return true;}
else{return false;}
}


Get most recent modified time of a folder recursively with PHP I'm relatively new to php and any advice is appreciated.

I have written a code to the most recently updated time for a folder in php.
Can you please correct me if I am doing anything wrong.

And here the code I use:

function get_m_time_dir ($directory)
{
$last_modified_time = 0;
$handler = opendir ($directory);
while ($file = readdir ($handler))
{
    if ($file == '.' || $file == '..')
    {
        continue;
    }
    if (is_file ($directory . DIRECTORY_SEPARATOR . $file))
    {
        $filemtime = filemtime ($directory . DIRECTORY_SEPARATOR . $file);
        if ($filemtime > $last_modified_time)
        {
            $last_modified_time = $filemtime;
        }
    } else
    {
        $filemtime = get_m_time_dir ($directory . DIRECTORY_SEPARATOR . $file);
        if ($filemtime > $last_modified_time)
        {
            $last_modified_time = $filemtime;
        }
    }
}
return $last_modified_time;
}

Can you please correct me if I am doing anything wrong.

It depends on whether you care for the deleted files or not. If you do, you should also check the modification time for the directory itself, not only files in it.

The code itself could be also improved:

you can remove the duplicated condition
which in turn, can be substituted with max() function.
also you can utilize the glob() function that can save you that boring $file == '..' condition, as well as other unnecessary code.

So in the end the code could be

function get_m_time_dir ($directory)
{
    $last_modified_time = 0;
    $dirmtime = filemtime ($directory);
    foreach (glob("$directory/*") as $file)
    {
        if (is_file ($file))
        {
            $filemtime = filemtime ($file);
        } else {
            $filemtime = get_m_time_dir ($file);
        }
        $last_modified_time = max($filemtime, $dirmtime, $last_modified_time);
    }
    return $last_modified_time;
}

I think you can also do like

function get_m_time_dir ($directory)
{
$last_modified_time = 0;
$handler = opendir ($directory);
while ($file = readdir ($handler))
{
    if (is_file ($directory . DIRECTORY_SEPARATOR . $file) && !($file == '.' || $file == '..'))
    {
        $filemtime = filemtime ($directory . DIRECTORY_SEPARATOR . $file);
        if ($filemtime > $last_modified_time)
        {
            $last_modified_time = $filemtime;
        }
    } else
    {

       if !($file == '.' || $file == '..'){
             $filemtime = get_m_time_dir ($directory . DIRECTORY_SEPARATOR . $file);
          if ($filemtime > $last_modified_time)
          {
             $last_modified_time = $filemtime;
          }
       }
    }
}
return $last_modified_time;
}


Longest substring in alphabetical order The code is meant to find the longest substring that is in alphabetical order.
I would like to know if I can do it more efficiently than I have done here. I am in my first week of my first introductory course, so I'm not looking for more advanced solutions. I just want to know if I'm not making it more complicated than necessary based on my current knowledge. I could't figure out how to use indexing for example.

s = 'azcbobobegghakl'

c = ""
d = ""
e = ""

for i in s:
    if c <= i:
        c = i
        d += c
        if len(d) > len(e):
            e = d
    else:
        c = i
        d = c
print('Longest substring in alphabetical order is: ' + e)

You are assigning c = i in both cases, which means you could pull this out and do this once unconditionally. This also will give you an idea of cs purpose and might give you ideas for how to name your variables in a better way. So trying to stay as close to your code as possible but with the suggested changes, your code could look something along the lines:

inputString = 'azcbobobegghakl'

prevChar = ""
currentString = ""
longestString = ""

for char in inputString:
    if prevChar <= char:
        currentString += char
        if len(currentString) > len(longestString):
            longestString = currentString
    else:
        currentString = char
    prevChar = char
print('Longest substring in alphabetical order is: ' + longestString )

Also I suggest you think about edgecases and treat these cases or at very least you make it clear (at least in a comment) what happens in those cases. As an idea of what I mean: what happens if there are several longest-strings of the same length? Which one should be printed? All? The first? The last? Just any one? What happens if the inputString is empty - your message will look a bit funny. You could output a different message in such a case.

You might think I‘m a bit pedantic, but I seriously recommend to think about your code in this way.

Other than that, I must admit I don‘t know python well enough to give you „the pythonic“ answer - I‘m not even sure if they use camelCase for python - still I hope you get the idea of the type of improvements I‘m recommending. Other than that your code looks ok to me.

Using while loops
s = "azcbobobegghakl"

c = s[0]
b = s[0]
n = 0
while n != len(s) -1:
    if s[n] <= s[n+1]:
        n+= 1
        c += s[n:n+1]
        if len(c) > len(b):
            b = c
    else:
        n += 1
        c = s[n]
print("Longest substring in alphabetical order is: "  + b)


Calculate SHA1 hash from binary and verify with a provided hash I applied for a job and they asked me to write code with the following requirements:

  Get a "toolbar offer" description from
  http..update.utorrent.com/installoffer.php?offer=conduit.  Parse the
  bencoded (see http://en.wikipedia.org/wiki/Bencode) response and
  extract the URL of the binary and its SHA1 hash.  The hash is contained
  in the value of the key 'offer_hash'. The binary is in list for the
  key 'offer_urls' — please use the one prefixed with
  "http..download.utorrent.com/".

  Download the binary and calculate its hash

  Verify that the calculated SHA1 hash is identical to the one provided
  in the offer details.  It is okay to use existing libraries for
  bencoding, hashing, etc.

What is wrong with my code? The recruiter said it didn't meet their bar.

# Script that verifies the correctness of a toolbar offers

import urllib, bencode , hashlib
from hashlib import sha1

url = "http://update.utorrent.com/installoffer.php?offer=conduit"

filename = "content.txt"

f= urllib.urlretrieve(url, filename)

#Bdecoding the response

with open (str(filename), 'rb') as myfile:
           data=myfile.read()
           decoded = bencode.bdecode(data)

# Returning the list for the key 'offer_urls'

list = decoded['offer_urls']

#print list

# Returning the URL of the binary that is prefixed with "http://download3.utorrent.com/"

length = len (list)

prefix = "http://download3.utorrent.com/"

i= -1

while i < length:
     i = i + 1
     if list[i].startswith(prefix) :
        break
binary= list[i]

print "The URL of the the binary is: " , binary

# Returning the sha1 hash contained in the value of the key 'offer_hash'

encrypted_hash = decoded['offer_hash']
sha1_hash1 = encrypted_hash.encode('hex')
print "The sha1 hash contained in the value of the key 'offer_hash' is: " , sha1_hash1

# Downloading the binary and calculating its hash

urllib.urlretrieve ( binary , "utct2-en-conduit-20130523.exe")
file = "C:\Python27\utct2-en-conduit-20130523.exe"
with open (file, 'rb') as myfile:
           downloaded=myfile.read()

k = hashlib.sha1()
k.update(downloaded)
sha1file = k.hexdigest()
print "The sha1 hash of the downloaded binary is: " , sha1file

# Verify that the calculated sha1 hash is identical to the one provided in the offer details

if (sha1file == sha1_hash1) :
    print "Test result = Pass"
else :
    print "Test result = Fail"

I'm going to tell you the things in your code that made me wonder.

# Script that verifies the correctness of a toolbar offers

import urllib, bencode , hashlib

Extra space after bencode suggests lack of attention to detail.

from hashlib import sha1

url = "http://update.utorrent.com/installoffer.php?offer=conduit"

filename = "content.txt"

f= urllib.urlretrieve(url, filename)

Use of single-letter variables suggests lack of attention to clean code.

#Bdecoding the response

with open (str(filename), 'rb') as myfile:

filename is already a string.  Converting it to a string again suggests confusion as to what is going on.

           data=myfile.read()
           decoded = bencode.bdecode(data)

Massive indentation suggests disregard for consistent indentation style.

Downloading the file to disk then reading it into memory is poor use of available APIs. You should download the file directly into memory.

# Returning the list for the key 'offer_urls'

Excess whitespace suggests someone is enter-happy.

list = decoded['offer_urls']

"Returning" is technically incorrect here. You aren't in a function, so you aren't returning anything. Use of list is a bad name for a variable since it's a built-in Python type.

#print list

# Returning the URL of the binary that is prefixed with "http://download3.utorrent.com/"

length = len (list)

Putting spaces after function calls is odd, and you don't do it consistently. There's also rarely a good reason to store the length of a list.

prefix = "http://download3.utorrent.com/"

i= -1

No space before the =.

while i < length:
     i = i + 1
     if list[i].startswith(prefix) :
        break
binary= list[i]

You are using a while loop when you should be using a for loop. It is also going to cause an IndexError, rather than fail gracefully, if the prefix isn't found.

print "The URL of the the binary is: " , binary

# Returning the sha1 hash contained in the value of the key 'offer_hash'

encrypted_hash = decoded['offer_hash']

Is that hash really encrypted?

sha1_hash1 = encrypted_hash.encode('hex')
print "The sha1 hash contained in the value of the key 'offer_hash' is: " , sha1_hash1

# Downloading the binary and calculating its hash

urllib.urlretrieve ( binary , "utct2-en-conduit-20130523.exe")
file = "C:\Python27\utct2-en-conduit-20130523.exe"

Hard-coded filename will be easy to break. You shouldn't even need to do it. The string also contains "\" which should be escaped, or the whole string should be raw. i.e.

"C:\\Python27\\utct2-en-conduit-20130425.exe"

or

r"C:\Python27\utct2-en-conduit-20130425.exe"

or in most cases you can use the other slashes, even on Windows.

"C:/Python27/utct2-en-conduit-20130425.exe"

You got away with what you did, but that's mostly because you are lucky.

with open (file, 'rb') as myfile:
           downloaded=myfile.read()

This last piece of code did pretty much the same thing as a previous bit of code, but you didn't use a function.

k = hashlib.sha1()
k.update(downloaded)
sha1file = k.hexdigest()
print "The sha1 hash of the downloaded binary is: " , sha1file

# Verify that the calculated sha1 hash is identical to the one provided in the offer details

if (sha1file == sha1_hash1) :

You've put parentheses, which are not necessary.

    print "Test result = Pass"
else :
    print "Test result = Fail"

Your basic problems:

Your style is poor and inconsistent
A few things suggest you don't quite understand what's going on
You haven't taken care to make the code readable

Here's my take on the problem:

# Script that verifies the correctness of a toolbar offers

import urllib
import bencode
import hashlib
import contextlib

# it's common practice to put input details like this
# as global constants at the start of your script
URL = "http://update.utorrent.com/installoffer.php?offer=conduit"
PREFIX = "http://download3.utorrent.com/"

# by breaking down your task into functions
# you can make the code easier to follow
def read_bencoded_url(url):
    with contextlib.closing(urllib.urlopen(url)) as offer_file:
        return bencode.bdecode(offer_file.read())

def find_string_with_prefix(strings, prefix):
    for string in strings:
        if string.startswith(prefix):
            return string
    else:
        raise Exception("Did not find prefix: {}".format(prefix))

# this function is a bit more complicated
# but it avoids saving the file to disk or loading it entirely into memory.
# instead it hashes it 4096 bytes at a time
def hash_of_url(url):
    hasher = hashlib.sha1()
    with contextlib.closing(urllib.urlopen(url)) as binary_file:
        while True:
            data = binary_file.read(4096)
            if not data:
                break
            hasher.update(data)
    return hasher.hexdigest()

# the actual high level logic just calls the functions
# this avoid obscuring the logic with lower level details
def main():
    decoded = read_bencoded_url(URL)
    binary = find_string_with_prefix(decoded['offer_urls'], PREFIX)
    reported_hash = decoded['offer_hash'].encode('hex')
    actual_hash = hash_of_url(binary)
    print "The sha1 hash contained in the value of the key 'offer_hash' is: ", reported_hash
    print "The sha1 hash of the downloaded binary is: " , actual_hash

    if reported_hash == actual_hash:
        print "Test result = Pass"
    else:
        print "Test result = Fail"

if __name__ == '__main__':
     main()

Problems:

I would say that import should be an efficient way of reusing the libraries.

import urllib, bencode , hashlib
from hashlib import sha1

In the above line of code you have imported hashlib and sha1 from hashlib on the second line. There is nothing wrong in that, but when you just want to use only sha1, then no need to import the entire class.

Same with other classes: how many members of the above mentioned libraries have you used in your code?
All the constants should be predefined at the beginning of the script, not wherever required.
What if the above file failed to open (in case file not present)?  How do you deal with this exception?

with open (str(filename), 'rb') as myfile:
       downloaded=myfile.read()

Use try-except to catch exceptions.
Filename is already a string.
No need to use an extra variable to store the length of the list.  Also there is a space in between the function len and its arg.

length = len (list)

Use a for loop for indexing, which is better than using a while loop.
Meaningless variable names such as i, f, k.
The tasks are not well-divided into different subroutines.


Object-oriented purchase data structure I wrote a code where customer can buy an item according to customer balance, price and quantity of item to be bought. Here is the code:

#include "bits/stdc++.h"
using namespace std;

class Item{
private:
string itemName;
string itemidNo;
int itemQuantity;
double itemPrice;
public:
Item(){
    itemPrice = 500;
    itemQuantity = 1;
}
//getters and setters
void setName(string name){itemName = name;}
void setId(string id){itemidNo = id;}
void setQuantity(int quantity){itemQuantity = quantity;}
void setPrice(double price){itemPrice = price;}

string getName(){return itemName;}
string getId(){return itemidNo;}
int getQuantity(){return itemQuantity;}
double getPrice(){return itemPrice;}

};

class customer {
private:
string Custname;
string CustidNo;
double Custbalance;
Item Custitem;
public:
customer(){
    Custbalance = 5000;
}
//getters and setters
void setName(string name){Custname = name;}
void setidNo(string id){CustidNo = id;}
void setbalance(double balance){Custbalance = balance;}

string getName(){return Custname;}
string getidNo(){return CustidNo;}
double getbalance(){return Custbalance;}

void print(){
    cout << Custitem.getName() << " " << Custitem.getId() << " " << Custitem.getQuantity() << " " << Custitem.getPrice() << "\n";
    Custbalance -= Custitem.getQuantity()*Custitem.getPrice();
    cout << Custbalance << "\n";
}

void buyItem(Item item){
    if(Custbalance < (item.getQuantity()*item.getPrice())){
        cout << "Insufficient Balance for " << item.getName() << "\n";
        return ;
    }
    if(item.getQuantity() < 1){
        cout << "order not valid";
        return;
    }
    Custitem = item;
    print();
}
};

int main(int argc, char const *argv[])
{
customer harsh;
harsh.setName("harsh");
harsh.setidNo("23");

Item item1;
item1.setName("mobile");
item1.setId("44");

harsh.buyItem(item1);

Item item2;
item2.setName("speakers");
item2.setId("F&D");
item2.setQuantity(2);
item2.setPrice(3000);

harsh.buyItem(item2);

return 0;
}

I am a bit confused as I think there is too much redundancy in my code, such as the getters and setters.  How can I make my code more object-oriented?

Headers and namespaces
#include "bits/stdc++.h"

That's not a standard C++ header; prefer to use standard headers so that others can use your code.
using namespace std;

Bringing all names in from a namespace is problematic; namespace std particularly so. See Why is “using namespace std” considered bad practice?.
Naming
Choose a consistent naming convention and stick to it.  A common one that's closest to the majority of your code is to use PascalCase for types, camelCase for members and snake_case for variables.
Class Base
A default-constructed Base will have default-constructed Id and Name members.  It probably makes no sense to default-construct; I think it's better to explicitly declare a constructor:
public:
    Base(std::string id, std::string name)
      : id(std::move(id)), name(std::move(name))
    {}

Once initialized, there should be no reason to change the name or id, so we can remove the setters.  The accessors can be made const, so we can call them on instances of const Base:
    std::string getName() const { return name; }
    std::string getId() const { return id; }

Class Item
This seems mis-named - it's more like an "order line" that specifies how many items and their price.  Again, a default constructor seems wrong, although defaulting the quantity to 1 makes sense:
public:
    Item(std::string id, std::string name, double price, int quantity = 1)
      : Base(std::move(id), std::move(name)),
        itemPrice(price),
        itemQuantity(quantity)
    {
        if (itemQuantity <= 0)
            throw new std::domain_error("negative quantity");
        if (itemPrice <= 0)
            throw new std::domain_error("negative price");
    }

Whether it's reasonably to modify the price and quantity after construction really depends on your application, so I can't comment on that, but the getters should both be marked const, like the ones for id and name.
Also, I'll add the standard warning that floating-point is usually a poor choice for financial accounting - you'll avoid numeric rounding if you choose instead to represent currency amounts in integer values of the smallest denomination.
Class customer
Although customers may have names and identifiers, does that really make them the same kind of thing as items?  Unless you have a use case where you may want to use them interchangeably, public inheritance isn't really what you need here.  It's a common trap in OO design to assume that because two things are made from similar pieces that they must be similar things.  In some cases, it really is mere coincidence.  By way of analogy, chairs and dogs are both quadrupeds, but blindly choosing to treat them alike is a mistake that could turn around and bite you.
It is extremely kind of you to credit all your new customers with 5000 currency units in their accounts - but it might not be good business sense (even if they are Zimbabwean Third Dollars).  I recommend 0. as a more sensible default.
If the customer object is intended to be an autonomous object, it should be responsible for crediting and debiting the account, rather than setting the amount; I'd expect something more like:
void deposit(Currency amount)
{
    balance += amount;
}
void withdraw(Currency amount)
{
    balance -= amount;
}
Currency balance() const { return amount; }

(Real implementations usually involve some locking, so that concurrent operations don't interfere with each other, but that's a whole review in itself)
Why can a customer buy only one Item?  Usually, an order is composed of several items (each with a quantity) and refers to a customer (who may at a later date order more items - it's bad business to restrict them from doing that).
Something that looks really strange is this:
void print(){
    cout << Custitem.getName() << " " << Custitem.getId() << " " << Custitem.getQuantity() << " " << Custitem.getPrice() << "\n";
    Custbalance -= Custitem.getQuantity()*Custitem.getPrice();
    cout << Custbalance << "\n";
}

Why does the customer have to pay each time you print their order?
Usually, print() would look something like
std::ostream& print(std::ostream& o) const
{
    return o
      << Custitem.getName()
      << " " << Custitem.getId()
      << " " << Custitem.getQuantity()
      << " " << Custitem.getPrice() << "\n"
      << Custbalance << "\n";
}

and the debiting of the account would be a separate (non-const) operation.

after lot of messing around with code , I found a way to reduce the retundancy , since customer and item share two same attributes name and id so i made an interface for the corresponding. Here is my refactored code -

#include "bits/stdc++.h"
using namespace std;

class Base{
//item and name both have two common properties name and id
protected:
string Name;
string Id;

public:
void setName(string name){Name = name;}
void setId(string id){Id = id;}

string getName(){return Name;}
string getId(){return Id;}
};

class Item : public Base{
private:
int itemQuantity;
double itemPrice;
public:
Item(){
    itemPrice = 500;
    itemQuantity = 1;
}
//getters and setters
void setQuantity(int quantity){itemQuantity = quantity;}
void setPrice(double price){itemPrice = price;}

int getQuantity(){return itemQuantity;}
double getPrice(){return itemPrice;}

};

class customer : public Base{
private:
double Custbalance;
Item Custitem;
public:
customer(){
    Custbalance = 5000;
}
//getters and setters
void setbalance(double balance){Custbalance = balance;}

double getbalance(){return Custbalance;}

void print(){
    cout << Custitem.getName() << " " << Custitem.getId() << " " << Custitem.getQuantity() << " " << Custitem.getPrice() << "\n";
    Custbalance -= Custitem.getQuantity()*Custitem.getPrice();
    cout << Custbalance << "\n";
}

void buyItem(Item item){
    if(Custbalance < (item.getQuantity()*item.getPrice())){
        cout << "Insufficient Balance for " << item.getName() << "\n";
        return ;
    }
    if(item.getQuantity() < 1){
        cout << "order not valid";
        return;
    }
    Custitem = item;
    print();
}
};

int main(int argc, char const *argv[])
{
customer harsh;
harsh.setName("harsh");
harsh.setId("23");

Item item1;
item1.setName("mobile");
item1.setId("44");

harsh.buyItem(item1);

Item item2;
item2.setName("speakers");
item2.setId("F&D");
item2.setQuantity(2);
item2.setPrice(1000);

harsh.buyItem(item2);

return 0;
}


Determining if a C-style string is alphabetic I'm looking for the fastest way to determine if a C-style string contains only alphabetic characters.

For this question, there are a few assumptions:

strlen(c) > 0
Strings are null-terminated
Continuous character encoding from a-z and A-Z is not guaranteed, but likely.
The string is not a null pointer, nor is the pointer its "default value".

#include <stdbool.h>
#include <ctype.h>

bool stralpha(const char *c)
{
    bool alphabetic = true;

    while(*c)
        alphabetic &= !!isalpha(*c++);

    return alphabetic;
}

/* so gcc does not complain */
int main(void){}

To be clear: I know that any performance difference in such a function will be minimal at best. This is just for fun.

In my case, I was having issues with the function returning false even when c was purely alphabetic. That is why I use double negation !! on isalpha(), because it is not guaranteed to return consistently 0 or 1 per the standard.

The main problem with your code (in terms of performance) is that it processes every character in the string regardless. It's probably better to actually exit early if you find a non-alpha character, with something like:

bool isStrAlpha (const char *chPtr) {
    // if (*chPtr == '\0') return false;
    while (*chPtr != '\0')
        if (! isalpha (*chPtr++))
            return false;
    return true;
}

And just a couple of extra notes:

This code works fine even for zero-length strings, assuming the intent is to ensure whatever characters are in the string are all alpha. In other words, it considers an empty string valid because it contains no non-alpha characters. If you want to ensure the string has to have at least one alpha, just reinstate the commented-out line at the start of the function.
Your second assumption is superfluous. A series of characters that are not terminated with \0 is not actually a string in C.
Whether alpha characters are likely to be consecutive is not relevant in my opinion, I usually prefer correct, portable code over speed. But, in any case, you can be reasonably certain that the people who created your implementation will have optimised the wazoo out of the character classification stuff.

The above answers using the is_alpha array are expensive in terms of memory.

you can use:

bool is_alpha(char c) {
     //A-Z
     if( (c>=65) && (c<=90) )
         return true;
     if( (c>=97) && (c<=122) )
         return true;
     return false;
}

all the conditions can be in one line, it is for clarity.


Numerology Calculator I've built the beginnings of a numerology calculator, and would like an experienced javascript developer to review this code.

It works as expected, but I can't help but think there is a better way to write it instead of using multiple for loops.

There are a few calculations that have to be made, so I can't see any way around it. Any direction would be great.

function addNumbers(arr, fn) {
    var arrayTotal = [],
    singleDigits = [];

    for (i = 0; i < arr.length; i++) {
        arrayTotal.push(
            fn(arr[i])
        );
    }

    for (value in arrayTotal) {
        for (num in arrayTotal[value]) {
            singleDigits.push(parseInt(arrayTotal[value][num]));
        }
    }

    for (i = 0, sum = 0; i < singleDigits.length; i++) {
        sum += singleDigits[i];
    }

    for (i = 0, numericValue = 0, sum = fn(sum); i < sum.length; i++) {
        numericValue += parseInt(sum[i]);
    }

    return numericValue;
}

function splitNumbers(item) {
    return item.toString().split('');
}

var value = addNumbers([13, 11, 1938], splitNumbers);
document.body.innerHTML = value;

map and reduce are tailor made for problems like this.

To understand the while loop in the final version of the code at the bottom of this post, it may help to consider a "longhand" version that does the same thing:

// this sets the answer we seek to the sum of all the digits
// of every number passed in via the array
finalAnswer = addDigits(combinedDigits);

// but since this sum may still be larger than a 1 digit
// number (ie, 10 or more), we need to continue the summing
// the digits until we end up with a single digit number
while ( finalAnswer > 9 ) {
  // this converts an integer to a string
  combinedDigits = '' + finalAnswer;
  // this sums the digits again
  finalAnswer = addDigits(combinedDigits);
}

Here's the final, complete version, with the more compact while loop.  We are simply combining some of the steps above.

function addDigitsRecursively(arr) {

  var combinedDigits = arr.join(''), finalAnswer;

  while ( (finalAnswer = addDigits(combinedDigits)) > 9 )
    combinedDigits = '' + finalAnswer;

  function addDigits(str) {
    return str.split('').map(function(n) {return parseInt(n)})
                        .reduce(function(a, b){return a+b;})
  }

  return finalAnswer;
}

var value = addDigitsRecursively([13, 11, 1938]);
document.body.innerHTML = value;

Just thought I'd post this. After reading up on map() and reduce(), I've realised they are only supported by IE9+. Taking some inspiration from Jonah's post, I've managed to slim down the code using two for loops. See below.

function mapForEach(arr) {
    var combinedNumbers = arr.join(''),
        totalValue = 0,
        finalValue = 0;

    for (i = 0; i < combinedNumbers.length; i++) {
        totalValue += parseInt(combinedNumbers[i]);
    }

    for (i = 0, totalValue += ''; i < totalValue.length; i++) {
        finalValue += parseInt(totalValue[i]);
    }

    return finalValue;
}

var myArray = [13, 11, 1938],
    value = mapForEach(myArray);

document.body.innerHTML = value;


LINQ Query returning all original objects I'm just refactoring some code I've come across whilst fixing a bug, but just wanted a quick sanity check to make sure I'm not missing any reason for keeping what I'm going to remove.

The current code:

uxButton.Enabled = ((from a in actions select a).Distinct().Count() == 1);

There really doesn't seem to be a need for the LINQ query, so:

uxButton.Enabled = actions.Distinct().Count() == 1;

Should do the trick, right?

As the other answers stated your given code does do the same thing, but could give performance issues for big collections since the entire collection is traversed when calling Count().

The following code should solve this:

var distinct = actions.Distinct();
uxButton.Enabled = distinct.Take( 2 ).Count() == 1;

Perhaps you could turn this into an extension method:

uxButton.Enabled = actions.Distinct().CountOf( 1 );

It seemed useful, so I added it to my library, along with a unit test.

/// <summary>
///   Returns whether the sequence contains a certain amount of elements.
/// </summary>
/// <typeparam name = "T">The type of the elements of the input sequence.</typeparam>
/// <param name = "source">The source for this extension method.</param>
/// <param name = "count">The amount of elements the sequence should contain.</param>
/// <returns>
///   True when the sequence contains the specified amount of elements, false otherwise.
////</returns>
public static bool CountOf<T>( this IEnumerable<T> source, int count )
{
    Contract.Requires( source != null );
    Contract.Requires( count >= 0 );

    return source.Take( count + 1 ).Count() == count;
}

Yes, you are right, the LINQ query is superfluous and you can just write actions instead.

The only comment I’d have about the code is that .Distinct().Count() will go through the entire collection, but you can stop when a non-distinct element is found. So personally I’d use this instead:

uxButton.Enabled = actions.Distinct().Skip(1).Any();


Select case string comparison I have a select case that checks against a string variable.  It's starting to get large and I am wondering if there is a better way of doing this?

Here is a sample of the Select Case statement:

Select Case Param.ToLower
    Case "dashboard_home_vm"
        Return _Locator.DashboardHome_VM
    Case "job_list_vm"
        Return _Locator.JobList_VM
    Case "job_add_vm"
        Return _Locator.JobAdd_VM
    Case "client_add_vm"
        Return _Locator.ClientAdd_VM
    Case "client_list_vm"
        Return _Locator.ClientList_VM
    Case "client_report_vm"
        Return _Locator.ClientReport_VM
    Case "employee_add_vm"
        Return _Locator.EmployeeAdd_VM
    Case "employee_certification_vm"
        Return _Locator.EmployeeCertification_VM
    Case "employee_list_vm"
        Return _Locator.EmployeeList_VM
    Case "employee_report_vm"
        Return _Locator.EmployeeReport_VM
    Case "employee_role_vm"
        Return _Locator.EmployeeRole_VM
    Case "employee_timeslip_vm"
        Return _Locator.EmployeeTimeslip_VM
    Case "product_add_vm"
        Return _Locator.ProductAdd_VM
    Case "product_category_vm"
        Return _Locator.ProductCategory_VM
    Case "product_list_vm"
        Return _Locator.ProductList_VM
    Case "product_type_vm"
        Return _Locator.ProductType_VM
    Case "product_unit_vm"
        Return _Locator.ProductUnit_VM
    Case "tracking_stock_vm"
        Return _Locator.TrackingInStock_VM
    Case "tracking_list_vm"
        Return _Locator.TrackingList_VM
    Case "tracking_site_vm"
        Return _Locator.TrackingOnSite_VM
    Case "vendor_add_vm"
        Return _Locator.VendorAdd_VM
    Case "vendor_list_vm"
        Return _Locator.VendorList_VM
    Case "vendor_report_vm"
        Return _Locator.VendorReport_VM
    Case Else
        Return Nothing
End Select

This actual list will be about 50 - 80 comparisons.  Is this the fastest way to do this or is there a better way?

A classic solution to this problem is to use a data table - in your case probably in form of a dictionary. It depends of what type your values are - I assume they are some sort of view models? I'm also going to assume they have a common base class:

One time setup:

Dim dict As New Dictionary(Of String, ViewModelBase)
dict.Add("dashboard_home_vm", _Locator.DashboardHome_VM)
...

You can look them up simply by

Dim vm As ViewModelBase
If (dict.TryGetValue(Param.ToLower, vm)) Then
    return vm
End If
return Nothing

Adding a new view model would be achieved by simply adding a new entry to the dictionary.

Cant you just use the output of param.tolower after _locator.?

return _locator.(Param.ToLower)

try if that works


Identifying array elements, rearranging them, and replacing them I am completing the CodingBat exercises for Java, and, having learnt from the answers to the previous one, I completed the one that follows:

  Return a version of the given array where all the 10s have been removed. The remaining elements should shift left towards the start of the array as needed, and the empty spaces a the end of the array should be 0. So {1, 10, 10, 2} yields {1, 2, 0, 0}. You may modify and return the given array or make a new array.

Here is my code:

public int[] withoutTen(int[] nums) {

    int slow = 0;

    for (int fast = 0; fast < nums.length; fast++) {
        if (nums[fast] != 10) {
            nums[slow] = nums[fast];
            slow++;
        }
        if (fast == nums.length-1) {
            for (int i = fast; i >= slow; i--) {
                nums[i] = 0;
            }
        }
    }
    return nums;

}

Please bear in mind I am doing these without importing anything extra like java.util.Arrays etc., as, primarily, this is not accepted by the assessor and, secondarily, I want to get to grips with arrays without importing anything extra yet.

I was taking influence from the comments I received on the previous question, by using a fast and slow variable for looping through the array, and limiting the code to one main for loop, for the sake of making my code generally better and putting efficiency into practice.

How could this be optimised? Is it ok to begin another loop only for the circumstance of fast reaching the last element in the array? It feels like I am repeating myself by saying "if we get to the last element..." because we absolutely will do anyway.

I played around with setting fast outside the loops, so that the second for loop can begin after the first (as opposed to nesting it). At that point, fast would be set to the last value anyway from having completed the previous loop, but more and more values would be set outside the loops. Which way makes more sense?

It's extraordinarily confusing to have if (fast == nums.length - 1) as a conditional within a loop that continues while fast < nums.count returns true.  This is the last part of the loop, and it only executes on the last iteration of the loop... so why is it even in the loop at all?

A couple of comments in our code can help us out.  We have two distinct parts:

Shuffling the non-tens to the front.
Zero-ing out the tens.

So let's use comments to mark these in distinct blocks.  And, let's move the second part to its own loop.

And while we're at it, let's make the method more generic to replace any number with another given number.

public int[] replaceNum(int[] nums, int eliminationNum, int replaceNum) {
    int nextMoveIndex = 0;

    // shuffle non-elims to the front
    for (int i = 0; i < nums.length; ++i) {
        if (nums[i] != eliminationNum) {
            nums[nextMoveIndex++] = nums[i];
        }
    }

    // zero out the elimination number
    while (nextMoveIndex < nums.length) {
        nums[nextMoveIndex++] = replaceNum;
    }

    return nums;
}

Here's my solution. I have two solutions, but the first one is better because it doesn't waste resources.

public int[] withoutTen(int[] nums) {
        //Create an ArrayList to add the iterations where ten is.
        ArrayList<Integer> tenPlaces = new ArrayList<Integer>();
        //Get the place where the tens
        //Create a return array to manipulate, set initally to nums.
        int[] ret = nums;
        for (int i = 0; i < ret.length; i++)
        {
          //If we find a number that is not a ten, we add it's place to the ArrayList
          if (ret[i] != 10)
          {
            tenPlaces.add(i);
          }
          //Otherwise, if it is ten, we change it to zero.
          else
          {
            ret[i] = 0;
          }
        }

        //Sort the return array by using the ArrayList.
        for (int i = 0; i < tenPlaces.size(); i++)
        {
          int temp = ret[i];
          ret[i] = ret[tenPlaces.get(i)];
          ret[tenPlaces.get(i)] = temp;
        }
        //Finally return ret;
        return ret;

        /*
        Other Solution
        int size = nums.length;
        int tenAmt = 0;
        ArrayList<Integer> foo = new ArrayList<Integer>();
        for (int i = 0; i < size; i++)
        {
          if (nums[i] == 10)
          {
            tenAmt++;
          }
          else
          {
            foo.add(nums[i]);
          }
        }
        int amtLeft = size - tenAmt + 1;
        for (int i = 0; i < amtLeft; i++)
        {
          foo.add(0);
        }
        int[] ret = new int[size];
        for (int i = 0; i < size; i++)
        {
          ret[i] = foo.get(i);
        }
        return ret;
        */
  }


Print a word, one character per line, backwards I have a problem that I solved in 2 different ways and I would like to know which one is better, cleaner and easier to read.
Problem:
Write a while loop that starts at the last character in the
string and works its way backwards to the first character in the string,
printing each letter on a separate line.
Example 1:
char = 'Wednesday'
cont = -1
cont2 = len(char)
cont3 = len(char) + 1
while cont < cont2:
    print(char[cont2:cont3])
    cont2 = cont2 - 1
    cont3 = cont3 - 1

Example 2:
char = 'Wednesday'
cont = len(char) - 1
while cont >= 0:
    cont2 = char[cont]
    print(cont2)
    cont -= 1

Looking at the two implementations side by side, ignoring the goal of the exercise and alternative implementations, I'd say the second example is better:

It uses one fewer variables.
It does a simple index lookup rather than a slice.
The implementation is easier to follow.

For simple problems like this it's often useful to think of the complexity of the solution as the number of "tokens", or distinct language elements, necessary to write it down. For example, print(cont2) is four tokens: print, (, cont2 and ). By a quick read the first example has about twice as many tokens as the second.

Another reason the first one is worse is that it prints an extra empty line at the beginning due to your off-by-one error. The only reason you don't get an error is that Python plays nice when you ask for out-of-bounds slices and gives you an empty string.


Check if a given string matches a wildcard pattern I'm trying to optimize my solution to the following question:

given a regular expression with chars and special char '*', (star which is a joker we can replace with any string), and a string, write a program which determines if the regular expression can represent that string or not.
for example: "a'*b" can represent "aaaacbbbb" , in contrast "a'*b'*c" cant represent "aaaabcccccccccb"

I solved the problem and inserted the code below, I want to optimize either space or time, and if possible to make it more readable or shorter.
#include <stdio.h>
#include <string.h>

int compareHelper(char *s, char *regex, int star);

int compareStrings(char *s, char *r);

int main()
{
    printf("%d\n", compareStrings("aaaacbbb", "a*b"));
    printf("%d\n", compareStrings("aaaabccccccccb", "a*b*c"));
    return 0;

}

//aaaaacbbb
//a*b

int compareStrings(char *s, char *r)
{
    return compareHelper(s, r, 0);

}

int compareHelper(char *s, char *regex, int star)
{
    if (*s == '\0' && *regex == '\0')
    {
        return 1;
    } else if (*regex == '*')
    {
        return compareHelper(s, regex + 1, 1); // star is seen hence will be set to true.
    } else
    {
        if (!star)
        {
            if (*s != *regex)
            { // star was not seen and they differ in char
                return 0;
            } else
            {
                return compareHelper(s + 1, regex + 1, star);
            }

        } else
        {
            while (*s && *s != *regex)
            {
                s++;
            }
            if (*s == '\0') return 0;
            else
            {
                return compareHelper(s + 1, regex + 1, 0) | compareHelper(s + 1, regex, 1);
            }
        }
    }
}

The code you wrote can be improved in lots of places. I'll go through it from top to bottom.
#include <stdio.h>
#include <string.h>

Perfect until now. The #include lines from the C standard library should be in alphabetic order.  I cannot say whether you did this intentionally or not since there's a 50-50 chance. :)
int compareHelper(char *s, char *regex, int star);

This function should be declared as static, since it is local to this file. To do this, replace the int compareHelper with static int compareHelper.
The compare in the function name is wrong. Typically, to compare two things, they have to be of the same or at least a compatible type. The technical type here is char *, but on a more abstract level, s is an arbitrary string while regex is a string with some constraints. Instead of compare, this function should be called match.
The type of the string parameters should be const char * instead of char * since this function does not modify the characters of these strings.
The type of the parameter star should be bool instead of int. To get this type, you have to add #include <stdbool.h> along with the other #include lines.
The name of the parameter regex is wrong. The word regex means regular expression, and this is a fixed term with a precise definition. Your patterns are not regular expressions, therefore the parameter name confuses the reader. Better call it pattern.
int compareStrings(char *s, char *r);

Again, the compare should rather be match, the parameter types should get an additional const, and the return type should be bool instead of int.
int main()
{
    printf("%d\n", compareStrings("aaaacbbb", "a*b"));
    printf("%d\n", compareStrings("aaaabccccccccb", "a*b*c"));
    return 0;
}

The empty parentheses () in the function declaration of main mean that the function can take any number of parameters. In C, this is only allowed for legacy code from 1990 and earlier. In modern code, write (void) instead to clearly say that there must be no parameters.
//aaaaacbbb
//a*b

This is the beginning of documentation, which is a good idea. To be useful for other readers, you should write in whole sentences though, for example:

// See if the string s matches the pattern p.
// In the pattern, '*' matches an arbitrarily long sequence of arbitrary characters.

int compareHelper(char *s, char *regex, int star)
{
    if (*s == '\0' && *regex == '\0')
    {
        return 1;
    } else if (*regex == '*')

After the return 1, the else is unnecessary. I would write the code like this instead:

    if (*s == '\0' && *regex == '\0') {
        return 1;
    }

    if (*regex == '*') {
        ...

This way, your code has one topic per paragraph. The empty line between the topics clearly marks a pointer where the reader may stop reading and take a deep breath, before starting to read the next paragraph.
The rest of the code looks fine, except for this line:
return compareHelper(s + 1, regex + 1, 0) | compareHelper(s + 1, regex, 1);

The operator | means a logical or, and it always evaluates both operands. This means that compareHelper is called twice, even if the left-hand side already evaluates to true. This is not necessary. To only evaluate the necessary parts of the condition, replace the | operator with ||.
All in all, the code is not too long. What makes it look verbose is that you put each { or } on its own line. You can rewrite it like this:
#include <stdbool.h>

int compareHelper(const char *s, const char *regex, bool star)
{
    if (*s == '\0' && *regex == '\0')
        return true;

    if (*regex == '*')
        return compareHelper(s, regex + 1, 1); // star is seen hence will be set to true.

    if (!star) {
        if (*s != *regex)
            return false; // star was not seen and they differ in char
        else
            return compareHelper(s + 1, regex + 1, star);
    }

    // seen a star; look for the text after the star
    while (*s && *s != *regex)
        s++;

    if (*s == '\0')
        return 0;

    return compareHelper(s + 1, regex + 1, 0) || compareHelper(s + 1, regex, 1);
}

Some people will complain that "you must always use braces in if and while statements", but that's not necessary. If you properly indent your code or let your IDE format the code automatically, the indentation properly expresses the relation of the code pieces to each other. The typical example is:
if (condition)
    goto fail;
    goto fail;

Modern compilers not only translate the source code to machine code, they also check for various possible bugs in the code. Wrong indentation like in the above snippet is one of them.
And here's a complete implementation that also matches character ranges like [A-Z]. The code looks remarkably similar to yours, which is good. If you take that code and remove the part that handles the [A-Z] part, it will be very similar to your code. Actually, taking other people's code and editing it just trying to understand it is an efficient way of thinking about why each of these lines of code is actually necessary.
While reading that code, you may notice that your code might fail for str_match("a good game", "a *game"). You should verify that the search after the star looks at every possible position in the string, not only the first time the letter g appears.

// The main function that checks if two given strings
// match. The first string may contain wildcard characters
bool match(char *first, char *second)
{
    // If we reach at the end of both strings, we are done
    if (*first == '\0' && *second == '\0')
        return true;

    // Make sure that the characters after '*' are present
    // in second string. This function assumes that the first
    // string will not contain two consecutive '*'
    if (*first == '*' && *(first + 1) != '\0' && *second == '\0')
        return false;

    // If the first string contains '?', or current characters
    // of both strings match
    if (*first == *second)
    {
        return match(first + 1, second + 1);
    }

    // If there is *, then there are two possibilities
    // a) We consider current character of second string
    // b) We ignore current character of second string.
    if (*first == '*')
        return match(first + 1, second) || match(first, second + 1);
    return false;
}
```


C++ calculator using classes This is my first time using classes objects and functions. What can I improve on?

#include <iostream>
#include <conio.h>
using namespace std;

class functions{
public:
    void Body(){
        cout << " :: Welcome to Taylor's CALCULATOR! ::" << endl;
    }
    int Addition(int x, int y){
        int ans = x + y;
        return ans;
    }
    int Subtraction(int x, int y){
        int ans = x - y;
        return ans;
    }
    int Multiplication(int x, int y){
        int ans = x * y;
        return ans;
    }
    int Division(int x, int y){
        int ans = x / y;
        return ans;
    }
};

int main(){
int func;
int x, y;

functions key; //Object
key.Body(); //Object

cout << "What function do you want to use? " << endl;
cout << "1 - Addition " << endl;
cout << "2 - Subtraction " << endl;
cout << "3 - Multiplication " << endl;
cout << "4 - Division " << endl;
cout << "Input: " << endl;

cin >> func;
cout << endl;

switch(func){

    case 1: //Addition
        cout << "**ADDITION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " + " << y << " = ";
        cout << key.Addition(x, y);
        break;
    case 2: //Subtraction
        cout << "**SUBTRACTION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " - " << y << " = ";
        cout << key.Subtraction(x, y);
        break;
    case 3: //Multiplication
        cout << "**MULTIPLICATION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " x " << y << " = ";
        cout << key.Multiplication(x, y);
        break;
    case 4: //Division
        cout << "**DIVISION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " / " << y << " = ";
        cout << key.Division(x, y);
        break;
    default:
        cout << "Invalid Input...";
        break;
}

}

Unless you'll be maintaining an accumulated number for multiple operations, this may not be the best use of classes.  For your code, you'd get the same effect by just performing these calculations within the switch statement (something similar to this).  Right now, you're just using a class to contain similar functions, including a trivial output function.

Some additional notes:

Try not to use using namespace std.
Indent everything within main() as well.  Not doing so may make it hard to tell that the code is being contained within something.
You don't need <conio.h> here, so just remove it.
It may be useful to prevent the user from dividing by 0, which is undefined behavior.

There are some good answers here. My two cents, keeping my suggestions small and simple regarding the class is:

class Calculator{ //Classes start with capital letters.
public:

    void message(){//'Body' is vague, 'message' isn't.
        std::cout << " :: Welcome to Taylor's CALCULATOR! ::" << std::endl;
    }

    double addition(const double & x, const double & y){ //Pass by reference to reduce copies.
        return x + y; // No need to creat new variables, just return a result.
    }

    double subtraction(const double & x, const double & y){ //No need to capitalize methods of a class.
        return x-y;
    }

    double multiplication(const double & x, const double & y){ //Passed values aren't mutable, so declare them const.
        return x * y;
    }

    double division(const double & x, const double & y){ //Return values are mutable, so don't declare them const.
        if(y==0){
            std::cout << "There was a divide-by-zero error in the division function.\n Don't divide by zero please." << std::end;
            return 0;
        }
        return x / y;
    }
};

Double may be better suited in case a result requires floating point.


Using return value vs out parameters It's very popular to see code like this:

bool DoSomething([some arguments]);

You can't understand the result of this method by looking at its name (it could actually be a void).

What's the best approach to refactor such kind of methods?  What I have in mind is something like this:

void DoSomething([some argumens],out bool everythingIsOk);

What do you think?

EDIT

It seems that so many people prefer

bool TryDoSomething();

rather than

void DoSomething(out bool success);

But what if a method is going to have a result instead of being void?  Can we use int.TryParse pattern or we should return the result and throw an exception if something goes wrong?

You’re asking the wrong question. In reality, both methods are bad. If you need the success message, either throw an exception on failure (but only if this is really an exceptional scenario, or return the success and indicate this in the method name. For instance, by calling the method TrySomething.

In general, your example sounds like the method is actually doing two distinct things, otherwise there would be no confusion in the first place. But this is code smell anyway: every method should have only one purpose. If that is the case, then the meaning of the return value is always clear, no need to give it a name.

In this case is not requied to refactor but what if you need two output parameters?
Example:

public bool DoSomething(object parameter, out ref string ErrorDescription)
{
try {
...
}
catch (Exception ex)
{
 ErrorDescription = ex.Message;
 return false;
}
ErrorDescription = "ok.";
return true;
}

If you would not use output parameter, you had to use any public property or create class as return type. This is easier.


Harvard CS50 Problem Set 1: greedy change-making algorithm The goal of this code is to take dollar or cents input from the user and give out minimum number of coins needed to pay that between quarters, dimes, nickels and pennies. If this code can be shortened, how would one do it?

#include <stdio.h>
#include <cs50.h>
#include <ctype.h>
#include <math.h>

int main(void)
{
float b;
do
    {
    printf("please give balance owing: ");
    b = GetFloat();
    }
while (b<0);

int v;
int q = 25;
int d = 10;
int n = 5;
int p = 1;
int count=0;

b=b*100;
int c=round(b);
int balance = c;
printf("%i\n", balance);
//printf("%.2f\n", round(c));

while(balance>=q)
    {
    v=balance/q;
    count=count+v;
    balance=balance-(q*v);
    }
while(balance>=d)
    {
    v=balance/d;
    count=count+v;
    balance=balance-(d*v);
    }
while(balance>=n)
    {
    v=balance/n;
    count=count+v;
    balance=balance-(n*v);
    }
while(balance>=p)
    {
    v=balance/p;
    count=count+v;
    balance=balance-(p*v);
    }

printf("the number of coins needed is: %i\n", count);
printf("balance after coins paid: %i\n", balance);

}

The first thing that sticks out to me is indentation.  But I'm going to recommend some other changes, so I'll address indentation last.

Let's give our variables some better, more descriptive names, and let's declare our constants as constants.  And since we're taking in a floating point number for input from the user, we need to compare that with other floating point numbers.  Or we need to use it as an integer.

int const kVALUE_QUARTER = 25;
int const kVALUE_DIME = 10;
int const kVALUE_NICKEL = 5;
int const kVALUE_PENNY = 1;

float userInput = -1.0;
int balance = 0;
int coinCount = 0;

do {
    printf("please give balance owing: ");
    userInput = GetFloat();
} while (userInput < 0);

balance = (int)(userInput * 100.0);

Now, we can eliminate all of our loops using the modulo operator (%) and integer division.

// number of quarters:
coinCount += balance / kVALUE_QUARTER;
balance %= kVALUE_QUARTER;

// number of dimes:
coinCount += balance / kVALUE_DIME;
balance %= kVALUE_DIME;

// number of nickels:
coinCount += balance / kVALUE_NICKEL;
balance %= kVALUE_NICKEL;

// number of pennies:
coinCount += balance / kVALUE_PENNY;
balance %= kVALUE_PENNY;

Now let's put it all together and take a look at our indentation:

#include <stdio.h>
#include <cs50.h>
#include <ctype.h>
#include <math.h>

int main(void) {
    int const kVALUE_QUARTER = 25;
    int const kVALUE_DIME = 10;
    int const kVALUE_NICKEL = 5;
    int const kVALUE_PENNY = 1;

    float userInput = -1.0;
    int balance = 0;
    int coinCount = 0;

    do {
        printf("please give balance owing: ");
        userInput = GetFloat();
    } while (userInput < 0);

    balance = (int)(userInput * 100.0);

    // number of quarters:
    coinCount += balance / kVALUE_QUARTER;
    balance %= kVALUE_QUARTER;

    // number of dimes:
    coinCount += balance / kVALUE_DIME;
    balance %= kVALUE_DIME;

    // number of nickels:
    coinCount += balance / kVALUE_NICKEL;
    balance %= kVALUE_NICKEL;

    // number of pennies:
    coinCount += balance / kVALUE_PENNY;
    balance %= kVALUE_PENNY;

    printf("the number of coins needed is: %i\n", count);
    printf("balance after coins paid: %i\n", balance);
}

Why make this more complicated than it has to be?

#include <stdio.h>

void change(int cents)
  {
  int q, d, n, p;

  q = cents / 25;
  d = (cents - (q * 25)) / 10;
  n = ((cents - (q * 25) - (d * 10)) / 5);
  p = cents - (q * 25) - (d * 10) - (n * 5);

  printf("cents : %d --> quarters %d  dimes %d  nickels %d  pennies %d\n",
         cents, q, d, n, p);
  }

int main(int argc, char **argv)
  {
  char str[1000];
  int cents;

  while(1)
    {
    fgets(str, 1000, stdin);

    cents = atoi(str);
    change(cents);
    }
  }

It's possible to eliminate all the temp variable declarations and just compute the values as we go:

void change(int cents)
  {
  printf("cents : %d --> quarters %d  dimes %d  nickels %d  pennies %d\n",
         cents,
         (cents / 25),
         (cents - ((cents / 25) * 25)) / 10,
         ((cents - ((cents / 25) * 25) - ((((cents - ((cents / 25) * 25)) / 10) * 10))) / 5),
         cents - (((cents / 25) * 25) +
                  (((cents - ((cents / 25) * 25)) / 10) * 10) +
                  (((cents - ((cents / 25) * 25) - ((((cents - ((cents / 25) * 25)) / 10) * 10))) / 5) * 5)));
  }

but this way lies madness.  :-)

Share and enjoy.


Generic class to generate combinations As junior java programmer I am working with Euler project and I would like to create a general purpose class which generates all the combinations from the length and number of choosen elements. The class returns a list of boolean arrays with length of length. Each array has choosen true value which highlight the chosen elements the others are false. I have a working solution (I mean it is tested to small numbers) but I wonder is there any opportunity to improve the performance or improve some other aspect. Here it is:

public static List<boolean[]> generate(int length, int choosen){
    List<boolean[]> variationList = new ArrayList<>();
    boolean[] actual = initArray(length, choosen);
    variationList.add(actual.clone());
    while(incrementArray(actual)) {
        variationList.add(actual.clone());
    }
    return variationList;
}

private static boolean[] initArray(int length, int choosen) {
    boolean[] array = new boolean[length];
    for (int index=0; index<choosen; index++) {
        array[index] = true;
    }
    return array;
}

private static boolean incrementArray(boolean[] actual) {
    for(int index = actual.length-1; index > -1 ; index--) {
        if(actual[index]) {
            if(moveForward(actual, index)) {
                return true;
            }
        }
    }
    return false;
}

private static boolean moveForward(boolean[] actual, int position) {
    if(position<actual.length-1 && !actual[position+1]) {
        actual[position] = false;
        actual[position+1] = true;
        copyBack(actual, position);
        return true;
    }else {
        return false;
    }
}

private static boolean copyBack(boolean[] actual, int position) {
    boolean answer = false;
    int delay = 2;
    for (int index = position+2; index<actual.length;index++) {
        if(actual[index]) {
            actual[index] = false;
            actual[position + delay] = true;
            delay++;
            answer = true;
        }
    }
    return answer;
}

Edit

Euler project was only the motive I would like to find a way independently. My aim was produce all of the combinations (by the way my solution for the original problem needed them all (not just the number of them) but there is a workaround which no need combination at all).
Some example of the method:

CombinationGenerator.generate(5, 1);

The result:

true false false false false
false true false false false
false false true false false
false false false true false
false false false false true

Invocation:

CombinationGenerator.generate(5, 3)

The result:

true true true false false
true true false true false
true true false false true
true false true true false
true false true false true
true false false true true
false true true true false
false true true false true
false true false true true
false false true true true

BitSet

A BitSet is deliberately designed to be a memory efficient way to hold Boolean values.

It also has initialize methods (clear and set) already, so no need for initArray.  In combination with nextSetBit and nextClearBit (or the previous variants), those methods can replace moveForward.  And copyBack isn't necessary without moveForward.

The single increment for loop in incrementArray would not be necessary with a BitSet.  Again, the nextSetBit and previousSetBit methods would allow you to jump through multiple steps at once.

I haven't compared performance, but generally built-in methods are more efficient than custom methods.  The counter-argument here might be that a BitSet is more space efficient than time efficient.  Or that using the built-in methods involves algorithmic tradeoffs.  You'd have to test to see.

Nitpick

Choosen is not a word.  Although the base word is choose, the past participle is chosen with a single o.  But the variable doesn't really hold what has been chosen.  It holds a count of how many choices should be set to true.  The BitSet definition suggests that the best name for this is cardinality.  At least that's the name of the BitSet method.

BitSet also uses size to represent the total number of choices.  And uses length for something else.  Consider switching the parameter name length to size to match.  Not because length is a bad name, just inconsistent with BitSet.

Try solving recursively where in each call you have to cases: appending 1 to an array and 0 (note that you do both each time). When you append 1, then you need to count if you have reached the chosen count or length, if not then call recursively on that array. Do the same for appending 0 except for you don't need to count if you exceeded choose limit.

Code is something like this (I use c++ but it is easily translatable to Java):

void gen_recursively(vector< vector<bool> > &combinations, vector<int> &current_combination, int len, int chosen) {
    if(current_combination.size() != len) {
        vector<int> current_with_0 = current_combination;
        current_with_0.push_back(0);
        gen_recursively(combinations, current_combination_with_0, len, chosen);

        vector<int> current_with_1 = current_combination;
        current_with_0.push_back(1);
        if(count_chosen(current_with_1)) {
            combinations.push_back(padd_with_zeros_at_end(current_with_1, len));
        } else {
            gen_recursively(combinations, current_combination_with_1, len, chosen);
        }
    } else {
        combinations.push_back(current_combination);
    }
}


Writing many cells to sheet causes Excel to not respond, replacing vlookups I have found that my code causes my Excel to 'not respond' when it gets to this for loop, For thisRow = 2 To lastWSrow(outWS). I'm guessing this is because I am writing cell by cell and the data is too much for excel to handle.

My code essentially does 55 of vlookups on sheet2. It would be similar to doing vlookup(c2&y2,Sheet1!I:J,2,false) but manually it takes long to do.

Could anyone look at this loop and help me optimize this area? I have left only the part needed adjustment. I have put comments on each line to provide description.

outws = sheet2

For thisRow = 2 To lastWSrow(outWS) '2 to last row of sheet2
    For thisCol = 1 To UBound(mappings, 1)

        'create unique key
        thisScen = outWS.Cells(thisRow, posIDcol).Value & "|" & mappings(thisCol, 1)

        'search
        thisDataRow = findInArrCol(thisScen, 1, scenData)

        'write to sheet2
        If thisDataRow = 0 Then
            If outWS.Cells(thisRow, posUnitsCol).Value <> 0 Then 'missing scenario
                outWS.Cells(thisRow, mappings(thisCol, 3)).Value = "No data"
                outWS.Cells(thisRow, mappings(thisCol, 3)).Value = 0

            End If
        Else
            If mappings(thisCol, 1) = "irpv01|PV01_Swap_1M" Then
                outWS.Cells(thisRow, PV01_1MCol).Value = outWS.Cells(thisRow, PV01_1MCol).Value + scenData(thisDataRow, 2) * scenData(thisDataRow, 3) 'since map PV01_0D to PV01_1m already, sum PV01_1M to that column in database.
            Else
                outWS.Cells(thisRow, mappings(thisCol, 3)).Value = scenData(thisDataRow, 2) * scenData(thisDataRow, 3) 'need to scale by position units to get correct risk
            End If
        End If

    Next thisCol
Next thisRow

findInArrCol

Public Function findInArrCol(matchVal As Variant, matchCol As Long, sortedArr() As Variant, Optional nearest As String) As Long

    Dim low As Long, mid As Long, high As Long

    findInArrCol = 0
    low = 1
    high = UBound(sortedArr, 1)
    Do While low <= high
        mid = (low + high) / 2
        If sortedArr(mid, matchCol) = matchVal Then
            findInArrCol = mid
            Exit Function
        ElseIf sortedArr(mid, matchCol) < matchVal Then
            low = mid + 1
        Else
            high = mid - 1
        End If
    Loop

    If findInArrCol = 0 Then
        If nearest = "lessThan" Then
            findInArrCol = WorksheetFunction.Max(high, 1)
        End If
        If nearest = "greaterThan" Then
            findInArrCol = WorksheetFunction.Min(low, UBound(sortedArr, 1))
        End If
    End If

End Function

As was explained in comments elsewhere, the Excel VBA thread is sending messages to the WM message queue to process information, but the same thread is running the for/next loop(s) and will not begin as you had the code until the loops are complete.  By inserting a DoEvents, it causes the VBA code to perform slower because it then allocates some time to the O/S to take care of other pending WM messages and returns back to the VBA code.  Too many DoEvents will greatly slow down your code processing and can even be harmful in some cases where another process can actually take over (hog) more processor time.  The solution is to use DoEvents sparingly to allow Excel to process the rows a few at a time.  What the ideal number is varies by situation, but you can experiment with various batch sizes determined by using modulo arithmetic.

Modify your loop in this way:

        Next thisCol
        If (thisRow Mod 100) = 0 Then
            DoEvents
        End If
    Next thisRow

You can tweak this number by replacing 100 with 10, 50, 250, 500, etc. to see what provides the most desirable result.

Application.Calculation = xlCalculationManual
Application.ScreenUpdating = False

'   Place actual code between the above and below

Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True

The above will generally fix what you are experiencing.

The first part turns off the auto calculation that happens after any change to the sheet.  It also turns off screen updating, so it isn't showing you the changes that are taking place.

Last part turns both back on, so the document behaves as normal.


Responsive CSS, abbreviated content I wonder if there is a "correct" way to display different content, based on the the state of responsive webdesign. Different in this case is meant as an abbreviated text to display instead of the full one.

I have created this example table with a couple columns, containing either the full name of a state, or, when resized to a smaller screensize, it's 2-letter code.

HTML:

<!doctype html>
<meta name="viewport" content="width=device-width"/>

    CaliforniaCA
    FloridaFL
    TexasTX
    GeorgiaGA

    Sacramento
    Tallahassee
    Austin
    Atlanta

CSS:

th {
    border: 1px solid #000;
}

td {
    border: 1px solid #000;
}

@media screen and (max-width:760px){
.full {
    display: inline;
    }
.abbr {
    display: none;
}
}

@media screen and (max-width:320px){
.full {
    display: none;
    }
.abbr {
    display: inline;
}
}

jsFiddle

It works as I want, but this solution kind of seems more like a hack to me to get it working.

I've googled, but haven't found anything regarding the topic, which might just be the result of not knowing what exactly to ask. Anyhow, I somehow got the feeling, that there's a more sophisticated way of doing such a thing.

To expand on Hubert Grzeskowiak's answer the custom data attribute is a good option for you as you can continue specifying content in the markup as it should.

But there is a way with CSS to display it with the content property. Used on a pseudo element, you can choose to show it or not depending on viewport width with media queries.

This will allow you to :

minimize markup
change the content according to the viewport width
no use of JS
keep the content in the markup

DEMO

th {
    border: 1px solid #000;
    position:relative;
}
td {
    border: 1px solid #000;
}
.full:after{
    content: attr(data-state);
    display:none;
}
@media screen and (max-width:320px) {
    .full:after{
        position:absolute;
        top:0;left:0;
        width:100%; height:100%;
        background:#fff;
        display:block;
}

        California
        Florida
        Texas
        Georgia

        Sacramento
        Tallahassee
        Austin
        Atlanta

I don't see anything wrong with your approach. Using pure CSS is fine. A better question might be "Why abbreviate the state when the city names are not abbreviated?" What benefit do you gain from this abbreviation when the width of the table is constricted to the widest city name?

I know that question was perhaps too focused on the code you posted, because your solution is a good general solution for showing abbreviated text based on screen size, but the question is nonetheless applicable to your specific code.


Using Fibonacci sequence to generate musical melodies I recently started picking up the Haskell programming language. I've managed to learn other languages rather quickly, but I'm still having a difficult time grasping some of the basics of Haskell.

I decided to practice and wrote a program that calculates the Fibonacci Sequence (original, I know) a certain amount of times and adds the results to a list, then divides each number in the list by 7 and creates a new list of the remainders of that division. It then assigns a musical note letter based in A minor (A, B, C, D, E, F, and G), and prints out each letter. The idea was to give me new ideas for writing melodies.

The program works, but with how efficient Haskell is I feel like it's way too long for what it does. How would you guys suggest re-writing this and what are some shortcuts I can use to write better code?

Here's the program:

import Data.List
import System.IO

fibo a b = a:fibo b (a+b)
divBy7 x = x `mod` 7
calcNums = take 16 $ map divBy7(fibo 0 1)
returnNote :: Int -> String
returnNote n
  | (n == 0) = "A"
  | (n == 1) = "B"
  | (n == 2) = "C"
  | (n == 3) = "D"
  | (n == 4) = "E"
  | (n == 5) = "F"
  | (n == 6) = "G"
-- the number will never be higher than 6 due to the pisano period

output = map returnNote(calcNums)

I'm just running ghci and asking it to return output

fibo

fibo is a fine function, but I would suggest a more complete name like fibonacci just to make it 100% clear that it returns fibonacci numbers to someone that did not read your plaintext introduction.

divBy7

divBy7 is not a good function: it is trivial and most importantly does not do what it says. It returns the remainder of dividing by 7, not the result of dividing by 7. I would delete this function.

calcNums

calcNums is not general enough, I feel no need for the take 16 at the start of it, this function has the potential to calculate as many fibonacci mod 7 as you want, let the caller decide how many to take.

Incidentally fibonacciMod7 is a better name for the function as it says what it does: calcNums was way too general.

returnNote

returnNote is too verbose and fails badly (= with no meaningful error message) if I pass it anything bigger than 6.

returnNote n
  | n `elem` [0..6] = "ABCDEFG" !! n
  | otherwise = error "n will never be higher than 6 due to the pisano period. (And it must be positive)"

This implementation is more concise and incorporates the comment into the code so that it can be shown to the user when he uses the function incorrectly.

you could shortcut it to sumthin like

ghc -e 'take(16)$
  map(("ABCDEFG"!!).(`mod`7))$
  fix((0:).scanl(+)1)'

likewise

fibos a b = a : fibos b (b + a)
music = fibos 0 1
note = ("ABCDEFG"!!).(`mod`7)
notes = map note music
melody = take 16 notes


Responsive module and tooltip module Recently, I've noticed that my code looks so ugly because there were a bunch of functions and global variables.  So, I started reading a little bit about design patterns. Now I come with something that's working for me, but I am not sure if it's good practice.

Anyway, I would like you to take a look at the code and tell me what can I improve.  I'd also like to know a better way to start using modular pattern in JavaScript.  Finally, I'd like suggestions on material for learning about modular pattern and JavaScript.

var responsiveModule = {

    settings: {
        device: false,
        button: $(".responsive-btn"),
        target: $("nav ul"),
        mobileClass: "toggle-menu",
        bgImage: '<img class="background-image" src="img/background.jpg" alt="">',
        bgImageSelector: $(".background-image"),
        windowWidth: $(window).width(),

    },

    init: function(){
        responsiveModule.checkDevice();
        responsiveModule.replaceImages();
        responsiveModule.bindFunctions();
        responsiveModule.listenResize();
    },

    checkDevice: function(){
        if(this.settings.windowWidth > 992){
            this.settings.device = "desktop";

        } else {
            this.settings.device = "mobile";
        }
    },

    bindFunctions: function(){
        var buton = this.settings.button,
            muelleBtn = this.settings.muelleBtn;
        buton.on("click touchstart", function(e){
            e.preventDefault();
            responsiveModule.animateMenu(responsiveModule.settings);
        });
    },

    animateMenu: function(settings){
        var device = settings.device,
            target = settings.target,
            mobileAnimation = settings. mobileClass;

        if(device == "mobile"){
            target.toggleClass(mobileAnimation);
        }
    },

    replaceImages: function(){
        var bgContainer = $("#main-content"),
            bgImage = responsiveModule.settings.bgImage,
            device = responsiveModule.settings.device,
            backgroundSelector = $(".background-image");

        if(device == "desktop" && backgroundSelector.length == 0){
            bgContainer.append(bgImage);
        }else if(device == "mobile" && backgroundSelector.length == 1){
            backgroundSelector.remove();
        }
    },

    listenResize: function(){
        $(window).on("resize", function(){
            responsiveModule.checkDevice();
            responsiveModule.replaceImages();
            responsiveModule.settings.windowWidth = $(window).width();
        });
    }

}

var tooltipModule = {

    settings: {
        tooltipState: false
    },

    init: function(){
        tooltipModule.bindUIfunctions();
    },

    bindUIfunctions: function(){
        var device = responsiveModule.settings.device;
        if(device == "mobile"){
            $(".ship").on("click", function(e){
                e.preventDefault();
                tooltipModule.manageTooltip(e);
            });
        }else{
            $(".muelle-item").addClass("desktop");
        }
    },

    manageTooltip: function(e){
        var tooltip = $(e.currentTarget).next(),
            tooltips = $(".tooltip");

        tooltips.removeClass("display");
        tooltip.addClass("display");
    }

}

$(document).on("ready", function(){
    responsiveModule.init();
    tooltipModule.init();
});

The best pattern to start "OOP" in JavaScript is the Revealing Pattern:

var revealed = function(){
   var a = [1,2,3];
   var abc = function(){
     return (a[0]*a[1])+a[2];
   }

   return {
      name: 'revealed',
      abcfn: abc
   }
}();

From here.

Then read this book.

And learn RequireJS, which will increase the modularity of your code and will load your JS files on demand.

I see you are using JS for create HTML elements.  To help you with this, take a look at UnderscoreJS templates and Mustache.

Don't, just don't.

JavaScript is not an OO language, it is a prototype language, something a lot of people tend to forget.

There are no 'classes' or 'interfaces' in javascript. Everything is an object. Even a function is an object.

So how do JavaScript Objects work? what is 'this'? and what does 'new' do?
A good place to start is understanding Scope and context.

So, what are JavaScript objects?  Again there is a good article.

Once you understand that and you still feel like writing OO Js. There are tons of frameworks out there that pretend to be OO, my personal favorite MooTools.  These frameworks give you an interface to write OO-like code. But then again, in big applications that code tends to get slow, leak memory, since we forget how prototypes really work and what the 'new' operator means.

Let's recap and go back to your question. Your real problem is that your code is becoming hard to read.

So you are looking for a certain pattern in JavaScript to help in maintaining your code. My favorite pattern is the Module Pattern, but there are many more very strong patterns for JavaScript.