Swap even/odd characters I wrote some code to solve this problem https://www.hackerrank.com/challenges/string-o-permute which basically states: take an even length string and swap all the even indexed characters. For example the string "abcd" should become "badc". I'm looking for a general code review as I'm very new to F# and specifically I'd really like to get rid of the ignoredTupleValue variable, it seems like a kludge to me. I'd also like to collapse the whole thing into one function but I was running into scoping issues with the StringBuilder.

open System
open System.Text

[<EntryPoint>]
let main argv =
    let numCases = Console.ReadLine() |> int
    for i = numCases downto 1 do
        let strIn = Console.ReadLine()
        let pairs = Seq.pairwise strIn
        let ignoredTupleValue = ('1', '1')
        let swappedTuples = pairs |> Seq.mapi (fun idx pair ->
                                if idx % 2 = 0 then
                                    let (a, b) = pair
                                    (b, a)
                                else
                                    ignoredTupleValue)
        let answer = Seq.fold (fun (sb : StringBuilder) pair ->
                                if pair <> ignoredTupleValue then
                                    let (a, b) = pair
                                    sb.Append(a) |> ignore
                                    sb.Append(b) |> ignore
                                sb)
                                (new StringBuilder())
                                swappedTuples
        printfn "%s" (answer.ToString())
    0

