Code Quality Catch-22 -- Duplication vs Complexity I've started using Code Climate to assess some of my code quality and ran into a situation that I can't seem to find a good solution for. In short, in trying to solve some code duplication that resulted in a low "grade" by using metaprogramming, I simply get a low grade for code complexity instead.

Anyone know a good a way out of this trap?

Code Quality Background Info

Here are some resources that point to the underlying libraries used by Code Climate to assess code quality. I think these are pretty standard in the ruby world so this  isn't a Code Climate specific question.

Deciphering Ruby Code Metrics

Tools used by Code Climate

Before Code (duplication problem)

Several associations on User that have basically identical functionality.

  has_many :user_conditions
  has_many :conditions, :through => :user_conditions do
    def <<(new_item) # disable duplicate addition
      super( Array(new_item) - proxy_association.owner.conditions )
    end
  end
  def activate_condition(condition)
    ActiveRecord::Base.transaction do
      self.conditions << condition
      self.update_attribute(:active_conditions, (self.active_conditions | [condition.id.to_s]))
    end
  end
  def deactivate_condition(condition)
    self.update_attribute(:active_conditions, (self.active_conditions - [condition.id.to_s]))
  end

  has_many :user_symptoms
  has_many :symptoms, :through => :user_symptoms do
    def <<(new_item) # disable duplicate addition
      super( Array(new_item) - proxy_association.owner.symptoms )
    end
  end
  def activate_symptom(symptom)
    ActiveRecord::Base.transaction do
      self.symptoms << symptom
      self.update_attribute(:active_symptoms, (self.active_symptoms | [symptom.id.to_s]))
    end
  end
  def deactivate_symptom(symptom)
    self.update_attribute(:active_symptoms, (self.active_symptoms - [symptom.id.to_s]))
  end

  has_many :user_treatments
  has_many :treatments, :through => :user_treatments do
    def <<(new_item) # disable duplicate addition
      super( Array(new_item) - proxy_association.owner.treatments )
    end
  end
  def activate_treatment(treatment)
    ActiveRecord::Base.transaction do
      self.treatments << treatment
      self.update_attribute(:active_treatments, (self.active_treatments | [treatment.id.to_s]))
    end
  end
  def deactivate_treatment(treatment)
    self.update_attribute(:active_treatments, (self.active_treatments - [treatment.id.to_s]))
  end

After Code (complexity problem)

Moved out of User into a module, admittedly this code is hard to grasp.

module UserTrackables
  extend ActiveSupport::Concern

  included do |base|

    %w( treatments conditions symptoms ).each do |trackable|

      base.class_eval do
        has_many "user_#{trackable}".to_sym
        has_many trackable.to_sym, :through => "user_#{trackable}".to_sym do

          # disable duplicate addition
          def <<(new_item)
            super( Array(new_item) - proxy_association.owner.send(proxy_association.reflection.plural_name.to_sym) )
          end

        end
      end

      define_method("activate_#{trackable.singularize}") do |trackable_object|
        ActiveRecord::Base.transaction do
          self.send(trackable) << trackable_object
          self.update_attribute("active_#{trackable}".to_sym, (self.send("active_#{trackable}") | [trackable_object.id.to_s]))
        end
      end

      define_method("deactivate_#{trackable.singularize}") do |trackable_object|
        self.update_attribute("active_#{trackable}".to_sym, (self.send("active_#{trackable}") - [trackable_object.id.to_s]))
      end

      define_method("current_#{trackable}") do
        trackable.singularize.capitalize.constantize.where(id: self.send("active_#{trackable}".to_sym).map(&:to_i))
      end

    end
  end

end

Your first version - extending the association - is probably the right approach in general, but there's another way to remove duplication, without resorting to metaprogramming.

Instead of defining methods as inlined extensions you can create a module for the association extensions. But rather than adding that module to the User model, you can add it the associations instead (see the docs)

E.g. if you create a module like so:

module TrackableAssociation
  def <<(item)
    # ...
  end

  def activate(item)
    # ...
  end

  def deactivate(item)
    # ...
  end
end

You should be able to add it like so:

class User < ActiveRecord::Base
  has_many :conditions, through: :user_conditions, -> { extending TrackableAssociation }
  has_many :symptoms, through: :user_symptoms, -> { extending TrackableAssociation }
  has_many :treatments, through: :user_treatments, -> { extending TrackableAssociation }
end

Which, in turn, should let you do things like

user.conditions.activate(condition)

Now your extensions are defined without using duplication or metaprogramming.

However. Your approach of storing an array of "activated IDs" smells fishy. It's fragile, and it requires the association extensions to modify the user instance. The code paths get tricky.

active should be an attribute on a join model - or at least a column in the join table. If I'm reading your code right, you've got Condition, Symptom and Treatment models, and join tables between them and User. You should probably make actual models for the join; said models can then have an active boolean attribute.


Throwing a custom error in a stored procedure I'm wanting to throw an error when reference keys are missing, however instead of failing through a integrity check, I want to list out the missing keys. I've created the below which works. However I'm hoping there is a way to optimise it and reduce the number of lines of code.

DECLARE @NonRefKeys INT

SELECT @NonRefKeys = SUM(1)
FROM staging.Sale sa
WHERE NOT EXISTS (
    SELECT cu.Customer_Shipping_ID
    FROM staging.Customer cu
    WHERE LTRIM(RTRIM(sa.Customer_Shipping_ID)) = LTRIM(RTRIM(cu.Customer_Shipping_ID)))

IF @NonRefKeys IS NOT NULL
BEGIN

IF OBJECT_ID('tempdb..#Missing_Ref') IS NOT NULL
DROP TABLE #Missing_Ref;

SELECT sa.Customer_Shipping_ID AS ID
INTO #Missing_Ref
FROM staging.Sale sa
WHERE NOT EXISTS (
        SELECT cu.Customer_Shipping_ID
        FROM staging.Customer cu
        WHERE LTRIM(RTRIM(sa.Customer_Shipping_ID)) = LTRIM(RTRIM(cu.Customer_Shipping_ID)))

DECLARE @Current_ID VARCHAR(50);
DECLARE @Missing_ID VARCHAR(MAX) = '';
DECLARE @Output_Error VARCHAR(MAX);

DECLARE id_cursor CURSOR FOR
SELECT ID
FROM #Missing_Ref;

OPEN id_cursor
FETCH NEXT FROM id_cursor INTO @Current_ID

I'm not sure I completely understand your logic, but since this is a stored procedure the temp table isn't visible outside it so you don't need to check if it exists (unless you have other code in the same procedure that might have created it already).

And the most obvious way to reduce the number of lines is not to count the number of IDs and then get them, but to just get them and use @@ROWCOUNT to see how many you got:

-- get missing IDs
SELECT sa.Customer_Shipping_ID AS ID
INTO #Missing_Ref
FROM staging.Sale sa
WHERE NOT EXISTS (
        SELECT cu.Customer_Shipping_ID
        FROM staging.Customer cu
        WHERE LTRIM(RTRIM(sa.Customer_Shipping_ID)) = LTRIM(RTRIM(cu.Customer_Shipping_ID)))

-- only continue if we got at least 1 missing ID
if @@rowcount > 0
begin

    DECLARE @Current_ID VARCHAR(50);
    DECLARE @Missing_ID VARCHAR(MAX) = '';
    DECLARE @Output_Error VARCHAR(MAX);

    DECLARE id_cursor CURSOR FOR
    SELECT ID
    FROM #Missing_Ref;

    -- rest of cursor here (presumably)
end

BTW, the title of your question seems to have nothing to do with the actual code, which is concerned with finding a set of rows that may or may not exist.


Flatten a multilevel linked list Given such a structure the output should be

10->5->12->7->11->4->20->13->17->6->2->16->9->8->3->19->15.

  Given a linked list where in addition to the next pointer, each node
  has a child pointer, which may or may not point to a separate list.
  These child lists may have one or more children of their own, and so
  on, to produce a multilevel data structure, as shown in below
  figure.You are given the head of the first level of the list. Flatten
  the list so that all the nodes appear in a single-level linked list.
  You need to flatten the list in way that all nodes at first level
  should come first, then nodes of second level, and so on.

This question is attributed to GeekForGeeks. Looking for code-review, optimizations and best practices. Verifying complexity to be \$O(n)\$, where \$n\$ is the count of all the nodes in the input multilevel linked list.

class Nodes<T> {
    Nodes<T> right;
    T item;
    Nodes<T> down;

    Nodes(T item) {
        this.item = item;
    }
}

public final class FlattenMultiLevelList<T> {

    public static <T> List<T> flatten (Nodes<T> node) {
        final List<T> flattenedList = new ArrayList<>();
        final Queue<Nodes<T>> nodeHead = new LinkedList<>();
        nodeHead.add(node);

        // travese the all heads
        while (!nodeHead.isEmpty()) {
            Nodes<T> currNode = nodeHead.poll();
            // traverse all Linkedlists
            while (currNode != null) {
                if (currNode.down != null) { nodeHead.add(currNode.down);  }

                flattenedList.add(currNode.item);
                currNode = currNode.right;
            }
        }

        return flattenedList;
    }
}

public class FlattenMultiLevelListTest {

    @Test
    public void test() {
        // level 1 should contain only a single linkedlist.
        Nodes<Integer> node1_1_1 = new Nodes<>(10);
        Nodes<Integer> node1_1_2 = new Nodes<>(5);
        Nodes<Integer> node1_1_3 = new Nodes<>(12);
        Nodes<Integer> node14 = new Nodes<>(7);
        Nodes<Integer> node15 = new Nodes<>(11);

        node1_1_1.right = node1_1_2;
        node1_1_2.right = node1_1_3;
        node1_1_3.right = node14;
        node14.right = node15;

        // level 2, first linkedlist, first node
        Nodes<Integer> node2_1_1 = new Nodes<>(4);
        Nodes<Integer> node2_1_2 = new Nodes<>(20);
        Nodes<Integer> node2_1_3 = new Nodes<>(13);

        node2_1_1.right = node2_1_2;
        node2_1_2.right = node2_1_3;
        node1_1_1.down = node2_1_1;

        // level 2, second linkedlist, first node.
        Nodes<Integer> node2_2_1 = new Nodes<>(17);
        Nodes<Integer> node2_2_2 = new Nodes<>(6);
        node2_2_1.right = node2_2_2;
        node1_1_3.down = node2_2_1;

        Nodes<Integer> node3_1_1 = new Nodes<>(2);
        node2_1_2.down = node3_1_1;

        Nodes<Integer> node3_2_1 = new Nodes<>(16);
        node2_1_3.down = node3_2_1;

        Nodes<Integer> node3_3_1 = new Nodes<>(9);
        Nodes<Integer> node3_3_2 = new Nodes<>(8);
        node3_3_1.right = node3_3_2;
        node2_2_1.down = node3_3_1;

        Nodes<Integer> node4_1_1 = new Nodes<>(3);
        node3_2_1.down = node4_1_1;

        Nodes<Integer> node4_2_1 = new Nodes<>(19);
        Nodes<Integer> node4_2_2 = new Nodes<>(15);
        node4_2_1.right = node4_2_2;
        node3_3_1.down = node4_2_1;

        assertEquals(Arrays.asList(10,5,12,7,11,4,20,13,17,6,2,16,9,8,3,19,15), FlattenMultiLevelList.flatten(node1_1_1));
    }

}

What you have here is a right-only in order traversal of your "tree". This means you will have to FIFO-Queue the discovered down nodes to continue from, when you can't traverse right anymore.

Your code nicely reflects this, but I have some comments, especially concerning style and naming:

if (currNode.down != null) { nodeHead.add(currNode.down);  }

IMO you put too much in this line at once, especially when your currNode cannot be nicely read out loud.

I do this as a sanity check for my variable names:
read them out loud, when you can't the name is gruesome,
when it sounds silly, the name is bad,
and only when you can read your complete variable name, without sounding Czech, your names are good.

In accordance to that "policy" I'd rename currNode to currentNode or even better traversalNode.

Something similar goes for your nodeHead. I set as a goal for myself:

  If anybody (not even programmer) can read the name, and instantly tell what the variable, class, interface [...] does, it's good. If a programmer can read the name and instantly tell, it's acceptable, all other names are subject to renaming.

If you hadn't provided the nice graphic, I'd have no clue what nodeHead does.
I'd rename to remainingTraversalHeads or something similar.

Additionally to already made points I personally prefer having blocks separate.
The code from above becomes:

if (traversalNode.down != null) {
    remainingTraversalHeads.add(traversalNode.down);
}

With just these two renames, a little styling and the comments removed, your flatten method becomes:

public static <T> List<T> flatten(Nodes<T> node) {
    final List<T> flattenedList = new ArrayList<>();
    final Queue<T> remainingTraversalHeads = new LinkedList<>();
    remainingTraversalHeads.add(node);

    while (!remainingTraversalHeads.isEmpty()) {
        Nodes<T> traversalNode = remainingTraversalHeads.poll();
        while (traversalNode != null) {
            if (traversalNode.down != null) {
                 remainingTraversalHeads.add(traversalNode.down);
            }
            flattenedList.add(traversalNode.item);
            traversalNode = traversalNode.right;
        }
    }
    return flattenedList;
}

This code IMO is more obvious in it's intention and behaviour.

If I got it right, then this is in fact \$O(n)\$ complexity.


NLTK language detection code in Python I need to write some code that checks thousands of websites, to determine if they are in English or not.
Below is the source code. Any improvements would be appreciated.

import nltk
import urllib2
import re
import unicodedata

ENGLISH_STOPWORDS = set(nltk.corpus.stopwords.words('english'))
NON_ENGLISH_STOPWORDS = set(nltk.corpus.stopwords.words()) - ENGLISH_STOPWORDS

STOPWORDS_DICT = {lang: set(nltk.corpus.stopwords.words(lang)) for lang in nltk.corpus.stopwords.fileids()}

def get_language(text):
    words = set(nltk.wordpunct_tokenize(text.lower()))
    return max(((lang, len(words & stopwords)) for lang, stopwords in STOPWORDS_DICT.items()), key=lambda x: x[1])[0]

def checkEnglish(text):
    if text is None:
        return 0
    else:
        text = unicode(text, errors='replace')
        text = unicodedata.normalize('NFKD', text).encode('ascii', 'ignore')
        text = text.lower()
    words = set(nltk.wordpunct_tokenize(text))
    if len(words & ENGLISH_STOPWORDS) > len(words & NON_ENGLISH_STOPWORDS):
        return 1
    else:
        return 0

def getPage(url):
    if not url.startswith("http://"):
        url = "http://" + url
    print "Checking the site ", url
    req = urllib2.Request(url)
    try:
        response = urllib2.urlopen(req)
        rstPage = response.read()
    except urllib2.HTTPError, e:
        rstPage = None
    except urllib2.URLError, e:
        rstPage = None
    except Exception, e:
        rstPage = None
    return rstPage

def getPtag(webPage):
    if webPage is None:
        return None
    else:
        rst = re.search(r'<p\W*(.+)\W*</p>', webPage)
        if rst is not None:
            return rst.group(1)
        else:
            return rst

def getDescription(webPage):
    if webPage is None:
        return None
    else:
        des = re.search(r'<meta\s+.+\"[Dd]escription\"\s+content=\"(.+)\"\s*/*>', webPage)
        if des is not None:
            return des.group(1)
        else:
            return des

def checking(url):
    pageText = getPage(url)
    if pageText is not None:
        if checkEnglish(getDescription(pageText)) == 1:
            return '1'
        elif checkEnglish(getPtag(pageText)) == 1:
            return '1'
        elif checkEnglish(pageText) == 1:
            return '1'
        else:
            return '0'
    else:
        return 'NULL'

if __name__ == "__main__":
    f = open('sample_domain_list.txt').readlines()
    s = open('newestResult.txt', "w")
    for line in f[:20]:
        url = line.split(',')[1][1:-1]
        check = checking(url)
        s.write(url + ',' + check)
        s.write('\n')
        print check

#    f.close()
    s.close()

Some of your functions behave a bit unconventionally.

checkEnglish() only returns 1 or 0.  It would be clearer to return True or False, and rename the function to isEnglish().

getPage() won't allow retrieval of HTTPS URLs.  I would avoid trying to be "helpful" by automatically mangling the URL, unless such mangling was really smart and thorough (as good as the intelligence in your browser's address bar).  By the way, URI schemes are case insensitive (RFC 3986 Sec 3.1).

In getPage(), you swallow exceptions.  That's not good practice, but if you're going to do it, do it succinctly:

def getPage(url):
    print "Checking the site ", url
    req = urllib2.Request(url)
    try:
        response = urllib2.urlopen(req)
        return response.read()
    except:
        return None

In getPtag() and getDescription(), avoid nesting:

def getPtag(webPage):
    if webPage is None:
        return None
    match = re.search(r'<p\W*(.+)\W*</p>', webPage)
    if not match:
        return None
    return match.group(1)

In general, HTML is case insensitive, so use case-insensitive regular expression matching (or an HTML parser).

Your checking() function could use some improvement:

Returning strings '1', '0', or 'NULL' is really weird.  Returning True, False, or None would make more sense.
The name of the function is weird.  It should be something like isEnglishUrl().
Prefer to return early, and express the cascade more simply.

def isEnglishUrl(url):
    pageText = getPage(url)
    if pageText is None:
        return None
    return isEnglish(getDescription(pageText)) or \
           isEnglish(getPtag(pageText)) or \
           isEnglish(pageText)

If getPage() hadn't swallowed exceptions in the first place, then isEnglishUrl() wouldn't have to deal with that pesky if pageText is None.  Instead, it could just let the exception propagate and let its caller deal with it, for more flexibility with less code.

Since Python 2.5, the preferred way to open and close files is using a with block:

RESULT_STR = { True: '1', False: '0', None: 'NULL' }
with open('sample_domain_list.txt') as f:
    with open('newestResult.txt', 'w') as s:
        for line in f.readlines()[:20]:
            url = line.split(',')[1][1:-1]
            eng = isEnglishUrl(url)
            s.write("%s,%s\n" % (url, RESULT_STR[eng]))


Split string by substring without using String.Split() The Aim

Since I very rarely write code to do things other than business logic anymore I've set myself a challenge:

Split a string into a list of substrings by splitting on a matching string, without using string.Split() or similar methods of string (substring, indexof, etc) or LINQ methods (edit or regex /edit).

The method must also accept a parameter stating whether the return list is allowed to contain empty strings (only excludes string.Empty, not whitespace).

The Code

private static IList<string> SplitString(string toSplit, string splitOn, bool allowEmptyResults = true)
{
    int cursorPosition = 0;
    int innerCursor = 0;
    var strings = new List<string>();

    while (cursorPosition < toSplit.Length)
    {
        bool match = false;

        for (int i = 0; i < splitOn.Length ; i++)
        {
            match = toSplit[innerCursor] == splitOn[i];

            if ((i < splitOn.Length - 1 && innerCursor == toSplit.Length - 1) || !match)
            {
                match = false;
                break;
            }

            innerCursor++;
        }

        if (match)
        {
            var result = StringFromCursor(toSplit, cursorPosition, innerCursor - splitOn.Length);

            if (result != string.Empty || allowEmptyResults)
            {
                strings.Add(result);
            }

            cursorPosition = innerCursor;
        }
        else
        {
            if (innerCursor < toSplit.Length - 1)
            {
                innerCursor++;
            }
            else
            {
                var result = StringFromCursor(toSplit, cursorPosition, toSplit.Length);

                if (result != string.Empty || allowEmptyResults)
                {
                    strings.Add(result);
                }

                break;
            }
        }
    }

    return strings;
}

private static string StringFromCursor(string toSplit, int cursorPosition, int matchStart)
{
    string result = string.Empty;

    for (int i = cursorPosition; i < matchStart; i++)
    {
        result += toSplit[i].ToString();
    }

    return result;
}

The Problem

This seems hopelessly complicated and prone to "off by 1" errors, I'm more interested in the cleaner logic for accomplishing this task rather than making it cleaner by refactoring into smaller methods.

Also I'm not too concerned about style (inconsistent use of var for example).

The code spends a large number of characters trying to deal with tricky behaviour at the end of a string but seems to work for the following test cases:

var result = SplitString("thetgonceupon the time there was a thing there", "the");
var result = SplitString("thetgonceupon the time there was a thing thereth", "the");
var result = SplitString("thetgonceupon the time there was a thing therethe", "the");

private static string StringFromCursor(string toSplit, int cursorPosition, int matchStart)
{
    string result = string.Empty;

    for (int i = cursorPosition; i < matchStart; i++)
    {
        result += toSplit[i].ToString();
    }

    return result;
}

This is a bad idea. Since C# string is immutable, the only way to support an append or += operation is to throw the old string away and build a new one.  Doing this repeatedly leads to an \$O(n^2)\$ running time. What you really need is a StringBuilder which does support building up strings piece by piece.


Create table that lists worksheet visibility Following my previous question Create a table that lists macros in a workbook or worksheet here's my Sub to determine worksheet visibility in a workbook. This arises from updating code that used extremely hard to understand logic and several disparate NamedRanges to subsequently hide/reveal sheets.

Is there a better/optimal to create a string as opposed to what I use: join(Array(param1, param2,...,paramN), DELIMIT)? I only have brief exposure to StringBuilder Class and would like to know how best to do this.

Public Sub ListWorksheetVisibilityInActiveWORKBOOK()
Const DELIMIT As String = "|", COLSPAN As Long = 2
Dim HEADER As String
Dim inputCell As Range
Dim Rw As Long, Col As Long
Dim Ws As Worksheet
Dim ASU As Boolean

Dim TableName As String

    HEADER = join(Array("SheetName", "Visibility"), DELIMIT)
On Error Resume Next 'Error handling to allow for cancelation
    Set inputCell = GetInputCell("Select where you want the table to go")
    If inputCell Is Nothing Then GoTo CleanExit
On Error GoTo 0 'Clear error handling

    TableName = Application.InputBox("Table name", Default:="WorksheetVisibility")
    If TableName = "False" Then
        MsgBox "Table name not entered. No table has been created."
        GoTo CleanExit
    End If

    'Check to avoid overwriting information below
Dim tblVisibility As Range, rngFormulas As Range, rngConstants As Range
    Set tblVisibility = inputCell.Resize(ActiveWorkbook.Worksheets.count + 1, COLSPAN)