The solution should look like (pseudocode, I know little F# too ;) ):

String
  |> ChunksOf 2
  |> Map Reverse
  |> Flatten

An example explains clearly

"abcd" -> ["ab", "cd"] -> ["ba", "dc"] -> "badc"

The key of effective Functional Programming is making good use of the built-in functions and carefully combining them.


Making at least one checkbox required This code controls whether the user has checked at least one checkbox field before saving the record. If the user has failed to do so the code throws an error message and cancels the operation.

Is there a better/more efficient way to write it?

function BusComp_PreWriteRecord ()
{
    try
    {
        var sSMS =          this.GetFieldValue("R SMS");
        var sEmail =        this.GetFieldValue("R Email");
        var sChannel1 =     this.GetFieldValue("R Channel 1");
        var sChannel2 =     this.GetFieldValue("R Channel 2");
        var sChannel3 =     this.GetFieldValue("R Channel 3");

        //R 17.12.14 ES: Added to make channel choice fields required if none are filled in
        if ((sSMS != "Y") && (sEmail != "Y") && (sChannel1 != "Y") && (sChannel2 != "Y") && (sChannel3 != "Y"))
        {
            TheApplication().RaiseErrorText("Please check at least one box");
        }
    }
    catch(e)
    {
        throw e;
    }
}

I'd probably verify the correct amount of checkboxes by iterating over them when verifying. This can get tricky in case your layout is complex, but you should usually be able to find a solution to this.

Since I liked the initial idea (I always try to avoid doing such checks or just do them server side), here's a fiddle I came up with. Just keep in mind that this code is barely optimized, complete or clean. It's just some quick example put together.

This works pretty straightforward and without any source code modifications in case anyone adds more checkboxes. You could even just modify the plain HTML template and everything would work  again.

Here's the magic:

All check box groups are grouped inside some HTML tag/container. In my case it's a <div>. These containers are marked with the class checkgroup.

<div class="checkgroup">
    <!-- Other content and checkboxes -->
</div>

Once verifying, I first determine all such containers in the current document.

var groups = document.getElementsByClassName('checkgroup');

Then I try to read custom attributes to get custom limits as well as a possible error message (stored in minimum, maximum, and message).

// Get the limits
if (group.hasAttribute('minchecked'))
    minimum = parseInt(group.getAttribute('minchecked'));
if (group.hasAttribute('maxchecked'))
    maximum = parseInt(group.getAttribute('maxchecked'));
// Get the actual error message
if (group.hasAttribute('errchecked'))
    message = group.getAttribute('errchecked');

I then iterate over all checkboxes included in the current group and count the number of checked instances.

var checkboxes = group.getElementsByTagName('input');
for (var j = 0; j < checkboxes.length; ++j) {
    // Here I should check whether that's indeed a checkbox...
    if (checkboxes[j].checked)
        ++ticked;
}

Once this is done, there's a simple comparison on the limits and an error message is shown just in case.

// Show an error message and highlight the background
if (ticked < minimum || ticked > maximum) {
    group.style.backgroundColor = '#fcc';
    alert(message);
    group.style.backgroundColor = '';
}


Searching for unused functions So I came up with a problem In C. Our compiler is pre c99 and wasn't identifying functions that are never called. So I ended up writing this script and decided to post it up to be critiqued and to help people who might be in the same predicament. The script works great for me and finished the job that I needed it to do. I just felt that it could be done better.

from sys import exit
import re
import os ,fnmatch
#"(static BOOL|BOOL|static void|void)\s+([A-Z a-z]+)(\(.*?\)\;)"
pattern = "(static BOOL|BOOL|static void|void|static BYTE|BYTE|static int|int)\s+([A-Z a-z]+)(\(.*?\)\;)"

compiled = re.compile(pattern)
commentend = re.compile("\*/")
commentbegin = re.compile("/\*.*") #|\}/s/\*.* extra pattern
count = 0
directory =" "

def menu():#menu function
    global directory
    print "what would you like to do"
    print "1. Enter directory of .c and .h files you want searched"
    print "2. Display possibly unused functions" #not used
    print "Any other key exit program"

    selection = raw_input("> ")
    if selection == '1':#this menu starts the search process
        print "you selected search"
        print "what is the file name"
        directory = raw_input("> ")
        openfiles()
    elif selection == '2':#this selection is not currently used
        print "displaying matching functions"
        print count
    else:
        exit(0)

def openfiles(): #gets a list of file names and then passes the name to search for function to open
    for root,dirs,files in os.walk(directory):
        for pat in ['*.c','*.h']:
            for current_file in fnmatch.filter(files,pat):
                searchforfunction(current_file)

def searchforfunction(file_name):
    global directory
    print file_name
    with open(os.path.join(directory,file_name) ,"r" ,1) as f:
        for line in f:
            matchfun(line)

def matchfun(line): #This searches for a function looking for specific definition then checks how many times the function occurs
    global count
    name_func = compiled.match(line)
    if name_func:
        name_func = name_func.group(2)
        count = searchfilesecond(name_func)
        if count <= 2:
            print name_func," was called",count,"times"
        if count == 2:
            writefunc_twice(name_func)
        if count == 1:
            writefunc_one(name_func)
    count = 0

def writefunc_one(name_func):
    with open("occur_once_func.txt", "a") as myfile:
        myfile.write(name_func+" was called 1 time\n")

def writefunc_twice(name_func):
    with open("occur_twice_func.txt", "a") as myfile:
        myfile.write(name_func + " was called 2 times\n")

def searchfilesecond(name_func): #walks the directory again and searches for a match by passing it so secondsearch
    global count
    global directory
    for root,dirs,files in os.walk(directory):
        for pat in ['*.c','*.h']:
            for current_file in fnmatch.filter(files,pat):
                count = secondsearch(name_func,current_file)

    return count

def secondsearch(name_func,current_file): #this function searches for any instances of the function
    global count
    global directory
    in_comment = False
    secondpatern = re.compile(name_func)
    with open(os.path.join(directory,current_file) ,"r" ,1) as f:

        for line in f:
            if commentbegin.search(line):#This statement searches for the /* part of a comment
                in_comment = True
            if commentend.search(line):#This statement searches for the */ part of a comment
                in_comment = False
            elif in_comment == False:
                name_func1 = secondpatern.search(line)
                if name_func1:
                    count += 1
            else:
                pass
    return count

while(1):
    menu()

Two major points:

The parsing is very fragile and only works when the code is laid out in a certain way. You should look for a proper parser and leverage that.
It is rather inefficient to scan all the files every time you encounter a function declaration. You could use dictionaries to keep track of functions so one pass over the files would suffice.


Script to create a customized, efficient adblocking hosts file I am writing a Python script that creates a customized, efficient adblocking hosts file.  Basically it checks the user's DNS cache and sees if any entries there are listed in the popular hosts files.  If so, it adds those entries to the user's hosts file.  The purpose is to keep the hosts file small and avoid adverse effects on browsing speed.  I was looking for any input on my overall approach, any ways to make it faster, and any stylistic revisions/suggestions.

import os
import urllib.request
import subprocess
import re
import datetime

# Create a list of ad domains from hosts files found online
def get_ad_dmns(src_urls):
    dmns = set()
    for src in src_urls:
        entries = [line.decode('utf-8') for line in list(urllib.request.urlopen(src))]
        for entry in entries:
            # If hosts file entry is a valid block rule, add domain to list
            if entry.startswith(('0.0.0.0', '127.0.0.1')):
                dmns.add(entry.split()[1])
    return dmns

# Create a list of domains found in the user's DNS cache
def get_dns_dmns():
    dns_cache = subprocess.check_output('ipconfig /displaydns').decode('utf-8')
    # Regex pattern to match domains in the DNS cache
    pattern = '\n\s+(\S+)\r\n\s+-'
    dmns = re.findall(pattern, dns_cache)
    return dmns

# Create a list of domains currently in the user's hosts file
def get_cur_dmns(hosts_dir):
    os.chdir(hosts_dir)
    dmns = set()
    hosts_file = open('hosts', 'r')
    for entry in hosts_file:
        if entry.startswith(('0.0.0.0', '127.0.0.1')):
            dmns.add(entry.split()[1])
    hosts_file.close()
    return dmns

# Write new domains to the hosts file
def write_hosts_file(dmns, hosts_dir):
    os.chdir(hosts_dir)
    hosts_file = open('hosts', 'a')
    hosts_file.write('\n# Updated: {}\n'.format(datetime.datetime.now()))
    for dmn in dmns:
        hosts_file.write('0.0.0.0 {}\n'.format(dmn))
    hosts_file.close()

def main():
    hosts_dir = 'C:/Windows/System32/drivers/etc'
    srcs = ['http://winhelp2002.mvps.org/hosts.txt',
            'http://someonewhocares.org/hosts/zero/hosts']
    ad_dmns = get_ad_dmns(srcs)
    dns_dmns = get_dns_dmns()
    cur_dmns = get_cur_dmns(hosts_dir)
    dmns_to_add = [dmn for dmn in dns_dmns if dmn in ad_dmns and dmn not in cur_dmns]
    if dmns_to_add:
        print ('Adding {} domain(s)...'.format(len(dmns_to_add)))
        write_hosts_file(dmns_to_add, hosts_dir)

if __name__ == '__main__':
    main()

Overall, this is pretty good, but all code can be better! A few suggestions:

There aren’t any docstrings telling me what a function does, or what it returns. With a small codebase like this, it’s fairly easy for me to just read the code, but it’s a good habit to get into.
I’m nitpicking, but module imports should really be alphabetically ordered. It’s not so much of a problem for short scripts like this, but it’s really useful in large codebases.
In the get_ad_dmns() function, you create the entries list by iterating over urllib.request.urlopen(src), then immediately iterate over the list. Could you skip creating the list? i.e.

for line in list(urllib.request.urlopen(src)):
    entry = line.decode('utf-8')
    # rest of the code here

I think that might be a little simpler and more memory efficient.
Rather than using dmns as an abbreviation for domains, just use the full name. Characters are cheap, and it makes your code easier to read.
In get_cur_dmns(), rather than using open(myfile) ... close(myfile), the more idiomatic construction is:

with open('hosts'() as hosts_file:
    for entry in hosts_file:
        # do stuff with the entry

Ditto for write_hosts_file().
In your main() function, the dmns_to_add list comprehension is just a little hard to read. I’d suggest adding an extra line break for the not in cur_dmns line to make it easier to read.


Calendar Print Method I'm currently learning Ruby and need help in editing code to be more idiomatic. I have the method below I could use some pointers on how to make more "Ruby" :-)

def print_cal(day_of_week, month_len)
  days = %w(Mon Tue Wed Thu Fri Sat Sun)
  days.each { |day| print "#{day} " }

  puts

  day_num = 1
  month_started = false
  while day_num <= month_len
    (1..7).each do |i|
      if  day_of_week > i && month_started == false
        print '    '
      else
        month_started = true
        print ' ' if day_num < 10
        print " #{day_num} " if day_num <= month_len
        day_num += 1
      end
    end
    puts
  end
end

Here's my take, with explanations below:

def print_calendar(offset, month_length)
  puts "Mon Tue Wed Thu Fri Sat Sun"
  dates = [nil] * offset + (1..month_length).to_a
  dates.each_slice(7) do |week|
    puts week.map { |date| date.to_s.rjust(3) }.join(' ')
  end
end

I'd call the first parameter offset rather than day_of_week. For one, day_of_week would seem to imply that you can use a Date object's wday as the offset, but wday's range is 0-6, with zero being Sunday, so that won't work. Also, with day_of_week I wonder if you mean a day's name, or its number? Oh, and which week are we talking about? The name offset is a lot more generic, but that might be a good thing here.
I've also spelled out print_calendar and month_length entirely. While those names weren't too confusing, there's no reason to skip a few letters.
Also note that the offset behaves slightly different from yours: In your method, the minimum offset is 1. That makes sense if you think "start on the 1st day", but it makes less sense if you think "offset everything by 1". So here, the minimum offset is zero.
Array#join is a nice method to know when you have to print lists of stuff (in any language, not just Ruby).  You could have done puts %w(Mon Tue Wed Thu Fri Sat Sun).join(' ') instead of your first 3 lines. But the end result would be no different than simply printing a hard-coded string, so that's what we're doing here instead.
In Ruby, you rarely have to use while loops. Generally, it's easier to construct and manipulate arrays using the built-in methods in Array and those mixed-in by the Enumerable module. (Seriously, if you're learning Ruby, go read as much as you can from those two links; it's some the most useful stuff in Ruby, and you'll be using it a lot.)
Here, we're constructing a dates array of nils that's offset items long by using Ruby's nifty ability to multiply an array by a number. Then we use + to concatenate that array with an array that's just the numbers 1..month_length.
In other words, dates consists of zero or more "blanks" (the offset) followed by the date numbers of the month.
each_slice should be fairly self-explanatory: Go through an array, X items at a time. In this case we go through it 7 items at a time (i.e. a week), which is what we need for each row/line of text.
Each of these weeks are just arrays of nils and/or numbers, so we convert each of them, using map, to an array of strings. Each blank/number is first converted to a string, and then padded with spaces using rjust. Finally, this mapped array is joined and output.

And that's it, really.

Calling print_calendar(2, 31) will print the following:

Mon Tue Wed Thu Fri Sat Sun
          1   2   3   4   5
  6   7   8   9  10  11  12
 13  14  15  16  17  18  19
 20  21  22  23  24  25  26
 27  28  29  30  31

However, it'd be even more idiomatic to let the method simply return the text without printing it. That could be done like so:

def format_calendar(offset, month_length)
  lines = [ "Mon Tue Wed Thu Fri Sat Sun" ]
  dates = [nil] * offset + (1..month_length).to_a
  dates.each_slice(7) do |week|
    lines << week.map { |date| date.to_s.rjust(3) }.join(' ')
  end
  lines.join("\n")
end

After which you can do something like

puts format_calendar(2, 31)

to print it.

Edit: As Naklion's answer rightly points out, it'd be even nicer to return an array of strings (the individual lines of text). In this case, you can do so by simply omitting the .join("\n") on the last line of the method above. That'll give you more structured data to work with, instead of a text blob.
As for printing, you don't need to change anything, since puts automatically adds linebreaks when printing an array.


A text-templating system similar to PHP...but with Clojure I wrote a simple text-templating/processing system which resembles PHP, except that it works with Clojure. The main goal was to provide an easy way to use Clojure PHP-style for small files.

This is a usage example, the following file

<?clj
        (def theName "Soonog")

        (defn replaceAs
                [input]
                (clojure.string/replace input "a" "X")
        )
?>

Hello <% theName %>, this is a simple template which should allow
you to <% (replaceAs "see the basics how this might work some day") %>.

should output (here are two newlines at the start missing)

Hello Soonog, this is a simple template which should allow
you to see the bXsics how this might work some dXy.

A short info on how it works:

Accepts files as arguments

If no arguments are provided or the argument is "-" it will read from stdin

Outputs to stdout
Uses regular expressions to split/process the text
Everything in <?clj ... ?> is evaluated with load-string
Everything in <% ... %> is wrapped in (echo ... ) and then treated like above

A few things I'm unsure about:

The treatment of static variables (f.e. tag-start)
The style itself
Using regular expressions to parse files
load-strings will load it into the current context, so all files passed in share the same context...this is by design (for now) so that you can "include" files. Though, having a function that uses a separate context might be interesting if this is used as library.

And here's the code:

(ns closeddoor.core
    (:gen-class :main true))

(require 'clojure.string)

(def tag-start
    "The long/normal start tag."
    "<\\?clj")
(def tag-end
    "The long/normal end tag."
    "(\\?>|\\Z)")

(def echo-start
    "The short/echo start tag."
    "<%")
(def echo-end
    "The short/echo end tag."
    "%>")

(def tag-pattern
    "The compiled pattern for the long/normal tags."
    (re-pattern (str tag-start "((?s:.+?))" tag-end)))
(def echo-pattern
    "The compiled pattern for the short/echo tags."
    (re-pattern (str echo-start "((?s:.+?))" echo-end)))

(defn- process-match
    "Processes the given match, and returns the output of
    the given match."
    [[match group]]

    ; Create a StringBuilder which we'll use for our content.
    (def buffer (StringBuilder.))

    (defn echo
        "A small helper function to allow to echo things."
        [string]
        (.append buffer string))

    ; Now load what the regex delivered to use.
    (load-string (str "(use 'closeddoor.core)" group))

    ; Return the buffer.
    ; We're replacing the $ with an escaped version because this output
    ; is directly passed into string/replace.
    (clojure.string/replace (.toString buffer) "$" "\\$"))

(defn- process-match-echo-wrapped
    "Process the given match, but wraps it first in the echo function."
    [[match group]]
    (process-match [match (str "(echo " group ")")]))

(defn parse
    "Parses the given input, processes the matches and returns the result.
    The long/normal tags are processed first, after that the short/echo tags.
    Order of appearance does not matter, all normal tags are processed first."
    [input]
    (clojure.string/replace
        (clojure.string/replace
            input
            tag-pattern
            process-match)
        echo-pattern
        process-match-echo-wrapped))

(defn process
    "Processes the given source, which means that it reads everything from
    the source with slurp, runs it through parse and spits it out into out.

    So (process input *out*) is basically shorthand for (spit *out* (parse input))."
    [source out]
    (spit out (parse (slurp source))))

(defn -main
    "The main function which does everything."
    [& args]
    (if (empty? args)
        (process *in* *out*)
        (doseq [arg args]
            (if (= arg "-")
                (process *in* *out*)
                (process arg *out*)))))

Cool idea! I could definitely see this coming in handy in a variety of domains.

I noticed that you have some def and defn statements within the function definition of process-match. It's generally considered un-idiomatic to use def/defn anywhere except for at the top level of the namespace you're in. Usually you would use let instead if what you're trying to define isn't used outside of the function, but it doesn't look like that's the case here -- I think you are wanting buffer and echo
to be used at the top level, so I would pull them out of the process-match function and do something like this:

(ns closeddoor.core
  (:require clojure.string)
  (:gen-class :main true))

(def tag-start "<\\?clj")  ; The long/normal start tag.
(def tag-end "(\\?>|\\Z)") ; The long/normal end tag.

(def echo-start "<%") ; The short/echo start tag.
(def echo-end "%>")   ; The short/echo end tag.

; the compiled regex patterns for each of the above
(def tag-pattern (re-pattern (str tag-start "((?s:.+?))" tag-end)))
(def echo-pattern (re-pattern (str echo-start "((?s:.+?))" echo-end)))

(def buffer (StringBuilder.))

(defn- echo [string]
  "A small helper function for echoing things."
  (.append buffer string))

(defn- process-match [[match group]]
  "Processes the given match, and returns the output of the given match."
  (load-string (str "(use 'closeddoor.core)" group))
  (clojure.string/replace (.toString buffer) "$" "\\$"))

(defn- process-match-echo-wrapped [[match group]]
  "Process the given match, but wraps it first in the echo function."
  (process-match [match (str "(echo " group ")")]))

(defn parse [input]
  "Parses the given input, processes the matches and returns the result.
   The long/normal tags are processed first, after that the short/echo tags.
   Order of appearance does not matter, all normal tags are processed first."
  (clojure.string/replace
    (clojure.string/replace input tag-pattern process-match)
    echo-pattern
    process-match-echo-wrapped))

(defn process [source out]
  "Processes the given source, which means that it reads everything from
   the source with slurp, runs it through parse and spits it out into out.
   So (process input *out*) is basically shorthand for (spit *out* (parse input))."
  (spit out (parse (slurp source))))

(defn -main [& args]
  "The main function which does everything."
  (if (empty? args)
    (process *in* *out*)
    (doseq [arg args]
      (if (= arg "-")
        (process *in* *out*)
        (process arg *out*)))))

BTW, I also changed your spacing from 4 spaces to 2 which is the Clojure standard. I guess there's nothing wrong with using 4 spaces, but 2 is predominantly what you see.

I've generally rearranged the spacing and line breaks to suit my own aesthetic preference, but it isn't necessarily "the right way," just my opinion on what formatting I find more readable. Just my 2 cents!

I also moved the (require 'clojure.string) into the namespace definition, which is idiomatic and more concise.

I'm a bit confused about your parse function... did you mean for the last argument of each call to clojure.string/replace function to be a function (process-match and process-match-echo-wrapped, respectively)? I believe you need to have strings there, indicating what to replace each match with. Maybe you meant (process-match match group), where match and group are replaced with the actual match and group? Maybe I'm missing something.

By the way, if you wanted to simplify the nested calls to clojure.string/replace in that function, you could do something like this:

(defn parse [input]
  "Parses the given input, processes the matches and returns the result.
   The long/normal tags are processed first, after that the short/echo tags.
   Order of appearance does not matter, all normal tags are processed first."
  (reduce (fn [s [pat rep]]
            (clojure.string/replace s pat rep))
          input
          [[tag-pattern process-match]
           [echo-pattern pattern-match-echo-wrapped]]))

If I can offer one last criticism: your methods that deal with modifying buffer (which is initially defined as (StringBuilder.) are destructive actions, so you might consider adding ! onto the end of your function names (echo!, process-match!, etc.)

Hope this helps!


Return a paged DataTable Is there a better way to do it and have it be .Net 2.0 compatible?

public static DataTable GetErrorLog(int startRowIndex, int maximumRows, string sortExpression, string logPath)
{
    if (string.IsNullOrEmpty(sortExpression))
    {
        sortExpression = "fileName DESC";
    }

    DataTable errorLog = GetErrorLogDataTable();

    string[] filePaths = Directory.GetFiles(logPath);
    foreach (string filePath in filePaths)
    {
        DataRow row = errorLog.NewRow();
        row["fileName"] = Path.GetFileName(filePath);
        row["filePath"] = filePath;
        errorLog.Rows.Add(row);
    }

    DataView dataView = new DataView(errorLog);
    dataView.Sort = sortExpression;
    errorLog = dataView.ToTable();
    DataTable pagedErrorLog = errorLog.Clone();

    for (int i = startRowIndex; i < startRowIndex + maximumRows; i++)
    {
        if (i >= errorLog.Rows.Count)
        {
            break;
        }
        pagedErrorLog.ImportRow(errorLog.Rows[i]);
    }

    if (pagedErrorLog.Rows.Count <= 0)
    {
        return errorLog;
    }
    else
    {
        return pagedErrorLog;
    }
}

private static DataTable GetErrorLogDataTable()
{
    DataTable dataTable = new DataTable();
    dataTable.Columns.Add("fileName");
    dataTable.Columns.Add("filePath");
    return dataTable;
}

Parameter validation
Because GetErrorLog() is a public method you should assume that any given parameter can be wrong so a at least basic validation should take place.
E.g assume a user of that method is passing a startRowIndex == -1 then the line pagedErrorLog.ImportRow(errorLog.Rows[i]); would throw with an IndexOutOfBoundException.
A much better way would be to validate that parameter at the beginning like so
if (startRowIndex < 0)
{
    throw new ArgumentOutOfRangeException("startRowIndex");
}

I would split that big method into several smaller ones which are having only one responsibility like
private static void FillErrorLogTable(DataTable table, string[] filePaths)
{
    foreach (string filePath in filePaths)
    {
        DataRow row = table.NewRow();
        row["fileName"] = Path.GetFileName(filePath);
        row["filePath"] = filePath;
        table.Rows.Add(row);
    }
}

private static void FillPagedErrorLog(DataTable source, DataTable destination, int lowerBound, int upperBound)
{
    int sourceRowCount = source.Rows.Count;

    for (int i = lowerBound; i < upperBound; i++)
    {
        if (i >= sourceRowCount)
        {
            break;
        }
        destination.ImportRow(source.Rows[i]);
    }
}

How should Rows.Count at this condition if (pagedErrorLog.Rows.Count <= 0) ever be < 0? That won't ever happen.
Summing up the changes with only the mentioned validation which should be extended to validate the remaining parameters as well, would lead to
public static DataTable GetErrorLog(int startRowIndex, int maximumRows, string sortExpression, string logPath)
{
    if (startRowIndex < 0)
    {
        throw new ArgumentOutOfRangeException("startRowIndex");
    }

    if (string.IsNullOrEmpty(sortExpression))
    {
        sortExpression = "fileName DESC";
    }

    DataTable errorLog = GetErrorLogDataTable();

    string[] filePaths = Directory.GetFiles(logPath);

    FillErrorLogTable(errorLog, filePaths);

    DataView dataView = new DataView(errorLog);
    dataView.Sort = sortExpression;
    errorLog = dataView.ToTable();
    DataTable pagedErrorLog = errorLog.Clone();

    FillPagedErrorLog(errorLog, pagedErrorLog, startRowIndex, startRowIndex + maximumRows);

    if (pagedErrorLog.Rows.Count == 0)
    {
        return errorLog;
    }
    else
    {
        return pagedErrorLog;
    }
}

Other than what I had mentioned, your code looks good, you are following the naming conventions and you have named your things well (except of the GetErrorLogDataTable() method which I would name Create..).


One-by-one file deletion for a derived list of files I have the following C# code for deleting files from a folder (and from its sub folders) excluding a list of files specified in an Excel file. The code works fine.

The deletion is happening one-by-one in a for loop. Is there a way to improve its performance by reducing the time taken for deletion?

class Program
{
    public static List<string> AllFilesStaticList = new List<string>();

    static void Main(string[] args)
    {

        string locationOfExcelList = @"G:\Test1\ExclusionList.xls";
        string sheetName = "List$";
        string sourceDirectory = @"G:\TFS2\";

        List<string> excludeList = Import_To_DataTable_FromExcel(locationOfExcelList, sheetName);
        ProcessAllDirectoryToGetFileList(sourceDirectory);

        //Get files to be deleted (Items that are not present in excludeList)
        List<string> eligibleListToProcess = AllFilesStaticList.Where(x => !excludeList.Any(y => x.Contains(y))).ToList();

        //Iterating two lists - need to be improved
        foreach (string fileEntry in eligibleListToProcess)
        {
            try
            {
                if (File.Exists(fileEntry))
                {
                    //foreach delete need to be made more efficient
                    File.Delete(fileEntry);
                    Console.WriteLine(fileEntry);
                }
            }
            catch
            {
                //Hide the exception for now
            }
        }
        Console.ReadLine();
    }

    // Process all files in the directory passed in, recurse on any directories
    // that are found, and process the files they contain.
    public static  void ProcessAllDirectoryToGetFileList(string targetDirectory)
    {
        // Process the list of files found in the directory.
        //Get files with their path
        string[] fileEntries = Directory.GetFiles(targetDirectory);
        foreach (string fileName in fileEntries)
        {
            AddFile(fileName);
        }

        // Recurse into subdirectories of this directory.
        string[] subdirectoryEntries = Directory.GetDirectories(targetDirectory);
        foreach (string subdirectory in subdirectoryEntries)
        {
            ProcessAllDirectoryToGetFileList(subdirectory);
        }
    }

    public static void AddFile(string fileName)
    {
        //Console.WriteLine();
        AllFilesStaticList.Add(fileName);
    }

    private static List<string> Import_To_DataTable_FromExcel(string FilePath,string sheetName)
    {
        List<string> deploymentFiles = new List<string>();

        string conStr = null;
        conStr = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source={0};Extended Properties='Excel 8.0;IMEX=1;HDR=NO;TypeGuessRows=0;ImportMixedTypes=Text'";

        conStr = string.Format(conStr, FilePath);
        OleDbConnection connExcel = new OleDbConnection(conStr);
        OleDbCommand cmdExcel = new OleDbCommand();
        OleDbDataAdapter oda = new OleDbDataAdapter();

        cmdExcel.Connection = connExcel;

        connExcel.Open();
        DataTable dtExcelSchema = default(DataTable);
        dtExcelSchema = connExcel.GetOleDbSchemaTable(OleDbSchemaGuid.Tables, null);

        connExcel.Close();

        //Read Data from First Sheet
        connExcel.Open();
        cmdExcel.CommandText = "SELECT * From [" + sheetName + "]";
        oda.SelectCommand = cmdExcel;

        DataTable deploymentListTable = new DataTable("deploymentListTable");
        oda.Fill(deploymentListTable);
        connExcel.Close();

        foreach (DataRow row in deploymentListTable.Rows )
        {
            //Read third column from the excel sheet
            string cellFileName = row[2].ToString();
            if (!String.IsNullOrEmpty(cellFileName))
            {
                deploymentFiles.Add(cellFileName);
            }
        }

        return deploymentFiles;

    }

}

I may suggest the following : why checking for file deletion ?

if (File.Exists(fileEntry))

After all, in file.delete documentation we have :

  If the file to be deleted does not exist, no exception is thrown.


WPF Palindrome Checker Application I made a WPF Application, called Palindrome Checker, which checks if what you input is a palindrome.

Any and all tips on how to make this code better in all capacities are wanted and appreciated.

public class Check {

    /// <summary>
    /// Method for checking if the word/text is a palindrome.
    /// </summary>
    public static bool IsPalindrome(string text) {

        int min = 0;
        int max = text.Length - 1;

        while (true) {
            if (min > max) {
                return true;
            }

            char a = text[min];
            char b = text[max];

            if (a != b) {
                return false;
            }

            min++;
            max--;
        }
    }
}

public partial class MainWindow : Window {

    public MainWindow() {

        InitializeComponent();

        lblInput.Foreground = Brushes.ForestGreen;
        lblResult.Foreground = Brushes.ForestGreen;
        lblTitel.Foreground = Brushes.ForestGreen;

    }

    /// <summary>
    /// User input and checking the input if the word a palindrome is.
    /// </summary>
    private void InputText_TextChanged(object sender, TextChangedEventArgs e) {

        string text = InputText.Text;

        bool isPalindrome = Check.IsPalindrome(text);

        OutputText.Text = text + (isPalindrome ? " is a palindrome" : " is NOT a palindrome");

        if(InputText.Text == string.Empty)
            OutputText.Clear();
    }
}

Specification

Palindrome

From wikipedia:

  A palindrome is a word, number, phrase, or other sequence of
  characters which reads the same backward as forward, such as madam or
  racecar or the number 10801. Sentence-length palindromes may be
  written when allowances are made for adjustments to capital letters,
  punctuation, and word dividers, such as "A man, a plan, a canal,
  Panama!", "Was it a car or a cat I saw?" or "No 'x' in Nixon".

We can argue that your specification is stricter/simpler than that of wikipedia.

Unicode compliance

Your algorithm checks characters, not actual glyphs. This is OK for most applications that use ASCII, ANSI or other encodings that represent characters with 16 bits as limit. But for extended unicode, combining characters and other diacritics, your algorithm is flawed.

Design

Guard arguments

You provide a public method, so argument checks are in place.

if (text == null) throw new ArgumentNullException(nameof(text));

While (true) loops

Many consider while (true) bad practice, because it can lead to infinite loops in missing edge cases. Specially when code gets more complex, this is the case. Consider refactoring the loop to use a boolean. However, since this is a trivial and simple algorithm, I understand why you have done it like this.

Unnecessary variables

Since the variables a and b are only used once, there is no need to create variables.

char a = text[min];
char b = text[max];

if (a != b) {
    return false;
}

Shorter:

if (text[min] != text[max]) {
    return false;
}

Lack of variables

Consider creating variables for objects you require on multiple occasions.

lblInput.Foreground = Brushes.ForestGreen;
lblResult.Foreground = Brushes.ForestGreen;
lblTitel.Foreground = Brushes.ForestGreen;

public Brush ForegroundColor { get; } = Brushes.ForestGreen;

lblInput.Foreground = ForegroundColor;
lblResult.Foreground = ForegroundColor;
lblTitel.Foreground = ForegroundColor;

Normalize user input

You could argue that whitespace at the start or end of the input is mostly unintentional, so you should trim it. This depends on your guidelines for best practices concerning user experience.

  string text = InputText.Text;

 var text = InputText.Text.Trim();

Formatted strings

Formatted strings are cleaner and better optimized than concatenating strings.

  OutputText.Text = text + (isPalindrome ? " is a palindrome" : " is NOT a palindrome");

OutputText.Text = $"text is{(isPalindrome ? " " : " NOT ")}a palindrome";

Unoptimized code flow

This code clears the output after checking for a palindrome. You should put this before the check, and only calculate the palindrome if any non-empty input is provided.

if(InputText.Text == string.Empty)
            OutputText.Clear();

WPF design

For a trivial UI, using the code-behind of a control is fine. Consider using M-V-VM when you decide to extend the design and make it more complex.


Three methods to serialize 7-, 6-, and 3-dimensional int arrays Here's some code I wrote which sends multi-dimensional arrays to a file, is there any way I can make a generic method?

public void sendSevenToFile(int[][][][][][][] seven, String filename) throws IOException {
    ObjectOutputStream outputStream = new ObjectOutputStream(new FileOutputStream(filename));
    outputStream.writeObject(seven);
}

public void sendSixToFile(int[][][][][][] six, String filename) throws IOException {
    ObjectOutputStream outputStream = new ObjectOutputStream(new FileOutputStream(filename));
    outputStream.writeObject(six);
}

public void sendThreeToFile(int[][][] three, String filename) throws IOException {
    ObjectOutputStream outputStream = new ObjectOutputStream(new FileOutputStream(filename));
    outputStream.writeObject(three);
}

An array in Java is an object, not a primitive, so you can accept the parameter as Object, without specializing it as an array (you do not do that in your current code either, so you do not lose anything by this approach). Yes, you lose some type safety this way, but you can get it back during runtime by using <argument array object>.getClass().isArray(). Overall, my approach is:
/**
* Serializes an array of arbitrary dimensions to a file
* @param array    the array to serialize
* @param filename the name of the file to serialize array to
* @throws IllegalArgumentException if array is not an array
* @throws IOException              if some error was encountered during serialization
*/
public void serializeArrayToFile(Object array, String filename) throws IOException {
    if(!array.getClass().isArray()){
        throw new IllegalArgumentException("Cannot serialize non-array object " + array.toString());
    }
    ObjectOutputStream outputStream = new ObjectOutputStream(new FileOutputStream(filename));
    outputStream.writeObject(array);
}

And just in case you want this to be absolutely specific for only int arrays of arbitrary dimensions,
/**
* Serializes an array of arbitrary dimensions to a file
* @param array    the array to serialize
* @param filename the name of the file to serialize array to
* @throws IllegalArgumentException if array is not an int array
* @throws IOException              if some error was encountered during serialization
*/
public void serializeArrayToFile(Object array, String filename) throws IOException {
    Class<?> classOfArray = array.getClass();
    if(!classOfArray.isArray()){
        throw new IllegalArgumentException("Cannot serialize non-array object " + array.toString());
    }
    Class<?> componentType = classOfArray.getComponentType();
    if(!(componentType.isPrimitive() &&
         int.class.isAssignableFrom(componentType)) {
        // Can also be `UnsupportedOperationException1, depending on required semantics
        throw new IllegalArgumentException(String.format(
                                           "Serialization of non-`int` array %s of component type %s is not allowed",
                                            array.toString(), componentType.toString()));
    }
    ObjectOutputStream outputStream = new ObjectOutputStream(new FileOutputStream(filename));
    outputStream.writeObject(array);
}

Note that a nice side-effect of this approach is that you can now serialize arrays of arbitrary dimensions!
Note the naming convention - I use serialize instead of send, as that is what you are actually doing. Although I do not do it here for purposes of compatibility with the existing project structure, you might consider making this method static as it does not depend on the implicit this.
Also, the first array.toString() isn't required, the string concatenation will do it implicitly, but in my policy for such cases, explicit is better than implicit.
Addendum
The OP asked in the comments as to how to determine the number of dimensions of an array if it has been casted to Object. This is possible via reflection, whether by descending the type hierarchy until the result of <array object>.getClass().isArray() is false (slow), or by counting the number of '[' characters in the result of <array object>.getClass().getName() (faster). I'll illustrate the second method, which has been suggested by @wizzwizz4 in the comments, as that is more practical in all respects (I do not consider it to be "hacky", as @wizzwizz4 does, as the result of getName() for the class of any array type is explicitly documented as here.
public static int getDimensionsOfArray(Object array) {
    return (int) array.getClass()
                                .getName()
                                .chars()
                                .filter(c -> c == '[')
                                .count();
}

I use Java 8 Streams here, but you could just as well do it with a loop (I like the declarative approach of using Streams, as my primary language is Scala). The cast to int exists because Stream.count() returns a long, as Streams are lazy and can theoretically hold an infinite number of elements.
References:
I referred to this StackOverflow answer for the array type verification code.


Susy basic fixed grid I'm new to Susy and I'm loving it. I've been learning and trying a simple example - a simple grid that contains two columns at a fixed resolution of 320 pixels. One test would be to have two side by side columns (#x and #y) with gutter space in-between and another would be another two side by side columns (#a and #b) but without any gutter space in-between and they're both .

For the susy map, I've used .1334 for gutters and it all adds up to 320.01 pixels.

Questions

Is there a better way of defining the gutter space? having some decimals in 320.01 seems odd.
Is there a better way of getting #a and #b to be half widths without setting it to 160px and using no-gutters?
Is there an overall better way of writing this whole thing (maps, layouts, settings)?

The code

html {
    box-sizing: border-box;
}
*, *:before, *:after {
    box-sizing: inherit;
}

$susy: (
  math: static,
  columns: 2,
  gutters: .1334,
  column-width: 150px,
  global-box-sizing: content-box,
  debug: (
      image: show,
      color: rgba(#66f, .25),
   output: background
  ),
  use-custom: (
      box-sizing: true
  )
);

main {
    @include container(320px);
}

#x,
#y,
#a,
#b {
    border: solid 1px #000;
    float: left;
}

#x {
    @include span(1 of 2);
}

#y {
    @include span(1 of 2 last);
}

#a {
    @include span(160px no-gutters);
}

#b {
    @include span(160px last);
}

#z,
#c {
    @include span(2 of 2);
    border: solid 1px #000;
}