On Error Resume Next 'If no cells are found error wont cause issue
    Set rngConstants = tblVisibility.SpecialCells(xlCellTypeConstants)
    Set rngFormulas = tblVisibility.SpecialCells(xlCellTypeFormulas)
On Error GoTo 0 'Clears error handling
    If Not rngConstants Is Nothing Or Not rngFormulas Is Nothing Then
Dim Msg As String
        Msg = "Some cells below will be overwritten. Overwrites cannot be undone..." & vbNewLine & vbNewLine & "Do you wish to proceed?"
        If MsgBox(Msg, vbYesNo + vbCritical, "Your attention please!") = vbNo Then End
    End If

ASU = Application.ScreenUpdating
Application.ScreenUpdating = False

    inputCell.Value2 = HEADER

    Rw = inputCell.row + 1
    Col = inputCell.Column
Dim Value As String
    For Each Ws In ActiveWorkbook.Worksheets
        Value = join(Array(Ws.Name, Ws.Visible), DELIMIT)
        Cells(Rw, Col).Value2 = Value
        Rw = Rw + 1
    Next

    tblVisibility.Columns(1).TextToColumns DataType:=xlDelimited, Other:=True, OtherChar:=DELIMIT

    ActiveSheet.ListObjects.Add(xlSrcRange, tblVisibility, XlListObjectHasHeaders:=XlYesNoGuess.xlYes, Destination:=inputCell).Name = TableName

CleanExit:
Application.ScreenUpdating = ASU
End Sub

Private Function GetInputCell(ByVal Prompt As String) As Range
    On Error GoTo ErrorHandler
    Set GetInputCell = Application.InputBox(Prompt, Type:=8)
    Exit Function
ErrorHandler:
    MsgBox "User Cancelled"
    Set GetInputCell = Nothing
End Function

I don't think there is a StringBuilder() class in VBA, only some tricks using Mid.

Const DELIMIT As String = "|", COLSPAN As Long = 2
Dim HEADER As String

This is a little confusing, UPPERCASE should indicate a constant, which is does with DELIMIT - but Header is not (cannot) be a constant. And that leaves me without a Dim or a Const for COLSPAN. Try to be a little more consistent with that - it will be much easier to tell what variables are what.

Dim ASU as Boolean
ASU = Application.ScreenUpdating
Application.ScreenUpdating = False
Application.ScreenUpdating = ASU

Now, I know ASU can't be a constant. Maybe screenIsUpdating? But then again, I think using a variable to store this is overkill unless you are trying to save the settings of the user - which you aren't

Dim screenIsUpdating as Boolean
screenIsUpdating = Application.ScreenUpdating
Application.ScreenUpdating = False
Application.ScreenUpdating = screenIsUpdating

This way you store the user's settings, but still turn it off for your procedure.

These variables could use better names, even if i and j -

Dim Rw As Long, Col As Long
Dim Ws As Worksheet

Ws works, but I don't recommend it, it will start to look pretty messy once you have a lot going on. Also, local variables should start with a lowercase letter Standard VBA naming conventions.

Dim tblVisibility As Range, rngFormulas As Range, rngConstants As Range

I see tblVisibility and think "oh, must be a boolean" - but it's a range. And rngFormulas and rngConstants seem to have the same issue, which is why they are prefixed with rng - yeah?

tableRange
formulaRange
constantRange

But, what is constantRange? If it's constant, it doesn't need a range.

Cells(Rw, Col).Value2 = Value

You did a good job qualifying most things, but this Cells isn't qualified - it should be inputCell.Parent.Cells - or just give that target sheet a variable.

If MsgBox(Msg, vbYesNo + vbCritical, "Your attention please!") = vbNo Then End

Here's an End again, try to avoid those. Also I think Msg (as well as some other fixed strings) could be a Const.

Rw = inputCell.Row + 1
Col = inputCell.Column
Dim Value As String
For Each Ws In ActiveWorkbook.Worksheets
    Value = Join(Array(Ws.Name, Ws.Visible), DELIMIT)
    Cells(Rw, Col).Value2 = Value
    Rw = Rw + 1
Next

This loop is pretty confusing to me. You are iterating up the rows, but have a loop for the sheets?

For index = 1 to Thisworkbook.Worksheets.Count
   targetSheet.Cells(index+1,tableColumn) = Join(Array(Worksheets(index).Name,Worksheets.Visible), DELIMITER)
Next

But, for that Join string, I would probably do it a different way -

Dim index As Long
Dim tableArray() As String
Dim sheetCount As Long
sheetCount = ThisWorkbook.Worksheets.Count
ReDim tableArray(1 To sheetCount, 1 To 2)
For index = LBound(tableArray) To UBound(tableArray)
    tableArray(index, 1) = ThisWorkbook.Worksheets(index).Name
    tableArray(index, 2) = ThisWorkbook.Worksheets(index).Visible
Next

Arrays are faster and you can just Transpose it into your table range. Or maybe just convert the array into a table.

Oh, and your procedure name

Public Sub ListWorksheetVisibilityInActiveWORKBOOK()

Good job on being descriptive, but it's a bit much. CreateSheetVisibilityTable() maybe?


Permutations of a string Today I had an interview, where I was asked to solve this question:

Find all the permutations of a given string
Example:
str = 'abc'
Answer: 'abc', 'acb', 'bac', 'bca', 'cab', 'cba'

I have written this code, but can it be improved or is there any other way to do it in better manner?
   public class Permutation{
     public static void main(String []args){
        Permutation p = new Permutation();
        String str = "1234";
        int n = str.length();
        p.permute(str, 0 ,n-1);

     }

     public static void permute(String str, int l, int r){
         if(l==r){
             System.out.println(str);
         }

         for(int i=l;i<=r;i++){
             str = swap(str,i,l);
             permute(str, l+1, r);
             str = swap(str,i,l);
         }
     }

     public static String swap(String str, int i, int l){
             char[] charArr = str.toCharArray();
             char t = charArr[i];
             charArr[i] = charArr[l];
             charArr[l] = t;
             return (String.valueOf(charArr));
         }

}