x

y

z

a

b

c

http://sassmeister.com/gist/d4b0d898d4a6c987f947

You'd be better off setting the gutter width as a ratio.
You can use math: fluid to create a fluid layout. Your column widths will then be calculated relative to the container without the need for specifying column width.
Sorry, don't know enough about Susy to give you general code improvement advice.


Web service to add a phone number with SMS validation I'm new to Clojure. How can I make this more idiomatic Clojure code? Among other things, there are three (do) blocks and I don't think using (def) as many times as I do is a recommended idea.

(defn add-phone
  "Add a phone number to a given service. There are a few steps we have to take:
    - Validate the service name and token
    - Validate the phone number
    - Send a text message to the number
    - Store the number in the database
  Arguments:
    request - an authorized HTTP request
  Returns:
    201 Resource Created - phone number added to a service
    400 Bad Request - invalid phone number or bad request"
  [request auth-type]
  (do
    (println "Url requested is " (:request-method request) " " (:uri request))
    (println request)
    (def auth-header ((:headers request) "authorization"))
    (def auth-map (utils/get-auth-header auth-type auth-header))
    (if (model/invalid-user-and-pass auth-map auth-type)
        {:status 401,
         :headers {"WWW-Authenticate" "Basic realm=\"smsauth.herokuapp.com\""}}
      (do
        (def raw-phone-no (:phone_number (:params request)))
        (def service-name (:service-name auth-map))
        (if raw-phone-no
          (do
            (def valid-phone-no (utils/parse-phone-no raw-phone-no))
            ; XXX get the service id some other way.
            (def service-map (model/service-in-db service-name))
            (def service-id (service-map :id))
            ; XXX check that service doesn't already have phone number
            (model/store-phone-no :numbers service-id valid-phone-no)
            (twilio/send-sms-verify twilio/sms-url valid-phone-no service-name)
            ; XXX jsonify this, send more information, document it.
            {:status 201,
             :body (json/json-str {
                      :service_name service-name,
                      :phone_number valid-phone-no,
                      :status "active",
                      :uri (:uri request)
                      ; should have some kind of unique id for the phone number
                      ; here.
                      ; also return the date
                    })})
          {:status 400,
            :body (json/json-str {:error "Please include a phone_number."})})))))

There are several errors/bad styles here:

First do isn't necessary, as body of function can contain several expressions
instead of def inside function it's better to use let that allows to "define" several variables in one step, and they will visible only inside this let block. So 2nd & 3rd do could be replaced with corresponding let
if variable is used only once, then you can include function call directly into map

So your code will look like:

(defn add-phone
  [request auth-type]
  (println "Url requested is " (:request-method request) " " (:uri request))
  (println request)
  (let [auth-header ((:headers request) "authorization")
        auth-map (utils/get-auth-header auth-type auth-header)]
    (if (model/invalid-user-and-pass auth-map auth-type)
      {:status 401,
       :headers {"WWW-Authenticate" "Basic realm=\"smsauth.herokuapp.com\""}}
      (let [raw-phone-no (:phone_number (:params request))
            service-name (:service-name auth-map)]
        (if raw-phone-no
          (let [valid-phone-no (utils/parse-phone-no raw-phone-no) ; XXX get the service id some other way.
                service-map (model/service-in-db service-name)
                service-id (service-map :id) ; XXX check that service doesn't already have phone number
                ]
            (model/store-phone-no :numbers service-id valid-phone-no)
            (twilio/send-sms-verify twilio/sms-url valid-phone-no service-name)
                                        ; XXX jsonify this, send more information, document it.
            {:status 201,
             :body (json/json-str {
                                   :service_name service-name,
                                   :phone_number valid-phone-no,
                                   :status "active",
                                   :uri (:uri request)
                                        ; should have some kind of unique id for the phone number
                                        ; here.
                                        ; also return the date
                                   })})
          {:status 400,
           :body (json/json-str {:error "Please include a phone_number."})})))))