Calling static methods

   public class Permutation{

Permutation doesn't really fit.  This isn't a permutation.  It's something that creates permutations.  Consider names like Permuter or PermutationUtils.

        Permutation p = new Permutation();
        String str = "1234";
        int n = str.length();
        p.permute(str, 0 ,n-1);

     }

     public static void permute(String str, int l, int r){

You don't need to call static methods from an object.  This could be just

        String str = "1234";
        int n = str.length();
        Permutation.permute(str, 0, n - 1);
     }

     public static void permute(String str, int l, int r) {

There's no need to instantiate an object.

Beyond that, I'd actually prefer

         PermutationUtils.permute("1234");
     }

     public static void permute(String str) {
         permute(str, 0, str.length() - 1);
     }

     private static void permute(String str, int l, int r) {

The caller shouldn't have to know details about how the method is called in my opinion.

Instantiating and using an object

class Permuter {

    private final String original;
    private final List<String> permutations = new ArrayList<>();

    public Permuter(String original) {
        this.original = original;
    }

    public List<String> permute() {
        if (permutations.empty()) {
            permute(original, 0, original.length() - 1);
        }

        return permutations;
    }

That's an example of how the class could be defined if it was meant to be instantiated.  Then you could call it like

    public static void main(String[] args) {
        Permuter p = new Permuter("1234");
        for (String permutation : p.permute()) {
            System.out.println(permutation);
        }
    }

This also returns the permutations and only displays them in the main method.  This is generally a better practice.  This way if what they really wanted was the permutations, you don't have to rewrite the whole code to give it to them.

You may want to consider using a longer variable name than p.  It's OK here, as it's only used on adjacent lines.  But in a longer method, single letter variables names can get confusing.

Don't do unnecessary work

         for(int i=l;i<=r;i++){
             str = swap(str,i,l);
             permute(str, l+1, r);
             str = swap(str,i,l);
         }

This pattern is what you would use if str were a mutable, like a StringBuilder or array.  Since swap creates a new string, you could just say

         for (int i = l; i <= r; i++) {
             permute(swap(str, i, l), l + 1, r);
         }

It's not necessary to call swap twice, since it doesn't change the original string.  This way str never changes.

I also changed the spacing.  It's easier to see where the variables start and end if there are spaces around the operators.

If efficiency matters

If efficiency matters, consider using a mutable instead.  This version creates a new string every time swap is called.  With a mutable, it could create a new string only when it needed to add to the list or display.


Create better Base DAO class I created a DAO class which is base class for all other DAO classes.

I use Spring Framework 4 and Hibernate 4.

Question: Is there anything that could be done better?

public class GenericDaoImpl<E, I extends Serializable> implements GenericDao<E, I> {
    private Class<E> entityClass;

    @Autowired
    private SessionFactory sessionFactory;

    public GenericDaoImpl(Class<E> entityClass) {
        this.entityClass = entityClass;
    }

    @Override
    public Session getCurrentSession() {
        return sessionFactory.getCurrentSession();
    }

    @Override
    @SuppressWarnings("unchecked")
    public List<E> findAll() throws DataAccessException {
        return getCurrentSession().createCriteria(entityClass).list();
    }

    @Override
    @SuppressWarnings("unchecked")
    public E find(I id) {
        return (E) getCurrentSession().get(entityClass, id);
    }

    @Override
    public void create(E e) {
        getCurrentSession().save(e);
    }

    @Override
    public void update(E e)  {
        getCurrentSession().update(e);
    }

    @Override
    public void delete(E e) {
        getCurrentSession().delete(e);
    }

    @Override
    public void flush() {
        getCurrentSession().flush();
    }
}

Sample DAO class which extends base DAO class is for example this one:

@Repository("articleDao")
public class ArticleDaoImpl extends GenericDaoImpl<ArticleEntity, Long> implements ArticleDao {
    public ArticleDaoImpl(){
        super(ArticleEntity.class);
    }
}

To be able to create a parameter-less constructor add the following code into your constructor. It will use reflection to set entityClass. This way you don't even need to worry about passing in a class type, you can extend out your generic DAO and its type will be set by parameterisation.

public GenericDaoImpl() {
    Type e = getClass().getGenericSuperclass();
    ParameterizedType pt = (ParameterizedType) e;
    entityClass = (Class<E>) pt.getActualTypeArguments()[0];
}

This removes the need for having constructors in subclasses of your GenericDAOImpl.

Why don't you combine your create and update methods? It makes for easier service calls.

@Override
public void saveOrUpdate(E e){
    getCurrentSession().saveOrUpdate(e);
}

Hibernate will automatically make the determination whether a save or update call is appropriate. If your ID field is set, then perform an update, otherwise perform a save.

Also, I would remove getCurrentSession() from your interface and change the method to private. You shouldn't be accessing the Hibernate session from anywhere outside the DAO so there is no reason to expose out your getCurrentSession() method.


Central ajax function in jquery I have created an central AJAX function (using JQuery) which will be responsible for handling all the AJAX calls in my project.

var ajaxCall = function(_url, _type, _data, _successCallback, _errorCallback, isAsync, _completeCallback){

    // Handling _url
    if(!_url.trim()) throw new Error('Sucess callback is not defined');

    // Handling _type
    if(!_type.trim()) throw new Error('TYPE is not defined');

    // Handling _successCallback
    if(!_successCallback || typeof _successCallback !== "function") throw new Error('Success callback is not defined');

    // Handling _errorCallback
    if(!_errorCallback || typeof _errorCallback !== "function") _errorCallback = function() {};

    // Handling _completeCallback
    if(!_completeCallback || typeof _completeCallback !== "function") _completeCallback = function() {};

    // AJAX configuration object
    var ajaxObj = {
        url : _url.trim(),
        contentType : "application/json",
        type : _type.trim(),
        data : _data || {},
        async: !!isAsync,
        success : function(data) {
            try {
                _successCallback(data);
            } catch (exception) {
                global.logger.i(TAG, 'Exception! ' + exception);
            }
        },
        error : _errorCallback,
        complete: _completeCallback
    };

    // Capturing AJAX call
    return $.ajax(ajaxObj);

};

Let me know if this is a valid way to centralize the AJAX calls.

Central ajax function in jquery

No. Just, no. It's tempting to handle how all AJAX operations are called in your app, but this is usually just for a handful of use cases. I've seen code time and again do this only to actually call $.ajax directly elsewhere because the "global" one just doesn't serve its needs.

var ajaxCall = function(_url, _type, _data, _successCallback, _errorCallback, isAsync, _completeCallback){

jQuery AJAX methods return a promise-like object. Use the standard promise interface to assign callbacks instead of using the old callback method. That means, use the then method. Using this also makes your code easily portable to standard APIs like fetch.

Additionally, there's no sense adding _ before variable names. It only adds noise. If you mean to avoid collisions, just use different variable names. Don't make it more complicated than it already is.

// Handling _url
if(!_url.trim()) throw new Error('Sucess callback is not defined');

// Handling _type
if(!_type.trim()) throw new Error('TYPE is not defined');

// Handling _successCallback
if(!_successCallback || typeof _successCallback !== "function") throw new Error('Success callback is not defined');

// Handling _errorCallback
if(!_errorCallback || typeof _errorCallback !== "function") _errorCallback = function() {};

// Handling _completeCallback
if(!_completeCallback || typeof _completeCallback !== "function") _completeCallback = function() {};

I also wouldn't do these param checks and preprocessing if I were you. A function should assume the arguments are correct. That way, it fails on bad input. It's also the duty of the consuming code to use the function correctly. Don't make it just catch the other code's fault. Let the function fail on bad input, let the browser throw an error or do a bad request. A visible error on the network or console is a bazillion times better than some function swallowing errors.

async: isAsync !== false ? true : false,

Synchronous XHR should never be an option. It freezes the UI and that's bad for UX. Learn how to write and deal with asynchronous code. Don't work around it.

try {
  _successCallback(data);
} catch (exception) {
  global.logger.i(TAG, 'Exception! ' + exception);
}

Lastly, your function should not be concerned about the callback. It's none of its concern. Let the callback handle its own errors. Also, globally swallowing errors is a bad idea. Errors are harder to trace. ANd if your logger is disabled, you will never know the callback failed.

In the end, the call you aim to globalize is nothing more than just the following. $.get is an explicit shorthand for $.ajax({ method: 'get' }) ($.ajax defaults to GET but you'd have to know jQuery by heart to know that). type can be omitted if your server returns the correct content type response headers for JSON so jQuery knows what to do with it.

$.get({
  contentType: 'application/json',
  data: { ... },
  url: '...'
}).then(function(res){
  // AJAX succeeded
}, function(err){
  // AJAX failed
});


Button presses counter I'm moving on to graphics, picking Pyglet and Cocos2D. This is my first program in Python with GUI.

Introducing:

Two distinct high definition button sprites
Satisfying high quality button pressing clicks
Conveniently positioned counter
Fully functional exit button
Detailed error logging techniques to support software users
Nice hand-picked background color

I would be very happy if you would correct my first steps to game programming.

Files

""" Button presses counter. """

import cocos
import pyglet

class App (cocos.layer.ColorLayer):
    """ A class holding an App layer, the only layer of the program. """
    is_event_handler = True

    def __init__(self):
        """ Initializes the App layer. """
        CENTER = 320, 240 # Center point constant. No >80 symbols and multilines.
        super(App, self).__init__(135, 206, 235, 255) # skyblue background color

        # Counter
        self.text = cocos.text.Label('0',
                                     anchor_x = 'center',
                                     anchor_y = 'center',
                                     position = CENTER)
        self.add(self.text, z = 1)

        # Button stuff
        self.b_released = True # Button state
        # Button sprites
        try:
            self.pressed = cocos.sprite.Sprite('pressed.png', position = CENTER)
            self.released = cocos.sprite.Sprite('released.png', position = CENTER)
            self.add(self.released) # When button state changes, sprites switch
        except Exception:
            with open('errlog.txt', 'a+') as f:
                f.write("Problems w/ 'pressed.png' & 'released.png'. ")
                f.write("Most likely do not exist or are corrupted.\n")
        # Sounds
        try:
            self.p_sound = pyglet.resource.media('press.wav',streaming=False)
            self.r_sound = pyglet.resource.media('release.wav',streaming=False)
        except Exception:
            with open('errlog.txt', 'a+') as f:
                f.write("Problems w/ 'press.wav' & 'release.wav'. ")
                f.write("Most likely do not exist or are corrupted.\n")

    def update_counter(self):
        """ Increases counter, positions it on center. """
        self.text.element.text = str ( int(self.text.element.text)+1 )
        self.text.element.position = 320, 240

    def on_mouse_press(self, x, y, buttons, modifiers):
        """ Changes state, sprite, plays sound when button is pressed. """
        if x > 241 and x < 399 and y > 161 and y < 319 \
        and self.b_released and buttons & pyglet.window.mouse.LEFT:
            self.b_released = False
            self.add(self.pressed)
            self.remove(self.released)
            self.p_sound.play()
            self.update_counter()

    def on_mouse_release(self, x, y, buttons, modifiers):
        """ Changes state, sprite, plays sound when button is released. """
        if not self.b_released:
            self.b_released = True
            self.add(self.released)
            self.remove(self.pressed)
            self.r_sound.play()

cocos.director.director.init()
cocos.director.director.run( cocos.scene.Scene( App() ) )

You defined a CENTER constant. Could you not do the same with SKY_BLUE, instead of the comment?

super().__init__(*SKY_BLUE)

def update_counter(self):
    self.text.element.text = str ( int(self.text.element.text)+1 )
    self.text.element.position = 320, 240

Using the text label as your data model is cumbersome. Keep an internal count instead of parsing the count back out of the UI.

Do you really need to re-center the label after each change? If so, explain why in a comment, and reuse a CENTER constant.

def on_mouse_press(self, x, y, buttons, modifiers):
    if x > 241 and x < 399 and y > 161 and y < 319 \
    and self.b_released and buttons & pyglet.window.mouse.LEFT:

Python supports double-ended inequalities like 241 < x < 399. I'm having a hard time understanding why the hot zone is defined with those particular bounds.  I'm guessing that you are hard-coding some coordinates that should be automatically determined.


Optimizing Project Euler 36 - double-base palindromes This one was quick and simple. Any optimizations?

from timeit import default_timer as timer

def sum_double_base_palindromes_under(limit):
    def is_palindrome(n):
        if str(n) == str(n)[::-1]:
            return True
        return False
    sum = 0
    for n in range(limit):
        if is_palindrome(n) and is_palindrome(int(bin(n)[2:])):
            sum += n
    return sum

start = timer()
ans = sum_double_base_palindromes_under(1000000)
elapsed_time = (timer() - start) * 1000 # s --> ms

print "Found %d in %r ms." % (ans, elapsed_time)

In any programming language, I'd consider (assuming c is a condition with a boolean value):

if c:
    return True
else:
    return False

or it's equivalent :

if c:
    return True
return False

to be an anti-pattern that should be written:

return c

Your for loop can easily be rewritten into a more concise (and more efficient according to my benchmarks) way using sum :

return sum(n for n in range(limit) if is_palindrome(n) and is_palindrome(int(bin(n)[2:])))

An easy optimisation is not to compute str(n) more than needed :

def is_palindrome(n):
    s = str(n)
    return s == s[::-1]

Also, it might even be faster to feed strings to your functions :

def is_palindrome(s):
    return s == s[::-1]
return sum(n for n in range(limit) if is_palindrome(str(n)) and is_palindrome(bin(n)[2:]))

On my machine, running your function 10 times, the running time went from 470 ms to 310 ms per iterations.


What are the top voted comments on closed questions, and who made the comments? Following this linked question and the feedback it generated, I put together a new query which utilizes a JOIN.

WITH UpvotedComments AS (
  SELECT
    PostId,
    Score,
    Text,
    --Comments.UserDisplayName doesn't return users' current display names
    Commenter = (
      SELECT DisplayName
      FROM Users
      WHERE Id = UserId
    )
  FROM Comments
  WHERE Score > 0
), ClosedPosts AS (
  SELECT Id, ClosedDate
  FROM Posts
  WHERE ClosedDate IS NOT NULL
)

SELECT Score, Text, Commenter

FROM UpvotedComments

INNER JOIN ClosedPosts ON UpvotedComments.PostId = ClosedPosts.Id

ORDER BY Score DESC, ClosedDate DESC

It can get a bit slow on sites like Stack Overflow, where the returns are greater than 50 000.

SEDE has two great features that you should incorporate into your code:

Input parameters
Magic markup columns

Input parameters
SEDE lets you declare fields to be input towards the query, like so:
##Parameter:type?defaultvalue##

And so, I suggest you use three of them for the following things:

Threshold (The threshold of comment scores to select)
Amount per page (The amount per page of comments)
Page number (To go with the above one)

By using pages, you can simplify your returning dataset and help to reduce the massive call times of the query.

Magic markup columns
SEDE has a great feature where certain columns can be transformed into special markup. The specific columns are:

Post Link (generated from Post Id)
User Link (generated from User Id)
Comment Link (generated from Comment Id)
Suggested Edit Link (generated from Suggested Edit Id)
Tags

As it stands...
your code is good, but you are doing a few things you can improve on:

Your generation of Commenter can be easily replaced with UserId as [User Link] to save extraneous calling of another table.
SELECT Id, ClosedDate your SELECTion is redundant. To use the field, you don't actually have to select it, simply reference it. Seeing as the resulting table is only used for the Id anyway, it's pointless to have it.


Reverse linked list using recursion Here is my code to reverse link list in using recursion:

       Node n = null;

    private void ReverseList(Node temp)
    {
        if (temp == null)
            return;

        ReverseList(temp.nextNode);

        Node newNode = temp;
        newNode.nextNode = null;
        if (n == null)
        {
            n = newNode;
        }
        else
        {

            Node TempNode = n;

            while (TempNode.nextNode != null)
            {
                TempNode = TempNode.nextNode;
            }
            TempNode.nextNode = newNode;
             head=n;
        }

    }

Is this the preferred way of doing this? What modifications can I make to optimize this code?

Do use descriptive name for the variables and parameters, all those n, temp, newNode do not tell anything.

Reversing the list using recursion should not rely on 'external' variables as you do with Node n = null;. Given that linked list after reversal should have the last element at its head, I would suggest to pass it through the recursive chain hierarchy as a return value.

Having a loop while (TempNode.nextNode != null) makes recursion useless, as you walk through the list via loops instead of recursion. Try to come up with the approach where you do not need loops to get the head of the resulting linked list.


Rock, Paper, Scissors, Lizard and Spock with OOP Today I saw an extremely interesting question on the Rock, Paper, Scissors, Lizard and Spock game. I've decided to take my swing at it and I've written the following OOP implementation:

function RockPaperScissors(name, hard_action, soft_action) {
    this.name = (name + '').toLowerCase();
    this.hard_action = hard_action + '';
    this.soft_action = (soft_action || hard_action) + '';
    this.strengths = {};
    this.weaknesses = {};
}

RockPaperScissors.prototype = {
    getName: function(){
        return this.name;
    },
    addStrength: function(strength) {
        if( !(strength instanceof RockPaperScissors) ) {
            throw new TypeError('A strength must be an instance of RockPaperScissors');
        }

        this.strengths[strength.getName()] = strength;
    },
    getStrengths: function() {
        var strengths = {};
        for(var k in this.strengths)
        {
            strengths[k] = this.strengths[k];
        }
        return strengths;
    },
    addWeakness: function(weakness, soft) {
        if( !(weakness instanceof RockPaperScissors) ) {
            throw new TypeError('A weakness must be an instance of RockPaperScissors');
        }

        this.weaknesses[weakness.getName()] = {
            enemy: weakness,
            soft: !!soft
        };
    },
    getWeaknesses: function() {
        var weaknesses = {};
        for(var k in this.weaknesses)
        {
            weaknesses[k] = this.weaknesses[k];
        }
        return weaknesses;
    },
    getAction(soft){
        return soft? this.soft_action : this.hard_action;
    },
    winsTo: function(enemy) {
        if( !(enemy instanceof RockPaperScissors) ) {
            throw new TypeError('An enemy must be an instance of RockPaperScissors');
        }

        return (enemy.getName() in this.strengths);
    }
};

function RockPaperScissorsGame(){
    this.score = {
        won: 0,
        lost: 0,
        tied: 0
    };
    this.games = 0;
}

RockPaperScissorsGame.prototype = {
    match: function(user, enemy){
        if( !(user instanceof RockPaperScissors) ) {
            throw new TypeError('The user must be an instance of RockPaperScissors');
        }
        if( !(enemy instanceof RockPaperScissors) ) {
            throw new TypeError('The enemy must be an instance of RockPaperScissors');
        }

        this.games++;

        if( user == enemy ) {
            this.score.tied++;
            return 'Tie';
        }

        var userName = user.getName();
        var enemyName = enemy.getName();

        if( user.winsTo(enemy) ) {
            this.score.won++;
            var enemyWeakness = enemy.getWeaknesses()[userName] || {enemy: enemy, soft: false};

            return userName + ' ' + user.getAction(enemyWeakness.soft) + ' ' + enemyName;

        } else {
            this.score.lost++;
            var userWeakness = user.getWeaknesses()[enemyName] || {enemy: enemy, soft: false};

            return enemyName + ' ' + enemy.getAction(userWeakness.soft) + ' ' + userName;
        }
    },
    getMatches: function(){
        return this.games;
    },
    getWins: function(){
        return this.won;
    },
    getLosses: function(){
        return this.lost;
    },
    getTies: function(){
        return this.tied;
    }
};

So far so good. The setup is minimal, allows to extend it to as many as you want (you can add a tornado, a hurricane, a thunder .... anything!).

Here, there's the concept of hard and soft actions. Soft actions may be, for example, the paper wrapping the rock. Hard actions are, for example, the scissor beheading the lizard. You can define if a weakness is soft or hard.

To make it easier, I've also implemented a way to have multiple games and easily start a new one.

It is a good idea, but I'm not 100% happy with its quality. There's something that makes me uneasy.

In terms of overall quality and OOP, what am I doing wrong? What readability improvements can be made?

First of all, you've got a syntax error:

getAction(soft){

should be:

getAction: function (soft) {

Anyway: It's pretty clean, but it a little too... Java'ish, for lack of a better word. Or at least it seems to be adhering too close to practices more applicable to other languages.

You're not using "private" (i.e. closure) variables, so many of the getters and setters aren't necessary. For instance, if I want the name of the user, I'd just write user.name. I wouldn't bother with user.getName().

Related to that, though there's no setter for the name property, that doesn't stop me from just assigning something.

All object properties are essentially public in JavaScript, and trying to avoid that is typically more hassle than it's worth. I find it easier to just roll with it for the most part.

Of course, you may want to keep some things "private", which is where closures come in. However, it's a little outside the scope of just a review, so I'll leave that be.

Point is, without all the getters/setters, you end up with a lot less code. And less code means less potential for bugs. Bugs like this:

getWins: function(){
    return this.won; // always returns undefined
},

The property this.won doesn't exist; you're looking for this.score.won. The same error is repeated for the other getters (except getMatches).

Related to all of this is the object copying you're doing with returning strengths and weaknesses. Again, it's a good idea - but is it worth it? Again, if I want to mess with the internal weaknesses obj, I can just go user.weaknesses... and manipulate the object directly anyway.

The getters only provide safety if I agree to use them. I'd rather agree to tread carefully in return for direct access to properties (which I already have, but still).

There's also a lot of type checking going on, which I often find a little too strict. If you pass an object to, say, winsTo that's duck-typing the correct API, there's no reason it should fail. Conversely, if it's completely incompatible, well, that'll probably manifest itself soon enough anyway. Yeah, I know, that's pretty loose (even lazy!), but this isn't a power plant control system, so...

Often times, a better solution to type-checking is feature detection. Instead of checking an object's ancestry, check whether it supports whatever it is you need:

if(typeof obj.someMethod === "function") {
  // ok, obj supports someMethod, so we can proceed
}

Lastly some style and structural things:

As ErikR points out, the names aren't great. RockPaperScissors is pretty vague. It's more a concept than object.
On that note, RockPaperScissorsGame, while a clearer name, isn't an impartial game. It's more of a scorecard, really - the user's score card, specifically. Values like won and lost are all from the perspective of one of the players. It'd be completely the opposite for the enemy player. Seems iffy to me.
You've got under_scored names like soft_action, but the JS convention is to use camelCase, i.e. softAction.
The something + '' trick isn't pretty. You can just use String(something) to coerce something to a string.
I'm not a fan of the asymmetry in strengths and weaknesses. For instance, if user beats enemy, you have to ask enemy how it happened by looking up the user in the enemy's weaknesses. That seems odd to me. I might prefer a central "table" object of sorts that keep track of what beats what and how.


Reversing an inputted sentence I sometimes hear people say how they can distinguish between good and bad coders of a certain programming language or even programming in general simply by looking at the code they write.

I'm relearning C, and I was just wondering whether my program follows best practices in terms of my commenting, the way I set it up, and generally the way I've incorporated the for and while loops along with macros (whether it be better to use a goto statement here, or a while loop instead of a for loop there, etc).

#include <stdio.h>

#define SIZE (sizeof(words[0])/sizeof(words[0][1]))
#define ROWS 10
#define COLUMNS 20

int main()
{
//The maxmimum letters in a words is 20 and the max words is 10
char words[ROWS][COLUMNS]={0},character;
int count_1=0,count_2=0,i,j;

printf("Enter something:  ");
character=getch();
putch(character);
while(character!='\r')
{
words[count_1][count_2]=character;
//Allows the next index to contain the value of the next inputted character
count_2++;
if(character==' ')
{
//index of inputted characters reset to 0
count_2=0;
//Allows the next row in array to be changed as one row will already satisfy a word
count_1++;
}

character=getch();
putch(character);
}

printf("\n");
//Starts from the last word
for(i=count_1;i>=0;i--)
{
for(j=0;j<SIZE;j++)
{
if(words[i][j]!=0)
printf("%c",words[i][j]);
}
}
}

I know the next bit is a bit opinionated, but does my code give an idea of where I'm at with logic and programming as a whole, or is the first part just some kind of myth (i.e. you can't judge a person by such a small piece of code)?

Note: I deliberately avoided the use of functions and strings. I got this task from C Programming: A Modern Approach, and I'm only attempting to solve a programming task simply by using only the features I've learned thus far in the book.

I sometimes hear people say how they can distinguish between good and bad coders of a certain programming language or even programming in general simply by looking at the code they write.

I think that's true, to some extent. How else (except by the code they write) would you judge a programmer's ability?

Distinguishing programmers from their code is just the beginning though: see this answer for example.

Professional-level programming might also be categorized e.g. with Programmer Competency Matrix

  I was just wondering whether my program follows best practices

Not all the best practices; it could still be improved, for example:

Avoid macros (#define)
Format it well (use indentation)
Give good user output (better than "Enter something:  ")
Use subroutines (don't put everything in main)
Don't overflow your input buffer (e.g. if the end-user enters more than 10 words, or words more than 20 characters long)
Use library methods appropriately (e.g. not printf("%c" when you want to output a whole string and not just one character)
Use good identifier names (e.g. I'd prefer row and col, or x and y, or imax and jmax, instead of count_1 and count_2)
Be flexible in handling input correctly (e.g. I'd prefer arbitrary-length words and arbitrary-many words, which would require you to use a heap-based list or array of variable/infinite-length strings, instead of a fixed-size array)

  I know the next bit is a bit opinionated, but does my code give an idea of where I'm at with logic and programming as a whole

I think so. The code matches where you said you're at, which is that you are "relearning C".

Working a 40-hour week makes about 2000 hours per year.

So an adult professional programmer will have spent many 1000s of hours of practice and studying.


LSTM Model - Validation Accuracy is not changing I am working on classification problem, My input data is labels and output expected data is labels
Labels  Count
1      94481
0      65181
2      60448

I have made X, Y pairs by shifting the X and Y is changed to the categorical value
create X/y pairs
df1 = df['Data_Used']
df1 = concat([df1, df1.shift(1)], axis=1)
df1.dropna(inplace=True)

    X   Y
    2   1.0
    1   2.0
    1   1.0
    2   1.0
    2   2.0

values = df1.values

encoder = LabelEncoder()
test_labels = to_categorical(encoder.fit_transform(values[:,1]),num_classes=3)
train_X,test_X,train_y,test_y= train_test_split(values[:,0], test_labels,test_size = 0.30,random_state = 42)

print(train_X.shape)
print(train_y.shape)
print(test_X.shape)
print(test_y.shape)

(154076,)
(154076, 3)
(66033,)
(66033, 3)
Converting this to LSTM format
train_X = train_X.reshape(train_X.shape[0],1,1)
test_X = test_X.reshape(test_X.shape[0],1,1)

# configure network
n_batch = 1
n_epoch = 10
n_neurons = 100

ModelArchitecture
tf.keras.backend.clear_session()

model = tf.keras.models.Sequential([
    tf.keras.layers.LSTM(n_neurons, batch_input_shape=(n_batch, train_X.shape[1],train_X.shape[2]), stateful=True),
    tf.keras.layers.Dense(64, activation='relu'),
    tf.keras.layers.Dense(100, activation = 'relu',kernel_regularizer=regularizers.l2(0.0001)),
    tf.keras.layers.Dense(3, activation='softmax')
])

model.summary()
model.compile(optimizer='rmsprop',
              loss='categorical_crossentropy',
              metrics=['acc'])
history = model.fit(train_X,train_y,validation_data=(test_X, test_y),epochs=n_epoch, batch_size=n_batch, verbose=1,shuffle= False)

Validation Accuracy is not Changing
Epoch 1/5
154076/154076 [==============================] - 356s 2ms/step - loss: 1.0844 - acc: 0.4269 - val_loss: 1.0814 - val_acc: 0.4310
Epoch 2/5
154076/154076 [==============================] - 354s 2ms/step - loss: 1.0853 - acc: 0.4256 - val_loss: 1.0813 - val_acc: 0.4310
Epoch 3/5
154076/154076 [==============================] - 355s 2ms/step - loss: 1.0861 - acc: 0.4246 - val_loss: 1.0814 - val_acc: 0.4310
Epoch 4/5
154076/154076 [==============================] - 356s 2ms/step - loss: 1.0874 - acc: 0.4228 - val_loss: 1.0825 - val_acc: 0.4310
Epoch 5/5
154076/154076 [==============================] - 353s 2ms/step - loss: 1.0887 - acc: 0.4208 - val_loss: 1.0828 - val_acc: 0.4310

What can be the changes to improve the model.

One possible reason of this could be unbalanced data. You should have same amount of examples per label. And if you don't have that data, you can use Loss Weights. It is a parameter in model.compile(). You can learn more about Loss weights on google.
Also, I noticed you were using rmsprop as the optimizer. Try using Adam optimizer, as it is one of the best optimizer.
If, doing all of these I mentioned above, doesn't changes anything and the results are the same, remove the Dense() Layers and just keep 1 dense() layer, that is, just keep the last Dense Layer, and remove all the other Dense() Layers.
This will surely improve the model. But, if still it doesn't changes anything, then have a look here


Remove the second number from every group of three numbers I have this simple loop that prints numbers from 0 to 29 (30 in total). I want to "split" the results in threes and remove the middle number. So the first set is 0,1,2 and I only want 0,2. The second set is 3,4,5 and I only want 3,5. This works as it supposed too, but is there a way to make it more simpler?

z = 1
for i in range(0,30):
    if z == 1:
        print(i, end=' ')
        z+=1
    elif z == 3:
        print(i, end=' ')
        z=1
    else:
        z+=1

This is the result:

0 2 3 5 6 8 9 11 12 14 15 17 18 20 21 23 24 26 27 29

You want to skip every number of the form 3k+1? Then say that, using a generator expression.

print(' '.join(str(i) for i in range(30) if i % 3 != 1))


Setting up notifications for Android Here is AlarmService for setting up notifications on my phone. Currently, the code works and is functional. However, I noticed my code looks different than a few examples I found online. Should I be using Notification Compact builder instead?

Also, any advice on how to make it better/cleaner would be great.

import android.app.Notification;
import android.app.NotificationManager;
import android.app.PendingIntent;
import android.app.Service;
import android.content.Intent;
import android.os.IBinder;

public class AlarmService extends Service
{

    private NotificationManager mManager;

    @Override
    public IBinder onBind(Intent arg0)
    {
        // TODO Auto-generated method stub
        return null;
    }

    @Override
    public void onCreate()
    {
        // TODO Auto-generated method stub
        super.onCreate();
    }

    @SuppressWarnings("static-access")
    @Override
    public void onStart(Intent intent, int startId)
    {
        super.onStart(intent, startId);

        mManager = (NotificationManager) this.getApplicationContext().getSystemService(this.getApplicationContext().NOTIFICATION_SERVICE);
        Intent intent1 = new Intent(this.getApplicationContext(),ScoreCollectionPager.class);

        Notification notification = new Notification(R.drawable.clock,"Score Collection!", System.currentTimeMillis());
        intent1.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP| Intent.FLAG_ACTIVITY_CLEAR_TOP);

        PendingIntent pendingNotificationIntent = PendingIntent.getActivity( this.getApplicationContext(),0, intent1,PendingIntent.FLAG_UPDATE_CURRENT);
        notification.flags |= Notification.FLAG_AUTO_CANCEL;
        notification.setLatestEventInfo(this.getApplicationContext(), "Score Collection!", "Please enter scores at this time!", pendingNotificationIntent);

        //Default notification alerts to vibrate and make a notification sound
        notification.defaults |= Notification.DEFAULT_VIBRATE;
        notification.defaults |= Notification.DEFAULT_SOUND;

        mManager.notify(0, notification);
    }

    @Override
    public void onDestroy()
    {
        // TODO Auto-generated method stub
        super.onDestroy();
    }

}

I don't know much about Android, so I can't answer that specific question, but there's still some things to change.

 @Override
public IBinder onBind(Intent arg0)
{
    // TODO Auto-generated method stub
    return null;
}

@Override
public void onCreate()
{
    // TODO Auto-generated method stub
    super.onCreate();
}

@Override
public void onDestroy()
{
    // TODO Auto-generated method stub
    super.onDestroy();
}

It looks like your IDE generated these for you, but you don't need them. Get rid of them, they're just clutter.

PendingIntent pendingNotificationIntent = PendingIntent.getActivity( this.getApplicationContext(),0, intent1,PendingIntent.FLAG_UPDATE_CURRENT);

Ouch! In Java, we put one space after each comma in the argument list, and sometimes spaces in expressions. This line should look like

PendingIntent pendingNotificationIntent = PendingIntent.getActivity(this.getApplicationContext(), 0, intent1, PendingIntent.FLAG_UPDATE_CURRENT);

Also, if you put spaces around an operator, it should be one space on each side, so Intent.FLAG_ACTIVITY_SINGLE_TOP| Intent.FLAG_ACTIVITY_CLEAR_TOP should be Intent.FLAG_ACTIVITY_SINGLE_TOP | Intent.FLAG_ACTIVITY_CLEAR_TOP

notification.setLatestEventInfo(this.getApplicationContext(), "Score Collection!", "Please enter scores at this time!", pendingNotificationIntent);

If I recall correctly, on Android all user-facing strings should be handled by the OS so that you can localize them easily.

Other than that, this code looks pretty good.


A Discord bot that connects to a local database (OOP) I am currently working on a Discord bot as a way to learn and practice Python. I have been trying to learn object-oriented programming, and apply the "don't repeat yourself" principle.

The code below connects/disconnects from a local database (using XAMPP) with the "mysql.connector" package, and registers a user in the database using their Discord ID. In my example below, I define a class called MySQL, and define three methods (connect(), disconnect() and query_users()) that I found myself using in almost every command/method.

In my register command/method, I call all three methods (connect(), disconnect() and query_users()). Is this the correct way to access those methods and their variables in my register command? My code works great, but the more I read on OOP, the less confident I become, and the more I question myself. Any tips or confirmation would be greatly appreciated.

Thank you all for your help.

"""The following packages/modules are required:"""
import json
import mysql.connector
from discord.ext import commands as viking

with open('config/database.json') as config:
    database = json.load(config)

class MySQL:
    def __init__(self, viking):
        self.viking = viking

    def connect(self):
        self.connection = mysql.connector.connect(**database)
        self.cursor = self.connection.cursor(buffered=True)

    def disconnect(self):
        self.cursor.close()
        self.connection.close()

    def query_users(self):
        self.cursor.execute("SELECT discord_id FROM users")
        self.existing_users = ', '.join([str(users[0]) for users in self.cursor])

    @viking.command(pass_context=True)
    async def register(self, ctx):
        """*register
        Viking will register your Discord ID in the Viking database."""

        self.connect()
        self.query_users()
        discord_id = str(ctx.message.author.id)

        if discord_id in self.existing_users:
            await self.viking.say('You are already a registered member in the Viking database.')
        else:
            self.cursor.execute("INSERT INTO users (discord_id) VALUES (%s)", (discord_id,))
            self.connection.commit()
            await self.viking.say('You are now registered in the Viking database.')

        self.disconnect()

Is this the correct way to access those methods and their variables in my register command?

Yes. Looks good to me.

I can't say I'm fond of your docstrings. But your code is perfectly clear.

You are focused on MySQL / MariaDB. I tend to access mysql through sqlalchemy, just in case in future I'll want to use sqlite or postgresql or another.

The code starting if discord_id in ... is a bit odd, perhaps it could go into a function? It is a usual expectation that calling code could import your module (define your class) more than once without odd side effects.


jQuery plugin best practice This is a plugin I wrote a while ago and I'm trying to improve it. Is this the best practice for jQuery plugins? Should I use options for flexibility?

The initial aim was to replicate the syntax for .fadeIn() in jQuery.

Any pointers would be appreciated.

/* AnimateCSS - CSS transitions instead of js */
(function ($, window, document, undefined) {

    // Function-level strict mode syntax
  'use strict';

    $.fn.animateCSS = function (effect, delay, callback) {

        // Return this to maintain chainability
        return this.each(function () {

            // Cache $(this) for speed and compression
            var $this = $(this),
                transitionEnd = "webkitAnimationEnd mozAnimationEnd msAnimationEnd oAnimationEnd animationEnd",
                animated = "animated",
                visibility = "visibility",
                visible = "visible",
                hidden = "hidden";

            // Create a function we can call later
            function run() {
                // Add the animation effect with classes
                $this.addClass( animated + " " + effect);

                // Check if the elemenr has been hidden to start with
                if ($this.css( visibility ) === hidden) {
                    // If it has, show it (after the class has been added)
                    $this.css( visibility, visible);
                }

                // If the element is hidden
                if ($this.is(":" + hidden)) {
                    // Show it
                    $this.show();
                }

                // Event triggered when the animation has finished
                $this.bind( transitionEnd, function () {
                    // Remove the classes so they can be added again later
                    $this.removeClass(animated + " " + effect);

                    // Add a callback event
                    if (typeof callback === "function") {
                        // Execute the callback
                        callback.call(this);

                        // Unbind the event handlers
                        $this.unbind( transitionEnd );
                    }
                });
            }
            // Check if delay exists or if it"s a callback

            if (!delay || typeof delay === "function") {
                // If it"s a callback, move it to callback so we can call it later
                callback = delay;

                // Run the animation (without delay)
                run();
            } else {
                // Start a counter so we can delay the animation if required
                setTimeout( run, delay );
            }
        });
    };
})(jQuery, window, document);

Awesome code,

Well named variables, commented and easy tor read
JsHint.com has nothing on this code
Use 'use strict'


Creating a user registration form How should I go about creating a registration page in Windows forms? I can easily create a registration page, but I'm not sure if it's the best way. I'd have a service account that is hard coded, and it creates entries in a user table. I don't like to hard code passwords, but I don't know of another way. I guess in every case I'd end up having to hard code a password, right? I do have SSL enabled, and I've Wiresharked it and was unable to read any of the traffic.

Here's a simplified example of some code I wrote a while ago:

Public Class Registration
Dim connStr As String = "server=domain.com; port=3306; user id=userid;password=password;database=db;SslMode=Required;"
Dim msc As New MySqlConnection

Private Sub registerBTN_Click(sender As System.Object, e As System.EventArgs) Handles registerBTN.Click
            Dim dr As MySqlDataReader
                dr = checkUsername.ExecuteReader()
            Dim match As Boolean = False
            While dr.Read()
                If dr("Username").ToString = usernameTXT.Text Then
                    match = True
                    Exit While
                End If
            End While
            dr.Close()
            If match = False Then
                Dim User_GUID As Guid = New Guid
                User_GUID = Guid.NewGuid()
                Dim Encrypted_Password As String = Encryption(passwordTXT.Text, User_GUID.ToString)
                    Dim mscCMD As MySqlCommand = New MySqlCommand("INSERT INTO Users (Username, UserID, Password, Profile, Email) " & _
                                                                      "VALUES (@uname, @id, @pwd, @url, @mail)", msc)
                    With mscCMD.Parameters
                        .AddWithValue("uname", usernameTXT.Text)
                        .AddWithValue("id", User_GUID.ToString)
                        .AddWithValue("pwd", Encrypted_Password)
                        .AddWithValue("url", urlOptTXT.Text)
                        .AddWithValue("mail", emailTXT.Text)
                    End With
                    mscCMD.ExecuteNonQuery()
                MsgBox("Registration successful!", MsgBoxStyle.Information)
                My.Settings.First_Run = False
                Me.DialogResult = Windows.Forms.DialogResult.OK
                Me.Close()
End Sub

Private Sub Registration_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
        Try
            msc.ConnectionString = connStr
            msc.Open()
        Catch ex As MySql.Data.MySqlClient.MySqlException
            Me.Close()
        End Try
    End Sub

I want to know if this is a good approach. Does this have any vulnerabilities? Is there a better way to create a user registration method?

Also, here's my encryption/decryption.

Public Function Encryption(ByVal Plaintext_Password As String, ByRef User_GUID As String) As String
    Dim guidANDPassword As String = User_GUID.ToString + Plaintext_Password
    Dim shaobj As New Security.Cryptography.SHA1CryptoServiceProvider
    Dim guidPwdBytes() As Byte = System.Text.Encoding.ASCII.GetBytes(guidANDPassword)
    guidPwdBytes = shaobj.ComputeHash(guidPwdBytes)

    Dim Encrypted_Password As String = String.Empty

    For Each b As Byte In guidPwdBytes
        Encrypted_Password += b.ToString("x2")
    Next
    User_GUID = User_GUID.ToString()
    Return Encrypted_Password
End Function

Public Function Decryption(ByVal Plaintext_Password As String, ByVal User_GUID As String) As String
    Dim guidANDPassword As String = User_GUID.ToString + Plaintext_Password
    Dim shaobj As New Security.Cryptography.SHA1CryptoServiceProvider
    Dim guidPwdBytes() As Byte = System.Text.Encoding.ASCII.GetBytes(guidANDPassword)
    guidPwdBytes = shaobj.ComputeHash(guidPwdBytes)
    Dim Encrypted_Password_Of_Specified As String = String.Empty

    For Each b As Byte In guidPwdBytes
        Encrypted_Password_Of_Specified += b.ToString("x2")
    Next
    Return Encrypted_Password_Of_Specified
End Function

If you are going to distribute this assembly, you are effectively distributing that password.  Tools such as ILSpy can decompile the assembly and the database will be compromised.

The Windows Forms application shouldn't directly connect to a remote SQL server, there should be a service or API layer on the web sever to handle that.

If you need to authenticate / authorize a user, they will need to each have their own logins.

WinForms should act like a thin client, just sending requests to the server and receiving responses.

The web server should receive, authenticate, authorize and execute those requests on the database, and then send a response.


Optimizing this AntiDos HttpModule I've written this module (using a tutorial on the web I can't find now  to stop unusual requests from clients.  It's working as I've tested it on a local system.

Is the logic fine enough?
Another problem is that it counts requests for non-aspx resources (images, css, ...), but it shouldn't.  How can I filter request for aspx pages ?

This is the module code:

public class AntiDosModule : IHttpModule
{
    const string blockKey = "IsBlocked";
    const string reqsKey = "Requests";

    public void Dispose()
    { }

    public void Init(HttpApplication context)
    {
        context.BeginRequest += ValidateRequest;
    }

    private static void ValidateRequest(object sender, EventArgs e)
    {
        // get configs from web.config
        AntiDosModuleSettings setting = AntiDosModuleSettings.GetConfig();

        int blockDuration = setting.IPBlockDuration;

        // time window in which request are counted e.g 1000 request in 1 minute
        int validationTimeWindow = setting.ValidationTimeWindow;

        // max requests count in specified time window e.g 1000 request in 1 minute
        int maxValidRequests = setting.MaxValidRequests;

        string masterKey = setting.MasterKey;

        HttpContextBase context = new HttpContextWrapper(HttpContext.Current);

        CacheManager cacheMgr = new CacheManager(context, masterKey);

        // is client IP blocked
        bool IsBlocked = (bool)cacheMgr.GetItem<Boolean>(blockKey);

        if (IsBlocked)
        {
            context.Response.End();
        }

        // number of requests sent by client till now
        IPReqsHint hint = cacheMgr.GetItem<IPReqsHint>(reqsKey) ?? new IPReqsHint();

        if (hint.HintCount > maxValidRequests)
        {
            // block client IP
            cacheMgr.AddItem(blockKey, true, blockDuration);

            context.Response.End();

        }

        hint.HintCount++;

        if (hint.HintCount == 1)
        {
            cacheMgr.AddItem(reqsKey, hint, validationTimeWindow);
        }

    }

}

internal class IPReqsHint
{

    public int HintCount { get; set; }

    public IPReqsHint()
    {
        HintCount = 0;
    }
}

and this is the CacheManager class:

public class CacheManager
{

    HttpContextBase context;

    string masterKey;

    public CacheManager(HttpContextBase context, string masterKey)
    {
        this.context = context;
        this.masterKey = masterKey;
    }

    public void AddItem(string key, object value, int duration)
    {
        string finalKey = GenerateFinalKey(key);

        context.Cache.Add(
            finalKey,
            value,
            null,
            DateTime.Now.AddSeconds(duration),
            System.Web.Caching.Cache.NoSlidingExpiration,
            System.Web.Caching.CacheItemPriority.Normal,
            null);
    }

    public T GetItem<T>(string key)
    {
        string finalKey = GenerateFinalKey(key);

        var obj = context.Cache[finalKey] ?? default(T);

        return (T)obj;
    }

    string GenerateFinalKey(string key)
    {
        return masterKey + "-" + context.Request.UserHostAddress + "-" + key;
    }
}

There definitely are not enough of these tools in the .NET world, but I've found that there are a lot in the WordPress/php world. My recommendation would be to review the code in those modules to determine if the logic is good enough. Certainly, "good enough" depends on your needs though.

Regarding the filtering of specific file types, perhaps https://stackoverflow.com/questions/331398/how-to-have-http-module-on-fire-events-only-for-specific-page-types will help?


Creating HTTP proxies for services I have a HTTP client factory that creates a separate HTTP proxy instance for each of my services - which are in the cloud. The factory creates each instance on the basis of an interface which the service implements. All of the proxies inherit from a base class called DalServiceBaseClient which you will see in my code. I don't quite like the way the factory resolves the type to be returned based on the interface.

Here's a simplified version of my code:

Here's how I call my factory to create one of the possible proxies:

var gatewayFactory = new GatewayFactory();
var masterClient = gatewayFactory.CreateMasterClient("someBaseAddress");

Here that same method in GatewayFactory:

public IMasterService CreateMasterClient(string baseAddress)
{
    return (MasterServiceClient)CreateHttpClient<IMasterService>($"{baseAddress}api/master/");
}

And here's the private create method in GatewayFactory where all of the magic happens:

private DalServiceBaseClient CreateHttpClient<T>(string baseAddress)
{
    var httpClient = new HttpClient
    {
        BaseAddress = new Uri(baseAddress)
    };

    if (typeof(T) == typeof(IMasterService))
    {
        return new MasterServiceClient(httpClient);
    }
    if (typeof(T) == typeof(ISlaveService))
    {
        return new SlaveServiceClient(httpClient);
    }
    if (typeof(T) == typeof(IRoleService))
    {
        return new RoleServiceClient(httpClient);
    }

    // A lot more ifs..

    throw new ArgumentException("No type exists for the provided interface");
}

Since there are a lot of different proxies my method ends up having a lot of ifs and I haven't been able to find a more elegant way of doing this.

Create a strategy to find the mapped implementation of the interface provided:

IDictionary<Type, Type> mappings; //Populated with all interface to implementation types

private DalServiceBaseClient CreateHttpClient<T>(string baseAddress) {
    var httpClient = new HttpClient {
        BaseAddress = new Uri(baseAddress)
    };
    var implementationType = mappings[typeof(T)];
    if (implementationType == null)
        throw new ArgumentException("No type exists for the provided interface");

    return (DalServiceBaseClient)Activator.CreateInstance(implementationType, httpClient);
}

Note how reflection is used to create an instance of the implementation.

The mappings would be pre-populated and can look like...

mappings = new Dictionary<Type, Type>();
mappings[typeof(IMasterService)] = typeof(MasterServiceClient);
mappings[typeof(ISlaveService)] = typeof(SlaveServiceClient);

//...other types;

Another option if not too keen on using reflection, is to use a factory delegate which would allow a more strongly typed mapping.

IDictionary<Type, Func<HttpClient, DalServiceBaseClient>> mappings; //Populated with all interface to implementation types

private DalServiceBaseClient CreateHttpClient<T>(string baseAddress) {
    var httpClient = new HttpClient {
        BaseAddress = new Uri(baseAddress)
    };
    var implementationFactory = mappings[typeof(T)];
    if (implementationFactory == null)
        throw new ArgumentException("No type exists for the provided interface");

    return implementationFactory(httpClient);
}

Which would be populated like

mappings = new Dictionary<Type, Func<HttpClient, DalServiceBaseClient>>();
mappings[typeof(IMasterService)] = (httpClient) => new MasterServiceClient(httpClient);
mappings[typeof(ISlaveService)] = (httpClient) => new SlaveServiceClient(httpClient);

//...other types;


Walk a directory tree recursively Any suggestions are welcome.

package je3.io;

import java.io.File;
import java.util.ArrayList;
import java.util.List;

/**
 * Created by IDEA on 31/01/15.
 */
public class DirWalker {
    private List<File> recursiveList = new ArrayList<File>();

    public void walkDir(String pathname) {
        File d = new File(pathname);
        recursiveList.add(d);
        if(d.isDirectory()) {
            for(String f : d.list()) {
                walkDir(f);
            }
        }
    }

    public void reset() {
        recursiveList.clear();
    }

    public List<File> getRecursiveList() {
        return recursiveList;
    }

    public static void main(String[] args) {
        DirWalker dirWalker = new DirWalker();
        dirWalker.walkDir("/tmp");
        dirWalker.getRecursiveList().forEach(System.out::println);
    }
}

It's simpler to just return the result immediately then need another method.

Also the caller is probably going to need them as File objects eventually so lets just return them as those.

Then there are 2 options, just return a List and use addAll each recursion step:

public List<File> walkDir(File pathname) {
    if(pathname.isDirectory()) {
        List<File> files = new ArrayList();
        files.add(pathname);
        for(File f : pathname.listFiles()) {
            filed.addAll(walkDir(f));
        }
        return files;
    }else{
        return Collections.singletonList(pathname);
    }
}

Or use a private recursive method where you pass in a accumulator List:

private void walkDirRecurse(File pathname, List<File> files) {
    if(pathname.isDirectory()) {
        for(File f : pathname.listFiles()) {
            files.add(f);
            walkDirRecurse(f, files);
        }
    }
}


Should a PDO object be passed to classes and procedural functions as a parameter? I've been trying to figure out the best way to manage database connections for a while now and am still unsure of how I should be doing things. I know there are a lot of similar questions on here, but I've read many of them and am still confused.

Some answers on here say singletons should be used, but it seems a larger percentage of answers say not to use singletons because they make debugging much harder.

My current method is to create a PDO object and then pass it as a parameter:

/**
 * Creates and returns PDO object
 *
 * @return PDO
 */
function createPDO() {
    // MySQL connection
    $dbUser = '';
    $dbPass = '';
    $dbName = '';

    try {
        $db = new PDO("mysql:dbname=$dbName;charset=utf8", $dbUser, $dbPass, array(PDO::ATTR_EMULATE_PREPARES => false, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION));
    }
    catch (PDOException $e) {
        errorHandler(0, $e->getMessage(), $e->getFile(), $e->getLine(), print_r($e, true), 'sql');
        die('SQL Error, terminating script.');
    }

    return $db;
}

/**
 * Makes a PDO query
 *
 * @param @var $stmt The PDO statement
 * @param array $array The array of PDO statement parameters
 */
function makeQuery($stmt, $array) {
    try {
        $stmt->execute($array);
    }
    catch (PDOException $e) {
        errorHandler(0, $e->getMessage(), $e->getFile(), $e->getLine(), print_r($e, true), 'sql');
        die('SQL Error, terminating script.');
    }
}

Then in my scripts I'll use the object like this:

$db = createPDO();

$stmt = $db->prepare('SELECT ....')
makeQuery($stmt, array(':boundParameter' => '..'));
$result = $stmt->fetch(PDO::FETCH_ASSOC);

If I instantiate new classes, or call procedural functions, I pass $db as a parameter:

$myClass = new myClass($db);

Is this a good way of doing things, or is there a better way?

Let's start with the review, and then talk about correctness of the approach.

Pass things by parameters - What do you do if your credentials change? Don't you want to reuse the function? Pass the credentials as parameters to the createPDO function, or better yet, the DSN directly:

function createPDO($dsn, $user, $password)

Add good practice defaults to PDO instance - Use $pdo->setAttribute() to set PDO::ATTR_ERRMODE to PDO::ERRMODE_EXCEPTION and PDO::ATTR_EMULATE_PREPARES to false.
Don't handle exceptions in your function - You are creating hidden dependencies (You now depend on errorHandler to exist somewhere in your application). Also, you don't know what you want to do if you fail creating the PDO instance. In this app you may want to kill the script, in another you may want to fall back to the filesystem. It's not your function's job to know!
Typehint what you can - Your $stmt variable is a PDOStatement object. You should hint it in the function's signature. Your $array is an array.

function makeQuery(PDOStatement $stmt, array $params)

Approach wise, you should ask yourself, What in my application needs a PDO object?

If the rest of your application is procedural, there's no point in even what little abstraction you tried to give here. However, if you do have some sort of other object which uses the PDO connection, then using lazy loading is a good practice to follow:

$pdoProvider = function() {
    //Do all PDO instantiation logic here
};
$mapperFactory = new MapperFactory($pdoProvider);

By passing in a function instead of the actual object, you can delay the creation of the object until it's first needed (which may not even happen, saving you the connection!)

In the MapperFactory:

class MapperFactory {
    private $provider;
    private $pdoInstance;
    public function __construct(callable $provider) {
        $this->provider = $provider;
    }
    public function create($mapper) {
        if ($this->pdoInstance !instanceof PDO) {
            $this->pdoInstance = call_user_func($this->provider); //Call the provider, get the PDO instance
        }
        //By this point, $this->pdoInstance has a PDO instance for sure.
    }
}

For more information, See this answer on Stack Overflow.


Abstract Factory + Strategy pattern for parsing log files in multiple formats — improvement suggestions? I'm writing a service to parse machine log files as they are written to a central directory and push them to a web service. Each machine generates several log files at a time, with each file encapsulating a particular domain of operational information (e.g. user login sessions, errors, statistics, etc.).

The log files are written by different models of machines from different manufacturers. All the machines generate the same kinds of information, but the structure of the log files differs according to the manufacturer of the machine and the type of information the log file contains. Below is a sketch of a set of factories that will create Log classes for each type of log file and configure them with a Parser appropriate for that manufacturer (Strategy Pattern).

Over time new machines will be added to the logging pool and it should be straightforward to add support for these. The proposed design is arranged around the manufacturer and it makes it simple to add support for new manufacturers. However, I am concerned about a scenario in which a new model from an existing manufacturer does not adhere to that manufacturer's existing log format.

Is there a better design?

public interface ILogFactory
{
    ISessionLog CreateSessionLog();
    ITipLog CreateTipLog();
    IStatisticsLog CreateStatisticsLog();
}

public class SmithsLogFactory : ILogFactory
{
    public ISessionLog CreateSessionLog()
    {
        return new SessionLog() { Parser = new SmithsSessionStrategy(); }
    }

    public ITipLog CreateTipLog()
    {
        return new TipLog() { Parser = new SmithsTipStrategy(); }
    }

    public IStatisticsLog CreateStatisticsLog()
    {
        return new StatisticsLog() { Parser = new SmithsStatsStrategy(); }
    }
}

public class L3LogFactory : ILogFactory
{
    public ISessionLog CreateSessionLog()
    {
        return new SessionLog() { Parser = new L3SessionStrategy(); }
    }

    public ITipLog CreateTipLog()
    {
        return new TipLog() { Parser = new L3TipStrategy(); }
    }

    public IStatisticsLog CreateStatisticsLog()
    {
        return new StatisticsLog() { Parser = new L3StatsStrategy(); }
    }
}

public class VividFactory : ILogFactory
{
    public ISessionLog CreateSessionLog()
    {
        return new SessionLog() { Parser = new VividSessionStrategy(); }
    }

    public ITipLog CreateTipLog()
    {
        return new TipLog() { Parser = new VividTipStrategy(); }
    }

    public IStatisticsLog CreateStatisticsLog()
    {
        return new StatisticsLog() { Parser = new VividStatsStrategy(); }
    }
}

Few thoughts about your code:

your case resembles more Abstract Factory pattern more than Strategy pattern. ILogFactory is your Abstract Factory while SmithsLogFactory, VividFactory etc are yours Concrete Factories. ISessionLog, ITipLog and IStatisticsLog are Abstract Products and, for example, TipLog with L3TipStrategy inside is your Product.
Because I see this case as Abstract Factory, I would add derived versions of ISessionLog, ITipLog and IStatisticsLog for each manufacturer. Constructor of each of those classes should take care of details - like creating proper version of Parser subclass.
When using Abstract Factory I always like to provide * construction parameters* to the methods, which can be used to control concrete products creation - and here is the place where I would handle issue with different versions of the logs from the same manufacturer.

Draft summarizing my thoughts:

    public class LogFactoryParameters
    {
       // ... for example, Version
    }

    public class L3LogFactory : ILogFactory
    {
        public ISessionLog CreateSessionLog(LogFactoryParameters parameters)
        {
            return new L3SessionLog(parameters);
        }

        public ITipLog CreateTipLog(LogFactoryParameters parameters)
        {
           return new L3TipLog(parameters);
        }

        public IStatisticsLog CreateStatisticsLog(LogFactoryParameters parameters)
        {
           return new L3StatisticsLog(parameters);
        }
    }


C++ call std::list of objects instead of C linked list Can this be made more C++ a like with std::list or something so the Clink class is not needed?

I have a dynamic list of objects, and the number of them could change during runtime.

The expected output in no specific order. As an example 'Cb' runs before 'Ca':

link-contructed
Ca-contructed
Cb-contructed
Cb-run
Ca-run
Ca-destructed
Cb-destructed
link-destructed

I would like the program to be more C++ like with std::list or something. I have looked at std::forward_list but can not really see how it should be used. All examples I could find is about std::list<int>. My Ctask needs to have next in it and Clink needs to know which objects is used. Not so generic. Here is the mixed C/C++ program which does the expected:

/*
vim: ts=4 :
g++ -lstdc++ -o linked linked.cpp && ./linked
*/

#include <iostream>
using namespace std;

class Ctask {
public:
    Ctask *next = NULL;
    Ctask(void) {}
    virtual ~Ctask(void) {}
    virtual void run(void) = 0;
};

/* --------------------------------- */

class Clink {
public:
    Clink(void) {
        cout << "link-contructed" << endl;
    }
    ~Clink(void) {
        Ctask *temp;
        while (temp = head) {
            head = head->next;
            delete temp;
        }
        cout << "link-destructed" << endl;
    }
    void run(void) {
        Ctask *temp = head;
        while (temp) {
            temp->run();
            temp = temp->next;
        }
    }
    void add(Ctask *newnode) {
        if (tail)
            tail->next = newnode;
        else
            head = newnode;
        tail = newnode;
    }
private:
    Ctask *head = NULL;
    Ctask *tail = NULL;
};

/* --------------------------------- */

class Ca: public Ctask {
public:
    Ca(void) {
        cout << "Ca-contructed" << endl;
    }
    ~Ca(void) {
        cout << "Ca-destructed" << endl;
    }
    void run(void) {
        cout << "Ca-run" << endl;
    }
};

class Cb: public Ctask {
public:
    Cb(void) {
        cout << "Cb-contructed" << endl;
    }
    ~Cb(void) {
        cout << "Cb-destructed" << endl;
    }
    void run(void) {
        cout << "Cb-run" << endl;
    }
};

/* --------------------------------- */

int main(void) {
    Clink link;
    link.add((Ctask *)new Ca());
    link.add((Ctask *)new Cb());
    link.run();
    return 0;
}

Based on your answer and the comments from @user673679, with some modifications:

Removing using namespace std.
Using unique_ptr to transparently handle object allocation and deallocation. Remember to create items with make_unique.
forward_list is just fine for a FIFO queue.

Note that I don't use a range-based for loop because I only iterate the list once (run function and remove element).

#include "ctask.h" // Ctask, Ca and Cb definitions
#include <forward_list>
#include <iterator>
#include <memory>

class TaskQueue {
public:
   TaskQueue() :
     _tasks(),
     _last( _tasks.before_begin() )
   {
   }

   ~TaskQueue() = default;

   void add( std::unique_ptr<Ctask>&& t ) {
     _last = _tasks.insert_after( _last, std::move(t) );
   }

   void run_all() {
     task_list::iterator prev = _tasks.before_begin();
     task_list::iterator curr = _tasks.begin();
     while( curr != _tasks.end() ) {
        (*curr)->run();
        curr = _tasks.erase_after(prev);
     }
     _last = prev;
   }

private:
   typedef std::forward_list<std::unique_ptr<Ctask>> task_list;
   task_list           _tasks;
   task_list::iterator _last;
};

int main(void) {
    TaskQueue queue;
    queue.add(std::make_unique<Ca>());
    queue.add(std::make_unique<Cb>());
    queue.run_all();
    return 0;
}

Edit: I realized that you can make run_all more readable since you are basically removing all the elements from the queue, so that the while loop doesn't actually depend on _last value. The simplified function would be:

   void TaskQueue::run_all() {
     while( !_tasks.empty() ) {
        _tasks.front()->run();
        _tasks.pop_front();
     }
     _last = _tasks.before_begin();
   }