P.S. it's also good idea to check value of service-map - if it will nil, then you'll get NPE. So instead of (service-map :id) you can write (:id service-map) that will correctly work if service-map will nil...


Replace function in VBA For reasons I'd rather like to not talk about, I am in a VBA environment that doesn't have the replace function (Access 97). I have wrote this to alleviate my frustrations.

My question is,  what is the quickest and most efficient way of writing this function? I am not married to it, I just want the best possible piece of code. While I do enjoy readability I am also OK with stepping away from that to learn something super cool.

Public Function RepStr(InputString As String, InvalidStr As String, ValidStr As String) As String
    Dim ModString As String

    ModString = InputString

    While InStr(1, ModString, InvalidStr) > 0
        ModString = Left(ModString, InStr(1, ModString, InvalidStr) - 1) & _
        ValidStr & Mid(ModString, Len(InvalidStr) + InStr(1, ModString, InvalidStr))
    Wend
    RepStr = ModString
End Function

This is a bit faster

Public Function RepStr2(txt As String, str1 As String, str2 As String) As String

    RepStr2 = Join(Split(txt, str1), str2)

End Function

Light testing with about 300K cells in col A, output in col B:

RepStr2(CStr(arr(i, 1)), "a", "x")

Col A          | Col B
abcabcabc      | xbcxbcxbc
aaa            | xxx
aaaaaaaaa      | xxxxxxxxx
aaaaaaaaaaaaaa | xxxxxxxxxxxxxx
... (duplicated down to 300K)

After repeated tests all functions run faster

Your version: 2.039 secs
This version: 1.406 secs
MS version:   1.688 secs (bellow)

Edit (as pointed out by ThunderFrame in the comment)

Microsoft's version is fast as well:

Public Function RepStr3(sIn As String, sFind As String, sReplace As String, _
                        Optional nStart As Long = 1, Optional nCount As Long = -1, _
                        Optional bCompare As VbCompareMethod = vbBinaryCompare) As String

    Dim nC As Long, nPos As Integer, sOut As String

    sOut = sIn
    nPos = InStr(nStart, sOut, sFind, bCompare)

    If nPos = 0 Then GoTo EndFn:

    Do
        nC = nC + 1
        sOut = Left(sOut, nPos - 1) & sReplace & Mid(sOut, nPos + Len(sFind))

        If nCount <> -1 And nC >= nCount Then Exit Do

        nPos = InStr(nStart, sOut, sFind, bCompare)

    Loop While nPos > 0

EndFn:

    RepStr3 = sOut

End Function


Finding minimal number of items from an array Given an array and a value x, I need an algorithm to find the minimum number of items to pick from the array so that the difference between the sum of those items and x is minimal.

public class Demo {

    static int[] solve(int arr[], int i, int esum, int csum, int size) {

        if (i == 0) {
            return new int[]{Math.abs(esum - csum), size};
        }

        int a1[] = solve(arr, i - 1, esum, csum + arr[i - 1], size + 1);
        int a2[] = solve(arr, i - 1, esum, csum, size);

        if (a1[0] < a2[0]) {
            return a1;
        } else if (a1[0] > a2[0]) {
            return a2;
        } else {
            if (a1[1] < a2[1]) {

                return a1;
            } else {
                return a2;
            }
        }
    }

    public static void main(String[] args) {
        //x = 108
        int arr[] = new int[]{70, 30, 33, 23, 4, 4, 34, 95, -50, -10, -10, -7};
        int a[] = solve(arr, arr.length, 108, 0, 0);
        System.out.println(a[0] + " " + a[1]);

    }
}

The output should be 0,3 (absolute difference), (number of items)

The first preference is the minimum absolute difference, only if there's multiple subsets with the same absolute difference we pick the one with the least number of items.

Scope

    static int[] solve(int arr[], int i, int esum, int csum, int size) {

You leave this as the default scope.  For Java, this is package private.

In this case, I would go with

    private static int[] solve(int arr[], int i, int esum, int csum, int size) {

The reason being that three of these parameters (i, csum, size) reflect internal knowledge of how the method works.  So I would make this private to make it clear that only people editing this class should use this method.  I would then add an additional method:

    public static int[] solve(int targetSum, int... numbers) {
        return solve(numbers, numbers.length, targetSum, 0, 0);
    }

This offers a public face to the private implementation.

I find names like numbers and targetSum easier to read than arr and esum.  In fact, I still don't know what esum is except by inspecting what it does.  Ideally, names should make it easier for me to understand the code.

This also allows solve to be called in more ways.  You can still call it

    solve(108, arr);

But you can also say

    solve(108, 70, 30, 33, 23, 4, 4, 34, 95, -50, -10, -10, -7);

If that happens to be easier.

This may not matter in real usage, but often it's easier to test in the latter form.

Object oriented

I would also consider making a class to hold the solution.  As is, the solution is held in a magic version of an int array.  What's in the array?  Which element is which?  Again, we have to go read the code to know.  I'd rather just be able to read an interface.  E.g.

public class PartialSum {

     private int distance;
     private int count;

     public PartialSumDistance(int distance, int count) {
         this.distance = distance;
         this.count = count;
     }

     public getDistance() {
         return distance;
     }

     public getSize() {
         return count;
     }

}

Now, instead of saying a1[0], I can say a1.getDistance(), which is much clearer about what is being discussed.

Optimizing the end condition

        if (i == 0) {

Consider

        if (i == 0 || esum == csum) {

If we have found a matching partial sum, we can never get closer to a solution.  We can only add additional items.  But smaller solutions are better.  So once we've found a matching sum, we can return.  This branch has nothing more to show us.  We've already found the best solution that it could find.

Sorting

If the input array were sorted (ascending), we could say

        if (csum < esum && arr[i - 1] < 0) {

And return the current solution.  We are currently as close to a solution as we could be.

Overall, there are \$2^n\$ sums to check where \$n\$ is the number of items in the array.  This is because each item can either be in or not in the sum.  So enumerating all the sums is \$\mathcal{O}(2^n)\$.  Sorting is only \$\mathcal{O}(n \log n)\$.  So we can sort for a trivial cost compared to the overall time to process the problem.  Meanwhile, we can potentially prune \$2^i\$ checks, where i is the number of elements remaining in the array as we process it.  Since i is potentially equal to \$n\$, this seems worthwhile.

Further optimization

To optimize further, we'd need to know more about how the data was meant to be used.  For example, if the number of targets to search is larger relative to \$2^n\$, it might make sense to pregenerate all the partial sums and put them in a NavigableMap or NavigableSet.  Then we could just fetch the appropriate value.  But that would be a lousy solution if we're only going to look up one target per array.


Multiples of 3 and 5 from Euler code challenge I have implemented the first coding challenge on http://projecteuler.net/. I would love to get it reviewed by some expert rubyists!

# Multiples of 3 and 5
#
# If we list all of the natural numbers below 10 that are multiples of
# 3 and 5, we get 3,5,6 and 9. THe sum of these multiples is 23.
#
# Find the sum of all the multiples of 3 or 5 below 1000.

class Multiples

  def multiples
    numbers = Array(1..999)
    multiples = Array.new
    for i in numbers
      if i%3 == 0 or i%5 == 0
        multiples.push(i)
      end
    end
    multiples
  end

  def sumMultiples(multiples)
    total = 0
    multiples.each { |i| total+= i }
    puts(total)
  end

end

multiples = Multiples.new
multiples.sumMultiples(multiples.multiples)

Don't use or. It's for flow control. Use || for boolean logic.
Don't use Array.new, use []
Don't use (1..999).to_a or Array(1..999) when all you really want to do is iterate. It's expensive to instantiate a 1000 item array when really you just want to iterate from 1 to 999
Any time you find yourself declaring an empty array and pushing items onto it in a loop, you are probably missing a map or select:

def multiples
  numbers = Array(1..999)
  multiples = Array.new
  for i in numbers
    if i%3 == 0 or i%5 == 0
      multiples.push(i)
    end
  end
  multiples
end

Could be better written this way:

def multiples
  (1..999).select do |i|
    i % 3 == 0 || i % 5 == 0
  end
end

Your summation loop follows a similar pattern:

def sumMultiples(multiples)
  total = 0
  multiples.each { |i| total+= i }
  puts(total)
end

Again, there is a more idiomatic way of doing this via inject:

def sumMultiples(multiples)
  total = multiples.inject(&:+)
end

A breakdown of how this works can be found in How to sum array of numbers in Ruby?.


Flattening a List containing Empty Lists As part of a text buffer implemented as a piece table, there is an often called procedure to flatten a nested list of character lists. The function is part of constructing substrings of the buffer for display, copying, and cutting.
I have been using a procedure based on Dan D.'s StackOverflow answer here: https://stackoverflow.com/a/33338608/193509. It has worked admirably except for one corner case.
If the user deletes all of the text in the buffer, the list of characters presented to the flatten function consists of a single element list. The single element is the empty list '().
The "stock" version of flatten does not handle this correctly. I've added a special case to the cond that recognizes the condition and responds appropriately (to my mind any way.) So,
(flatten '(())) ;=> '()

Here's what I have for flatten.
(define (flatten lst)
  (letrec ((helper (lambda (lst acc stk)
                     (cond ((null? lst)
                            (if (null? stk) (reverse acc)
                                (helper (car stk) acc (cdr stk))))

                           ((pair? (car lst))
                            (helper (car lst) acc (if (null? (cdr lst))
                                                      stk
                                                      (cons (cdr lst) stk))))
                           ;; Special case for a list containing just empty lists.
                           ((null? (car lst)) '())

                           (else
                            (helper (cdr lst) (cons (car lst) acc) stk))))))
    (helper  lst '() '())))

But the special case feels clunky and not very general. Is there a better way to do this?
More generally, what should a flattened list containing empty lists look like? I have just assumed that reducing to a single empty list is correct. Likewise, it seems to me that flattening a list containing a mixture of empty lists and non-empty should just return a list of the non-empty elements.
(flatten '(() () ())) ;=> '()
(flatten '((a) (b (x y) c) () (d) () ())) ;=> (a b x y c d)

Any thoughts on that?

In general flatten is intended to flat a tree, not a list, that is to collect all the non-null leaves of the tree in the same order as a classical traversal of the tree. See for instance the definition of flatten in the Racket Manual:

Flattens an arbitrary S-expression structure of pairs into a list. More precisely, v is treated as a binary tree where pairs are interior nodes, and the resulting list contains all of the non-null leaves of the tree in the same order as an inorder traversal.

With this characterization of the function, we do not see empty lists as something that must be “removed” from a list, and here is a simple definition that follows this idea:
(define (flatten tree)
  (let ((lst '()))
    (define (traverse subtree)
      (cond ((empty? subtree) '())
            ((cons? subtree)
               (traverse (car subtree))
               (traverse (cdr subtree)))
            (else (set! lst (cons subtree lst)))))
    (traverse tree)
    (reverse lst)))

The function visits the tree in the classical inorder traversal, and collects all the non-null leaves every time it finds one of them.
From this you can define a more optimized version, if you need to.


Modifying the response object for creating and downloading a document I've been looking at some code done in C# ASP .NET WebForms and for some reason I really don't like the below code. What is the best alternative for creating a document and downloading it in Webforms, on the click of a button? Should the developer be messing with the Response at all, in this way?

Response.ClearContent();
Response.BufferOutput = true;
Response.ContentType = "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet";
Response.AddHeader("content-disposition", string.Format("attachment; filename={0:yyyyMMdd}.xlsx", DateTime.Now));
Response.BinaryWrite(pck.GetAsByteArray());
Response.Flush();
Response.End();

Yes, in ASP.NET WebForms you don't have any nice helper method to hide this. However if you do it often you should make it easier and less error-prone.

public static void WriteDownload(this HttpResponse response, string fileName, byte[] data)
{
    if (String.IsNullOrWhiteSpace(fileName))
        throw new ArgumentException("File name must be specified.", "fileName");

    if (data == null)
        throw new ArgumentNullException("data");

    response.ClearContent();

    response.ContentType = MimeMapping.GetMimeMapping(fileName);
    response.AddHeader("content-disposition",
        String.Format("attachment; filename={0}", fileName));

    response.BinaryWrite(data);
    response.End();
}

It will then be used like this:

Response.WriteDownload(GetFileNameFromTodayDate(".xlsx"), pkg.GetAsByteArray());

Where GetFileNameFromTodayDate() is simply:

private static string GetFileNameFromTodayDate(string extension)
{
    return String.Format("{0:yyyyMMdd}.{1}",
        DateTime.Now, extension.TrimStart(' ', '.');
}

Few more notes:

BufferOutput is true by default, you do not need to repeat it. However I'd consider to set it to false.
If you're using .NET Framework 4.5+ you do not need any function to resolve MIME type otherwise you'd better to resort some helper function but do not hard-code it anywhere.
You do not need Response.Flush() because buffering is on and, moreover, you do not stream anything else before Response.End(), it's just redundant.


Checking for pangrams in C This will test if a user entered an pangram (every letter in alphabet used at least once), however with 4 for loops there's got to be a better algorithmic approach or with the language of C itself.

pangram.c

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

 typedef enum { true, false } bool;

 typedef struct node{
     char letter;
     bool exists;
 } node;

 int main(int argc, char *argv[]){
     int SIZE = 500;
     char input[SIZE];

     printf("Enter your pangram: ");
     fgets(input, SIZE, stdin);

     // 26 letters
     node alphabet[26];
     int i = 0;
     for(char c='a'; c<='z'; ++c, i++){
         alphabet[i].letter = c;
         alphabet[i].exists = false;
     }

     for(int i=0; i<SIZE; i++){
         for(int j=0; j<27; j++){
             if(isalpha(input[i]) && input[i] == alphabet[j].letter){
                 alphabet[j].exists = true;
             }
         }
     }

     for(int i=0; i<27; i++){
         if(alphabet[i].exists==false){
             printf(" no pangram, missing letter.\n");
             return 1;
         }
     }

     printf("you've entered a pangram.\n");

     return 0;
 }

command line:

>> gcc -o pangram pangram.c -std=c99; ./pangram
Enter your pangram: this should fail
 no pangram, missing letter.

>> gcc -o pangram pangram.c -std=c99; ./pangram
Enter your pangram: the quick brown fox jumps over the lazy dog
you've entered a pangram.

Please never define true to have the value 0, since that value is reserved for the name false. Just include <stdbool.h> instead of defining the type yourself.

Do not call any function from <ctype.h> with a char argument since that can easily lead to undefined behavior.

In the call to fgets fails, the input array may be uninitialized, which would also lead to undefined behavior.

Accessing alphabet[26] leads to undefined behavior since there valid array indices go from 0..25.

The rest of your code looks well-organized and is easy to read. Congratulations on that.

For your primary question of solving the pangram with fewer than 4 for loops, you can find at least 10 different solutions by searching the web since the pangram is a classical programming task.

Forget the above paragraph. I just searched for "pangram c" and found only crappy solutions, and telling these apart from the good ones is impossible for a beginner.

One idea is to remember for each letter whether it was found it not:

bool found[26] = { false };   // This initializes all 26 values at once, but only works for "zero" values.
int remaining = 26;

for (size_t i = 0; input[i] != '\0'; i++) {
    char ch = input[i];

    if ('a' <= ch && ch <= 'z') {
        if (!found[ch - 'a']) {
            found[ch - 'a'] = true;
            remaining--;
        }
    }
}

If, at the end of the loop, there is no remaining character, you have found them all, and the input is a pangram. All with a single loop.

Note: the code I suggested only works when all 26 letters are defined in a contiguous block of the character set. On all modern systems this is the case. When working with IBM machines and EBCDIC encoding, this will not work. On Rosetta Code, the code is perfect and also handles these exotic cases. It uses the same idea and takes care of everything. It looked complicated at first sight, therefore I preferred to explain the basic idea first. But now you should be able to understand the code over there.


One instance per method call or multiple method call per instance I have the following class designs, and I'm looking for the best practices, aligned to the OOP design principles/patterns.

The ParameterParser will be used inside a foreach block through 10 items approximately.

Any comments regarding the method body will also be appreciated, though the focus of the question is on the class design/usage.

Use the constructor to take the parameter and parse (one instance per parse)

public class ParameterParser
{
    private readonly Type _propertyType;
    private readonly string _parameter;

    public ParameterParser(Type propertyType, string parameter)
    {
        _propertyType = propertyType;
        _parameter = parameter;
    }

    public object Parse()
    {
        if (typeof(IEnumerable).IsAssignableFrom(_propertyType)
         || typeof(IEnumerable<>).IsAssignableFrom(_propertyType))
            return _parameter.Split(new[] { ',', ';', '|' }, StringSplitOptions.RemoveEmptyEntries);

        if (_propertyType.IsEnum)
            return Enum.Parse(_propertyType, _parameter);

        return Convert.ChangeType(_parameter, _propertyType);
    }
}

Parameterless constructor, multiple parsing per instance

public class ParameterParser2
{
    public object Parse(Type propertyType, string parameter)
    {
        //same code
    }
}

A helper class, static method

public class ParameterHelper
{
    public static object Parse(Type propertyType, string parameter)
    {
        //same code
    }
}

I think the first one is more likely to be the best, but I can't explain which exactly are the advantages over the others. The question is "Considering design principles, which one is the best and why? Is there any pattern which would fit the scenario?".

Option 1 is surprising.
I find it funny that you have return Enum.Parse(_propertyType, _parameter); in a parameterless Parse() method. Enum.Parse is just an example (int.Parse would be another); these methods are commonly used by C# programmers and your single-use parser seems to break POLS in my opinion.
Option 2 is [more] consistent with the framework.
I don't mean to repeat what I just said, but it feels natural for a Parse method to take in all the parameters it needs. Now there is another problem here: your API is exposing object, which means the parsed value will need to be cast from the calling code, into the correct type. This is bad. Very bad. Nothing should ever be returning an object. If you're parsing a value type, you're boxing it right there. Is that not likely?
How about keeping it type-safe and taking a generic type argument instead of a Type parameter?
public T Parse<out T>(string value)
{
    ...
    return Convert.ChangeType(value, typeof(T));
}

This way if T is a value type, you're not boxing it. And you're not returning an object, and it's still the calling code that decides what type to parse the string with.
Option 3 feels wrong.
Whenever I feel the need for a class to be called xxxxxHelper, something itches. The class should be static as well, if it's only going to expose static methods. That said, as @Simon has noted (oh, well, Simon says...) this brings DI to its knees. Best stay clear from that.


File handling script This is my first bash script so I ask to you to say what you think.

The script must check every directory and if it finds a pdf file in this directory:  move all the pdf files to parent directory, change the name of the files and remove directory.

So if I had this folder structure:

folder1
  file.pdf
  file.txt
folder2
  some.pdf
  another.pdf
folder3
  some.txt

the script must change it to:

 folder1.pdf
 folder2.pdf
 folder2_2.pdf
 folder3
   some.txt

Here is a script code:

#!/bin/bash

working_directory="$HOME/MEGAsync/downloads/books"

cd $working_directory

# for each directory in working directory
for D in `find . -type d`
do
    # skip '.'
    if [[ $D != "." ]]; then
        # get folder name
        folder_name=${D##*/}
        # and cd to folder
        cd $D

        # number of pdf files in the current directory
        declare -i pdf_count=0
        # for each file in the current directory
        for f in *; do

            # get file without path
            file=$(basename "$f")
            extension="${file##*.}"
            filename="${file%.*}"

            if [[ $extension == 'pdf' ]]; then
                pdf_count=pdf_count+1
                if [[ pdf_count < 2 ]]; then
                    new_filename="${working_directory}/${folder_name}.pdf";
                else
                    new_filename="${working_directory}/${folder_name}_${pdf_count}.pdf";
                fi

                echo cp $working_directory/$folder_name/$f $new_filename
                cp $working_directory/$folder_name/$f $new_filename
            fi
        done

        # return to parent folder
        cd ..

        # delete directory if pdf-file was found
        if [[ $pdf_count != "0" ]]; then
            echo rm -R $working_directory/$folder_name
            rm -R $working_directory/$folder_name
        fi
    fi
done

For a first script, this is great, and I am impressed.

your structure is neat
you are using loops properly
you are using variable substitution which is 'advanced'

The script appears that it will work for all the standard cases, and, I have seen much worse in 'production' code.

So, as a beginner, well done.

What's next?

the edge cases are going to kill this script - spaces in file names.
the logging and error handling could be better
I recommend basename and dirname instead of the variable regexes you use:

dirname: http://linux.die.net/man/1/dirname
basename: http://linux.die.net/man/1/basename (note that basename path/to/file.pdf .pdf returns file)

consistency on for->do loop - you have do on the next line once, and then for ... ; do the next time. Either is fine, but be consistent.

All of the error-handling, logging, and the spaces-in-files can be solved easily by using a function for running commands. Also, the "$@" construct in bash is 'special'

Consider the bash function:

function runcmd {
    echo "$@"
    # Fail if the command fails...
    "$@" || exit 1
}

So, this function will print the command, and then will run it, and exit the script if the command fails.

Now, instead of structures like:

            echo cp $working_directory/$folder_name/$f $new_filename
            cp $working_directory/$folder_name/$f $new_filename

you can do:

            runcmd cp "$working_directory/$folder_name/$f" $new_filename

and you will still get the echo, you will get the new error handling, and you will handle spaces in names just fine.


Instantiating a property when the required attributes have been set I have a simple class which holds my product. Someone can add a serial to this product and also a purchase id. On change of the properties, I want to check whether both properties are set, and get the license class and add it to the product.

public class License
{
    public bool IsTrial { get; set; }

    public bool IsValid { get; set; }

    public string Type { get; set; }

    public License()
    {

    }

    public License(string purchaseId, string serial)
    {
        this.IsTrial = true;
        this.IsValid = true;
        this.Type = "Enterprise";
    }

Example License class

public class Product
{
    private License _license;
    public License license
    {
        get
        {
            return _license;
        }
        set
        {
            _license = value;
        }
    }

    public string Name { get; set; }

    private string _purchaseId;
    public string PurchaseId {
        get
        {
            return _purchaseId;
        }
        set
        {
            _purchaseId = value;
            GetLicense();
        }
    }

    private string _serial;
    public string Serial
    {
        get
        {
            return _serial;
        }
        set
        {
            _serial = value;
            GetLicense();
        }
    }

    private void GetLicense()
    {
        if (!string.IsNullOrEmpty(this.Serial) && !string.IsNullOrEmpty(this.PurchaseId))
        {
            License lice = new License(serial, PurchaseId);
            this.License = lice;
        }
    }

That's the approach I used. Is this a correct way to do it? How is this called and what is the best practice? I need to use .NET Framework 3.5.

Getters & Setters + Validation

I think it's better to validate the serial and the purchase-id in their setters instead in the GetLicense method that actually could be a readonly-property.

private void GetLicense()
{
    if (!string.IsNullOrEmpty(this.Serial) && !string.IsNullOrEmpty(this.PurchaseId))
    {
        License lice = new License(serial, PurchaseId);
        this.License = lice;
    }
}

After this tiny refactorings the result is:

public class Product
{
    // it's prettier if you group private fields togehter at the top
    private string _purchaseId;
    private string _serial;

    public string Name { get; set; }

    public string PurchaseId
    {
        get { return _purchaseId; }
        set
        {
            if (string.IsNullOrEmpty(value))
                throw new ArgumentNullException(nameof(PurchaseId));
            _purchaseId = value;
        }
    }

    public string Serial
    {
        get { return _serial; }
        set
        {
            if (string.IsNullOrEmpty(value))
                throw new ArgumentNullException(nameof(Serial));
            _serial = value;
        }
    }

    public License License
    {
        get { new License(Serial, PurchaseId); }
    }
}

Magic Strings

  this.Type = "Enterprise";

You should create a const/readonly string for the "Enterprise" and name it for example DefaultLicenseType

Bug

There is a bug. The license constructor expects purchaseId + serial and you create a license with a serial + purchase id

Constructor signature:

public License(string purchaseId, string serial)

License creation:

new License(serial, PurchaseId);


Creating a graph in Java I'm building an application that implements the A* algorithm to calculate a route between two rooms. I am trying to create a graph which the algorithm can operate on and I am not sure if this is the right way to do it.

Here's what I've done so far:

Vect2.java:

package myalgorithm;

public class Vect2 {

    private int x;
    private int y;

    public Vect2(int x,int y ){

        this.x = x;
        this.y = y;

        }

}

Node.java

package myalgorithm;

public class Node {

    Node parent;
    Vect2 vector;
    public int x;
    public int y;

    public double f;
    public double g;
    public double h;

    public Node( int x,int y,Node parent, double g,double h){

        this.x = x;
        this.y = y;
        this.parent= parent;
        this.g = g;
        this.h = h;
        this.f= this.g + this.h;

    }
}

NodeGraph.java

package myalgorithm;
import java.util.Arrays;

public class NodeGraph {

    Node a = new Node(88,623,null,0,0);
    Node b = new Node(727,627,null,0,0);
    Node c = new Node(723,93,null,0,0);
    Node d = new Node(90,92,null,0,0);
    Node e = new Node(94,349,null,0,0);
    Node f = new Node(397,358,null,0,0);
    Node g = new Node(722,339,null,0,0);
    Node[] arr = new Node[7];

public NodeGraph init(){

    arr[0]= a;
    arr[0]= b;
    arr[0]= c;
    arr[0]= d;
    arr[0]= e;
    arr[0]= f;
    arr[0]= g;

    return this;
}
public void createMatrx(){
boolean[][] matrix = new boolean[7][];
for (int i=0; i<matrix.length; i++) matrix[i] = new boolean[7];

int nodeA = Arrays.binarySearch(arr, a);
int nodeB = Arrays.binarySearch(arr, b);
int nodeC = Arrays.binarySearch(arr, c);
int nodeD = Arrays.binarySearch(arr, d);
int nodeE = Arrays.binarySearch(arr, e);
int nodeF = Arrays.binarySearch(arr, f);
int nodeG = Arrays.binarySearch(arr, g);

matrix[nodeA][nodeB] = true;
matrix[nodeA][nodeE] = true;
matrix[nodeB][nodeA] = true;
matrix[nodeB][nodeG] = true;
// A is connected to D
matrix[nodeC][nodeG] = true;
matrix[nodeC][nodeD] = true;
// B is connected to D
matrix[nodeD][nodeC] = true;
matrix[nodeD][nodeE] = true;
matrix[nodeE][nodeD] = true;
// C is connected to D
matrix[nodeE][nodeF] = true;
matrix[nodeE][nodeA] = true;
matrix[nodeF][nodeE] = true;
matrix[nodeE][nodeG] = true;

matrix[nodeG][nodeF] = true;
matrix[nodeE][nodeB] = true;
matrix[nodeE][nodeC] = true;

matrix[nodeD][nodeC] = true;
}
}

In general, you have some problems being consisten in your coding.

1) Formatting:

In your NodeGraph class you are not consistent in your indentation. Your createMatrx (which has a typo and should be called createMatrix) you are not using any indentation.

2) Access modifiers:

In your class Node2 you set your members private, as expected.

In your class Node you don't specifically set the access modifier as private for some members, and you set them explicitly publbic for others. I would expect all members to be private.

3) Naming:

Single character variable names are not very descriptive. Name your variables according to their purpose.


Asp.net MVC login method I wrote this login method:

    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult Login(LoginModel logModel, string returnUrl)
    {
        if (!ModelState.IsValid)
        {
            TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
            return RedirectToAction("LoginFailed");
        }
        User user;
        try
        {
            user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));
        }
        catch(Exception ex)
        {
            //log it
            TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.DatabaseException;
            return RedirectToAction("LoginFailed");
        }
        if (user == null)
        {
            TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
            return RedirectToAction("LoginFailed");
        }

        //if we got so far it means that username does exist
        //is user blocked?
        if (user.IsLockedOut)
        {
            TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.AccountBlocked;
            return RedirectToAction("LoginFailed");
        }
        //is the password correct?
        string passHash = ComputePasswordHash(logModel.LoginPassword, user.Salt);
        if (!String.Equals(passHash, user.Password))
        {
            user.FailedPasswordAttemptCount++;
            try
            {
                _usersRepository.Update(user);
            }
            catch(Exception ex)
            {
                //just log it, not a big deal the FailedPasswordAttemptCount could not be updated
                user.FailedPasswordAttemptCount--;
            }
            TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
            TempData[Keys.ACCOUNT_TEMPDATA_INVALIDATTEMPTS] = user.FailedPasswordAttemptCount;
            return RedirectToAction("LoginFailed");
        }
        //is the account activated?
        if (!user.IsApproved)
        {
            TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.AccountNotAcctivated;
            return RedirectToAction("LoginFailed");
        }
        //if we got so far then everything is ok
        FormsAuthentication.SetAuthCookie(logModel.LoginUsername, false);
        if (Url.IsLocalUrl(returnUrl) && returnUrl.Length > 1 && returnUrl.StartsWith("/") && !returnUrl.StartsWith("//") && !returnUrl.StartsWith("/\\"))
        {
            return Redirect(returnUrl);
        }
        else
        {
            return RedirectToAction("Index", "Home");
        }
    }

Is it ok to retrieve all the user related information, specifically sensitive info like password? Should I first check if that username exists, then select the salt, compute the password hash and check if there is a user with that username and that password hash?
What would you improve?

If you can, I would encrypt the password entered before it is passed back to the service.  This will eliminate any chance it could be intercepted.  As for the rest of your first question, I think the how you process is up to you.  I don't think there is a right or wrong answer.

Now onto improvements:

First off, I think you code is doing too much.  This is very evident by then number of if/else and return statements.

I think a better approach would be to pull out all of the logic into a number of separate methods, or even better classes that all work together to process the login, and decide what the appropriate course of action is.  I'm going to tackle the multiple method approach.  If you want to see how multiple classes can be implemented let me know, and I'll update the post then.

There are two important pieces of data that you require, the LoginStatus and the ActionResult  Keeping this in mind, I would first create a class call LoginResult, which contains the two pieces of information in it

internal class LoginResult
{
    public ActionResult ActionResult { get; private set; }
    public LoginStatus LoginStatus { get; private set; }

    public LoginResult(ActionResult actionResult, LoginStatus loginStatus)
    {
        ActionResult = actionResult;
        LoginStatus = loginStatus;
    }
}

This will allow for each method to return the important data.

Now that you have the first building block, I would start by pulling out every if/else statement in your original code into its own method, returning a LoginResult.  There is another important piece of data returned from one of the checks, and that is user, so add a UserLoginResult class:

internal class LoginResult
{
    public ActionResult ActionResult { get; private set; }
    public LoginStatus LoginStatus { get; private set; }

    public LoginResult(ActionResult actionResult, LoginStatus loginStatus)
    {
        ActionResult = actionResult;
        LoginStatus = loginStatus;
    }
}

internal class RetrieveUserResult
{
    public LoginResult Result { get; private set; }
    public User User { get; private set; }

    public RetrieveUserResult(LoginResult loginResult, User user)
    {
        Result = loginResult;
        User = user;
    }
}

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Login(LoginModel logModel, string returnUrl)
{
    _returnUrl = returnUrl;
    _logModel = logModel;

    var result = EnsureModalStateIsValid();
    if (result.LoginStatus == LoginStatus.InvalidCredentials)
    {
        TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;

        return result.ActionResult;
    }

    var retrieveResult = TryToRetrieveUserFromRepository();

    if (retrieveResult.User == null)
    {
        TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;
        return result.ActionResult;
    }

    var lockedResult = CheckIfUserIsLockedOut(retrieveResult.User);

    if (lockedResult.LoginStatus == LoginStatus.AccountBlocked)
    {
        TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = lockedResult.LoginStatus;
        return lockedResult.ActionResult;
    }

    var passwordResult = ConfirmUserPasswordMatches(retrieveResult.User);
    if (passwordResult.LoginStatus == LoginStatus.InvalidCredentials)
    {
        TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = passwordResult.LoginStatus;
        return passwordResult.ActionResult;
    }

    //is the account activated?
    var activatedResult = ConfirmUserIsActivated(retrieveResult.User);
    if (activatedResult.LoginStatus == LoginStatus.AccountNotActivated)
    {
        TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = activatedResult.LoginStatus;
        return activatedResult.ActionResult;
    }

    return activatedResult.ActionResult;

}

private LoginResult EnsureModalStateIsValid()
{
    if (!ModelState.IsValid)
    {
        return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials);
    }

    // Should come up with a dummy Action for this case
    return DetermineSuccessResult();
}

private RetrieveUserResult TryToRetrieveUserFromRepository()
{
    try
    {
        var user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));
        return new RetrieveUserResult(new LoginResult(RedirectToAction("Index", "Home"), LoginStatus.LoginOK), user);
    }
    catch (Exception ex)
    {

        return new RetrieveUserResult(DetermineSuccessResult(), null);
    }
}

private LoginResult CheckIfUserIsLockedOut(User user)
{
    if (user.IsLockedOut)
    {
        return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountBlocked);
    }

    return DetermineSuccessResult();
}

private LoginResult ConfirmUserPasswordMatches(User user)
{
    string passHash = ComputePasswordHash(logModel.LoginPassword, user.Salt);
    if (!String.Equals(passHash, user.Password))
    {
        user.FailedPasswordAttemptCount++;
        try
        {
            _usersRepository.Update(user);
        }
        catch (Exception ex)
        {
            //just log it, not a big deal the FailedPasswordAttemptCount could not be updated
            user.FailedPasswordAttemptCount--;
        }

        TempData[Keys.ACCOUNT_TEMPDATA_INVALIDATTEMPTS] = user.FailedPasswordAttemptCount;

        return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials);
    }

    return DetermineSuccessResult();
}

private LoginResult ConfirmUserIsActivated(User user)
{
    if (!user.IsApproved)
    {
        return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountNotActivated);
    }

    return DetermineSuccessResult();
}

private LoginResult DetermineSuccessResult()
{
    FormsAuthentication.SetAuthCookie(_logModel.LoginUsername, false);
    return new LoginResult(ReturnUrlIsValid() ? Redirect(_returnUrl) : RedirectToAction("Index", "Home"), LoginStatus.LoginOK);
}

private bool ReturnUrlIsValid()
{
    return (Url.IsLocalUrl(_returnUrl) && _returnUrl.Length > 1 && _returnUrl.StartsWith("/") &&
        !_returnUrl.StartsWith("//") && !_returnUrl.StartsWith("/\\"));
}

The Login method still is doing too much, but at least it has moved the checking logic out into sub routines.  You should now start to see a pattern emerging in the Login method: DoCheck -> CheckResult -> return if failed -> repeat.  This can be moved into a chain event where if the first one fails, it returns, if it passes, it calls the next event:

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Login(LoginModel logModel, string returnUrl)
{
    _returnUrl = returnUrl;
    _logModel = logModel;

    var result = Process();

    TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;

    return result.ActionResult;
}

private LoginResult Process()
{
    LoginResult result = EnsureModalStateIsValid();
    return result.LoginStatus == LoginStatus.InvalidCredentials ? result : TryToRetrieveUserFromRepository();
}

private LoginResult EnsureModalStateIsValid()
{
    return !ModelState.IsValid
                ? new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials)
                : DetermineSuccessResult();

    // Should come up with a dummy Action for this case
}

private LoginResult TryToRetrieveUserFromRepository()
{
    try
    {
        var user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));

        return retrieveUserResult.User == null ? result : CheckIfUserIsLockedOut(retrieveUserResult.User);
    }
    catch (Exception ex)
    {
        return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.DatabaseException);
    }
}

private LoginResult CheckIfUserIsLockedOut(User user)
{
    return user.IsLockedOut
                ? new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountBlocked)
                : ConfirmUserPasswordMatches(user);
}

private LoginResult ConfirmUserPasswordMatches(User user)
{
    //is the password correct?
    string passHash = ComputePasswordHash(logModel.LoginPassword, user.Salt);
    if (!String.Equals(passHash, user.Password))
    {
        user.FailedPasswordAttemptCount++;
        try
        {
            _usersRepository.Update(user);
        }
        catch (Exception ex)
        {
            //just log it, not a big deal the FailedPasswordAttemptCount could not be updated
            user.FailedPasswordAttemptCount--;
        }

        TempData[Keys.ACCOUNT_TEMPDATA_INVALIDATTEMPTS] = user.FailedPasswordAttemptCount;

        return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials);
    }

    return ConfirmUserIsActivated(user);
}

private LoginResult ConfirmUserIsActivated(User user)
{
    return !user.IsApproved
                ? new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountNotActivated)
                : DetermineSuccessResult();
}

private LoginResult DetermineSuccessResult()
{
    FormsAuthentication.SetAuthCookie(_logModel.LoginUsername, false);
    return new LoginResult(ReturnUrlIsValid() ? Redirect(_returnUrl) : RedirectToAction("Index", "Home"),
                            LoginStatus.LoginOK);
}

private bool ReturnUrlIsValid()
{
    return (Url.IsLocalUrl(_returnUrl) && _returnUrl.Length > 1 && _returnUrl.StartsWith("/") &&
            !_returnUrl.StartsWith("//") && !_returnUrl.StartsWith("/\\"));
}

With this interation, you get rid of the RetrieveUserResult class.

There are a few more things I'd suggest, but I don't like giving too much information in these all at once.

Hope this helps.


Counting length-2 substrings that are common to two strings at the same offset -- Python I solved the CodingBat task:

Given 2 strings, a and b, return the number of the positions where they contain the same length 2 substring. So "xxcaazz" and "xxbaaz"
yields 3, since the "xx", "aa", and "az" substrings appear in the same
place in both strings.
stringMatch("xxcaazz", "xxbaaz") → 3
stringMatch("abc", "abc") → 2
stringMatch("abc", "axc") → 0

import doctest

def all_two_chars_occurencies(string):
    """
    >>> list(all_two_chars_occurencies('abcd'))
    ['ab', 'bc', 'cd']
    >>> list(all_two_chars_occurencies('xxcaazz'))
    ['xx', 'xc', 'ca', 'aa', 'az', 'zz']
    """
    for index, char in enumerate(string[:-1]):
        yield char + string[index + 1]

def common_two_chars_occurences(a, b):
    """
    Given 2 strings, a and b, return the number of the positions where
    they contain the same length 2 substring.

    >>> common_two_chars_occurences('xxcaazz', 'xxbaaz')
    3
    >>> common_two_chars_occurences('abc', 'abc')
    2
    >>> common_two_chars_occurences('abc', 'axc')
    0
    """
    equal_duets = 0
    for a_duet, b_duet in zip(all_two_chars_occurencies(a),
                              all_two_chars_occurencies(b)):
        if a_duet == b_duet:
            equal_duets += 1
    return equal_duets

if __name__ == "__main__":
    doctest.testmod()

There is no such word as "occurencies".  In any case, all_two_chars_occurencies is a very long name.  I suggest pairwise_chars.  Note that it could also be written in a style similar to the pairwise() recipe in Python's documentation.

In Python, explicit looping is slightly cumbersome.  Fortunately, Python offers many ways to just do what you want as a "one-liner", without looping.  Here's one approach, which uses a generator expression, and this technique to find its length.

def common_two_chars_occurences(a, b):
    """
    Given 2 strings, a and b, return the number of the positions where
    they contain the same length 2 substring.

    >>> common_two_chars_occurences('xxcaazz', 'xxbaaz')
    3
    >>> common_two_chars_occurences('abc', 'abc')
    2
    >>> common_two_chars_occurences('abc', 'axc')
    0
    """
    return sum(1 for pair
                 in zip(pairwise_chars(a), pairwise_chars(b))
                 if pair[0] == pair[1])