Creating a definition text from 3 string arguments Here is my code to create a definition text from 3 arguments. Is there any way to make it work with better performance and look nicer?

 private const string IllegalCharacters = "\\" + "\"" + "'#,.[]{}" + " ";

 public static string CreateDefText(string mainId, string     secondaryId,    string extraText)
    {
        var sb = new StringBuilder();
        sb.Append(mainId);
        if (sb.Length > 0 && !string.IsNullOrEmpty(secondaryId))
        {
            sb.Append("_");
        }
        sb.Append(secondaryId);
        sb.Append(extraText);
        if (sb.Length > 0 && (char.IsNumber(sb[0]) || '+'.Equals(sb[0]) || '-'.Equals(sb[0])))
        {
            sb.Insert(0, "N");
        }
        foreach (var illegalChar in IllegalCharacters)
        {
            sb.Replace(illegalChar.ToString(CultureInfo.InvariantCulture), string.Empty);
        }
        return sb.ToString();
    }

Code Style

Logic section

Use blank lines to separate your code into logical sections

Instead of:

var sb = new StringBuilder();
sb.Append(mainId);
if (sb.Length > 0 && !string.IsNullOrEmpty(secondaryId))
{
   sb.Append("_");
}
sb.Append(secondaryId);
sb.Append(extraText);

You can write:

var sb = new StringBuilder();   // section of object creation

sb.Append(mainId); // section of appending mainID

// section of checking for mainID and secondaryID
if (sb.Length > 0 && !string.IsNullOrEmpty(secondaryId))
{
   sb.Append("_");
}

sb.Append(secondaryId);
sb.Append(extraText);

The same for the rest part.

Long condition in if statement

Don't use long condition in if statement. It's not readable.

Instead of:

if (sb.Length > 0 && (char.IsNumber(sb[0]) || '+'.Equals(sb[0]) || '-'.Equals(sb[0])))

You can use:

isNum = char.IsNumber(sb[0]);
isPlus = sb[0].Equals('+');
isMinus = sb[0].Equals('-');

if (sb.Length > 0 && (isNum || isPlus || isMinus)
{
   sb.Insert(0, "N");
}

We can move deeply and I want to get rid of this long if statement:

if (sb.Length > 0)
{
   isNum = char.IsNumber(sb[0]);
   isPlus = sb[0].Equals('+');
   isMinus = sb[0].Equals('-');

   if (isNum || isPlus || isMinus)
   {
      sb.Insert(0, "N");
   }
}

I still don't like this if construction, I'll extract this code into separate method. Before this I'll create an array of legal symbols

if (sb.Length > 0)
{
   isNum = char.IsNumber(sb[0]);
   isLegalSymb = isLegalSymb(sb[0]);

   if (isNum || isLegalSymb)
   {
    sb.Insert(0, "N");
   }
}

Separate method for checking if character is legal.

private static bool isLegalSymb(char symb) {
   string[] legalSymb = { "+", "-" };

   for (int i = 0; i < legalSymb.Length; i++) {
     if (symb.Equals(legalSymb[i])) {
       return true;
     }
   }

   return false;
}

Comments

Your code is not always obvious and other developers may don't understand anything without a context of this code.

In such situations you can provide more comments to make our life more easy.

It relates to your code:

foreach(var illegalChar in IllegalCharacters) {
  sb.Replace(illegalChar.ToString(CultureInfo.InvariantCulture), string.Empty);
}

I'm also confused with:

private const string IllegalCharacters = "\\" + "\"" + "'#,.[]{}" + " ";

I'll again extract this code into separate method:

replaceAllBannedCharacters(sb);

Separate method for replacing all banned symbols:

private static void replaceAllBannedCharacters(StringBuilder sb) {
  string[] bannedSymb = { "#", "'", ".", "[", "]", "{", "}", "," };

  for (int i = 0; i < bannedSymb.Length; i++) {
    sb.Replace(bannedSymb[i], string.Empty);
  }
}

Naming

mainId can be replaced with firstID

secondaryId can be replaced with secondID


Optimizing FirstOrDefault I'm working on an application in which it takes quite a bit of time to initialize the data.

Some background: I'm creating a sort of pivot table in which I turn Figure 1 below into Figure 2. I've gone through with Stopwatchs and have been able to isolate a single line of code that takes 2 seconds during program load (it is in a loop called for roughly 2-300 rows of data, 2 seconds is the total time across all iterations). That line is marked in the code example below, Figure 3.

My question is if there is a better way to structure the example method such that I can avoid whatever overhead that line is causing. It is worth noting that when that line executes, mChar contains sometimes 0 elements, but up to around 5 or 1 (see edit below), so I can't imagine what would cause this to take so much time to execute.

Figure 1

SAMPLENUMBER | ANALYSIS_NAME | ANALYSIS_STATUS
----------------------------------------------
   1234      |     NO3       |       I
   1234      |     SO4       |       C
   5678      |     NO3       |       C

Figure 2

SAMPLENUMBER |     NO3       |      SO4
----------------------------------------------
   1234      |      I        |       C
   5678      |      C        |

Figure 3

private List<string> GetPivotData(DataRow sourceRow, DataTable sourceTable)
{
  int pivotStartIndex = sourceTable.Columns.IndexOf(Cols.HotDate) + 1;
  var retList = new List<string>(sourceTable.Columns.Count - pivotStartIndex);
  for (int i = pivotStartIndex; i < sourceTable.Columns.Count; i++)
  {
    string[] elementTest = sourceTable.Columns[i].ColumnName.Split('-');
    var mChars =
        from s in mainData.AsEnumerable()
        where s.Field<string>(Cols.Samplenumber) == sourceRow.Field<string>(Cols.Samplenumber)
          && s.Field<string>(Cols.ElementName) == elementTest[0].Trim()
          && s.Field<string>(Cols.AnalysisNumber) == elementTest[1].Trim()
        select s.Field<string>(Cols.AnalysisStatus);
    retList.Add(mChars.FirstOrDefault()); // <-- this is the line taking so much time.
  }
  return retList;
}

I've tried things like having retList as an array rather than a list, as well as calling .ToList() on my LINQ result and using .Find(x => true) and they really haven't had any effect. The method in Figure 3 is itself inside a for loop, called roughly 200 times, once per sample. The for loop inside the method itself iterates on average 5 times, so that line of code gets called about 1000 times, depending on the data. That means that the average time for the line is around 2ms, which may just be too small in itself to be able to do anything with. Swapping my for loops for Parallel.For halves the time it takes, but I've found that my logic doesn't work well with concurrency. At all.

Edit: I should clarify why I'm using FirstOrDefault() I suppose and I need to clarify an error in my initial post as well. The error is that I said mChars can contain up to 5 elements when FirstOrDefault() is called (it is retList that has up to 5). It will only ever have 0 or 1 element(s). When it's empty, I still need to add a null to the list, so I'm looking into a way to add the mChars result directly to the list without any searches at all.

The line with from only defines the enumeration, but it doesn't actually run over the list and apply your select clause. In your case, that one is only applied when you call FirstOrDefault.

Therefore, to speed up the call to FirstOrDefault you need to speed up your where condition. I mean this one:

s.Field<string>(Cols.Samplenumber) == sourceRow.Field<string>(Cols.Samplenumber)
      && s.Field<string>(Cols.ElementName) == elementTest[0].Trim()
      && s.Field<string>(Cols.AnalysisNumber) == elementTest[1].Trim()

The most expensive parts about this are probably the calls to s.Field<string>(...), followed by the Trim() calls. All of these can be cached. Just convert the maindata into something that's cheap to access, and only do the conversion once, outside of the for loop. Something more or less like this (this won't run as is, only use it as inspiration):

struct TempItem
{
    public string Samplenumber;
    public string ElementName;
    public string AnalysisNumber;
    public string AnalysisStatus;
}
string sourceSampleNumber = sourceRow.Field<string>(Cols.Samplenumber);
var lookUpList = mainData.AsEnumerable().Where(i => i.Samplenumber == sourceSampleNumber).Select(i=> new TempItem {
        Samplenumber = s.Field<string>(Cols.Samplenumber),
        ElementName = s.Field<string>(Cols.ElementName),
        AnalysisNumber = s.Field<string>(Cols.AnalysisNumber),
        AnalysisStatus = s.Field<string>(Cols.AnalysisStatus)
    }).ToList(); // the ToList() part here forces application of the linq expressions

for (int i = pivotStartIndex; i < sourceTable.Columns.Count; i++)
{
    string[] elementTest = sourceTable.Columns[i].ColumnName.Split('-');
    string elementName = elementTest[0].Trim();
    string analysisNumber = elementTest[1].Trim();
    var mChars = lookUpList.Where(i => i.ElementName == elementName && i.AnalysisNumber == analysisNumber).Select(i=>i.AnalysisStatus);
    retList.Add(mChars.FirstOrDefault());
}


GUI system in PyGame I'm making a GUI system in Python for game development with PyGame. Right now I have a button that changes colors when you hover and click on it. My question is, is this code well designed or can it be improved?

game.py

import sys, pygame
from pygame.locals import *
from gui import *

def input(events):
    for e in events:
        if e.type == QUIT:
            pygame.quit()
            sys.exit()

pygame.init()
pygame.display.set_caption("GUI Demo")

window = pygame.display.set_mode((640, 480))
screen = pygame.display.get_surface()
clock = pygame.time.Clock()
font = pygame.font.Font(None, 32)
button = Button("btn_1", "Hello World", 50, 50, 200, 120)

button.decorate((255, 0, 0), (0, 255, 0), (0, 0, 255))

while True:
    screen.fill((120, 40, 200))
    input(pygame.event.get())
    button.update(pygame.mouse.get_pos(), pygame.mouse.get_pressed())
    button.draw(screen)
    pygame.display.flip()

gui.py

# Mini-GUI demo

import pygame

BUTTON_NORMAL = 0
BUTTON_HOVER = 1
BUTTON_ACTIVE = 2

class Surface:
    def __init__(self, x, y, w, h, color):
        self.x = x
        self.y = y
        self.w = w
        self.h = h
        self.color = color

    def collides(self, surface):
        return self.y < surface.top + surface.height and self.y + self.h > surface.top and self.x < surface.left + surface.width and self.x + self.w > surface.left

    def getCoords(self):
        return (self.x, self.y)

    def draw(self, surface):
        pass

class GUIElement(Surface):
    def __init__(self, id, x, y, w, h, color):
        super(GUIElement, self).__init__(x, y, w, h, color)
        self.id = id

class Textbox(GUIElement):
    def __init__(self, id, x, y, w, h, color):
        super(Textbox, self).__init__(id, x, y, w, h, color)
        self.data = ""

    # TODO: update and draw

class Button(GUIElement):
    def __init__(self, id, text, x, y, w, h):
        super(Button, self).__init__(id, x, y, w, h, None)
        self.text = text
        self.font = pygame.font.Font(None, 32)
        self.state = BUTTON_NORMAL
        self.normalColor = None
        self.hoverColor = None
        self.activeColor = None

    def decorate(self, normalColor, hoverColor, activeColor):
        self.normalColor = normalColor
        self.hoverColor = hoverColor
        self.activeColor = activeColor
        self.color = self.normalColor

    def update(self, mouseCoords, mousePressed):
        (up, down, middle) = mousePressed
        if len(mouseCoords) < 4: mouseCoords = pygame.Rect(mouseCoords[0], mouseCoords[1], 1, 1)

        if not self.collides(mouseCoords):
            self.state = BUTTON_NORMAL
            self.color = self.normalColor
            return

        if up:
            self.state = BUTTON_ACTIVE
            self.color = self.activeColor
        else:
            self.state = BUTTON_HOVER
            self.color = self.hoverColor

    def draw(self, surface):
        pygame.draw.rect(surface, self.color, pygame.Rect(self.x, self.y, self.w, self.h))
        surface.blit(self.font.render(self.text, 1, (255, 255, 255)), self.getCoords())

There are no docstrings. What is the purpose of each of your classes and methods, and how am I supposed to call them?
If you click outside the button, and then slide the pointer over the button, it becomes active. Normal GUI buttons require the initial click to be on the button in order to be able to activate it.
The class name Surface runs the risk of confusion with pygame.Surface. Or in some applications, of shadowing it.
The GUIElement class is a Surface with an id. But none of the code uses the id, so this class is useless. (No doubt you have some purpose in mind, but what is it?)
Calling sys.exit() makes it awkward to test your program from the interactive interpreter.
If you care about portability to Python 2, you should make Surface inherit from object so that it is a new-style class. On the other hand, if you don't care about portability to Python 2, just call super() instead of super(GUIElement, self) or whatever.
Surface takes arguments x, y, w, h. Why not use a pygame.Rect? Then you could just call Rect.colliderect instead of writing your own version.
Surface.draw does nothing. Is this because Surface is an abstract base class? If so, you should use metaclass = ABCMeta and decorate the draw method with @abstractmethod to make this clear.
In order to create a coloured button, you have to create the Button object and then call its decorate method to set its colours. Why not allow the caller to set the colours when they create the button? (Via optional keyword arguments.) Similarly for the font.
Since you can deduce a button's color from its state, you don't need a separate color attribute (or do you?). By having two attributes you run the risk of them getting out of sync. Why not have a map from state to colour?
up, down, middle are misleading variable names: the values returned by pygame.mouse.get_pressed() are actually the state of the three mouse buttons. Use names like button1, button2, button3.
pygame.mouse.get_pos() returns the mouse coordinates (x, y) so it's not clear why you write if len(mouseCoords) < 4. Won't this always be true? And why do you need to make a rectangle here anyway?


Advice on "Factory" Pattern Implementation I'm refactoring some code around a couple of ASP.NET Web Forms pages and decided to try out a variation of the Abstract Factory pattern of my own design. I need to create an implementer of an abstract class based on what query string value is available.

Here's (a simplified version of) what I have right now:

internal static class Factory
{
   public static AbstractClass Create(NameValueCollection queryString)
   {
       if (queryString["class1"] != null)
       {
            return new Class1(queryString["class1"]);
       }
       if (queryString["class2"] != null)
       {
           return new Class2(queryString["class2"]);
       }
       if (queryString["class3"] != null)
       {
           return new Class3(queryString["class3"]);
       }

       return null;
   }
}

I don't know how I feel about this implementation. It certainly is better than what was there before, but I have a strong sense this could be improved some how.

So now, in the relevant pages, I can simply do this:

AbstractClass item = Factory.Create(Request.QueryString);
item.DoStuff();

Does anyone have any suggestions? Or would this be considered good as-is?

I Think you should do it more generic:

public static class Factory
{
    static Dictionary<String, Func<String, Abstract>> classes;

    public static Abstract GetInstance(String s){
        if(!classes.ContainsKey(s)) throw new Exception();
        Func<String, Abstract> create=classes[s];
        return create(s);
    }

    static Factory ()
    {
        classes=new Dictionary<String, Func<String, Abstract>>(){
            {"class1", (x)=>new Class1(x)},
            {"class2", (x)=> new Class2(x)}
        };
    }
}

This code is way more readable.
You have the static constructor block, where you register the factory methods according to the keys. And the GetInstance-Method shrinks to 3 lines, of which one is a purely guarding clause.

Edit: modified after Mat's Mug's hint.

P.S: Oh, I forgot to mention, that you really never should return null.

Better a) throw an Exception or b) use a NullObject.


Possible sub-optimal selection sort Why is this code "bad"? In all of the selection sort codes I am learning from, they do it much differently. I will learn the proper code from the book, but I would love to know why this particular code is bad.

public class Main {
    public static void main(String[] args) {
        int[] array = {8, 3, 5, 7, 9, 2, 4, 6, 8, 0};
        System.out.println("The sorted array is: ");
        displayArray(selectionSort(array));
    }

    public static int[] selectionSort(int[] list) {
        int[] result = list;

        for (int i = 0; i < result.length - 1; i++) {
            for (int j = i + 1; j < result.length; j++) {
                if (result[i] > result[j]) {
                    int temp = result[i];
                    result[i] = result[j];
                    result[j] = temp;
                }
            }
        }
        return result;
    }

    public static void displayArray(int[] list) {
        for (int i = 0; i < list.length; i++) {
            if ((i + 1) % 5 == 0)
                System.out.println(list[i]);
            else
                System.out.print(list[i] + " ");
        }
    }
}

Too many swaps
The "standard" selection sort finds the minimum element and swaps the first element with the minimum element once per loop. In total there should be N swaps. Your version of selection sort can swap up to (N^2)/2 times.
Unnecessary variable
I'm not sure why you use the variable result and return it.  You can just operate on the array that is passed in (list) directly.  When I first read your code I thought you were making a copy of the array, because you had this extraneous variable.
The "standard" selection sort
Here is an example of a "standard" selection sort:
public static void selectionSort(int[] list)
{
    int len = list.length;
    for (int i = 0; i < len - 1; i++) {
        int smallestIndex = i;
        int smallestValue = list[i];
        for (int j = i + 1; j < len; j++) {
            if (list[j] < smallestValue) {
                smallestIndex = j;
                smallestValue = list[j];
            }
        }
        if (smallestIndex != i) {
            list[smallestIndex] = list[i];
            list[i] = smallestValue;
        }
    }
}


Convert light-years to other representations the code is very messy, long and I am looking how to shorten it and/or simplify it. There are a lot of repeated variables, for example; 'hourToDay' and then 'hourToDay1', 'hourToDay2' and so forth. Can you help me find a better way to make this program more efficient?

P.S - I am a new coder and wanting to improve my ability, any suggestions would be grand :)

long lightYear = 6000000000000L;
int hoursInDay = 24;
int daysInYear = 365;
int yearsInCentury = 100;
int yearsInMillenium = 1000;

Scanner lightYearInput = new Scanner(System.in);
System.out.println("Enter light year's you'd like to convert" );

long light = lightYearInput.nextLong();
long totalLightYear = (long) (lightYear * light);

// Different vehicle variables with speed.

double car  = 277.87; //mph koenigseggAgeraRS
double MiG25 = 1919.42; //mph
double parkerSpaceProbe = 213240; //mph

// car conversion
long hourConversion = (long)(totalLightYear / car );
System.out.println("Driving the fastest car in space, would take you " + hourConversion + " hours to travel " + light + " light years."); //hours

long hourToDay = hourConversion / hoursInDay;
System.out.println(hourConversion + " hours is equivalent to " + hourToDay + " days."); //hours to days

long dayToYear = hourToDay / daysInYear;
System.out.println(hourToDay +" is equivalent to " + dayToYear + " years."); //days to years

long yearToCentury = dayToYear / yearsInCentury;
System.out.println(dayToYear +" is equivalent to " + yearToCentury + " Centuries."); //years to centuries

long yearToMillenium = yearToCentury / yearsInMillenium;
System.out.println(yearToCentury +" is equivalent to " + yearToMillenium + " millenium.\n"); // centuries to millenium

//MiG25 Conversion

System.out.println("PLANE");
long hourPlaneConversion = (long)(totalLightYear / MiG25 );
System.out.println("Driving the fastest plane in space, would take you " + hourPlaneConversion + " hours to travel " + light + " light years."); //hours

long hourToDay1 = hourPlaneConversion / hoursInDay;
System.out.println(hourPlaneConversion + " hours is equivalent to " + hourToDay1 + " days."); //hours to days

long dayToYear1 = hourToDay1 / daysInYear;
System.out.println(hourToDay1 +" is equivalent to " + dayToYear1 + " years."); //days to years

long yearToCentury1 = dayToYear1 / yearsInCentury;
System.out.println(dayToYear1 +" is equivalent to " + yearToCentury1 + " Centuries."); //years to centuries

long yearToMillenium1 = yearToCentury1 / yearsInMillenium;
System.out.println(yearToCentury1 +" is equivalent to " + yearToMillenium1 + " millenium.\n"); // centuries to millenium

The output works correctly (unless my maths / code is incorrect), so the program runs fine. I'm just simply looking to make my code more efficient. I'm sure you can see that I need to improve my ability, hopefully I can get some supportive guidance here.

Constants should be Constant

You shouldn't be able to change constants; they should be fixed, non-modifiable values.  In Java, this means declaring them final.

If you create two instances of your class, you'll end up with two instances of lightYear, hoursInDay and so on.  But since these values are fixed compile-time constants, it doesn't make sense to have multiple copies of them.  You just need one copy of these constants, which means declaring them as static.

Things like hours in a day, and days in a year are also fairly general values.  They could be of use in other classes, so it makes sense to make these values sharable, which means public.

Finally, convention is to name constants with an UPPERCASE_NAME.

So ...

public final static long MILES_PER_LIGHT_YEAR = 5878625541248L;

public final static int  HOURS_PER_DAY = 24;
public final static int  DAYS_PER_YEAR = 365;
public final static int  YEARS_PER_CENTURY = 100;
public final static int  YEARS_PER_MILLENNIUM = 1000; // Note: 2 N's

Time Conversion

As noted by @Sixsmith, the Hours to Days, Years, Centuries, and Millennium calculation is being repeated.  Unfortunately, they went a little far and added System.out.println("Driving the fastest car in space to their hoursConversion() function, so it is not general enough to use for the plane (or eventually, space probe).

Reducing the conversion code slightly produces a reusable function:

public static void output_hour_equivalents(long hours) {

    long days = hours / HOURS_PER_DAY;
    if (days > 0)
        System.out.println(hours + " hours is equivalent to " + days + " days.");

    long years = days / DAYS_PER_YEAR;
    if (years > 0)
        System.out.println(days + " days is equivalent to " + years + " years.");

    long centuries = years / YEARS_PER_CENTURY;
    if (centuries > 0)
        System.out.println(years + " years is equivalent to " + centuries + " years.");

    long millennia = years / YEARS_PER_MILLENNIUM;
    if (millennia > 0)
        System.out.println(years + " years is equivalent to " + millennia + " millennia.");
}

You could, of course, add "weeks" and "months" to this without loss of generality.  I added tests at each conversion to avoid printing out a zero duration if given a small number of hours, like output_hour_equivalents(10000).

Duration for distance at speed

The conversion from speed and lightyears, for the fastest class of an object, to duration can be moved into a general purpose function.  But we don't want to hard-code too much text.  We want to fly a plane, not drive one, so we should take in the object class, and the action we perform with it, as well as it's maximum speed and the distance to be traversed:

public static void describe_fastest(String object, String action, double max_speed, long light_years) {
    long miles = light_years * MILES_PER_LIGHT_YEAR;
    long hours = (long)(miles / max_speed);

    // Capitalize the first letter of our action, for output.
    action = action.substring(0, 1).toUpperCase() + action.substring(1);

    System.out.println(action + " the fastest " + object + " in space, would take you " + hours + " hours to travel " + light_years + " light years.");
    output_hour_equivalents(hours);
}

You can now use this in your method.  For example, from main, borrowing @Sixsmith's variable names:

public static void main(String[] args) {

    long light_years = ...

    describe_fastest("car", "driving", carMph, light_years);
    describe_fastest("plane", "flying", planeMph, light_years);
    describe_fastest("space probe", "traveling in", spaceProbeMph, light_years);
}

1 Light Year

If you provide the value 1 for light_years you'll get something like:

  Driving the fastest car in space ... to travel 1 light years.

I hate that "1 light years."  It should be "1 light year."  Singular.  MessageFormat can help us here:

private final static String MSG_FORMAT =
    "{0} the fastest {1} in space, would take you {2} hours to travel " +
    "{3} {3,choice,0#light years|1#light year|1<light years}.";

public static void describe_fastest(String object, String action, double max_speed, long light_years) {
    long miles = light_years * MILES_PER_LIGHT_YEAR;
    long hours = (long)(miles / max_speed);

    // Capitalize the first letter of our action, for output.
    action = action.substring(0, 1).toUpperCase() + action.substring(1);

    System.out.println(MessageFormat.format(MSG_FORMAT, action, object, hours, light_years));
    output_hour_equivalents(hours);
}

The format string constant is private, since it is unlikely that anyone else would re-use that string anywhere.

You can also add commas between groups of 3 digits, and other things to make the output a little more pleasing.  See MessageFormat for details.

You can also make it output "hour" or "hours", "day" or "days", "year" or "years", "century" or "centuries", and "millennium" or "millennia" in the other messages.

Exercise left to student.


Load modules conditionally Python I wrote a python module that needs to  load a file parser to work. Initially it was just one text parsing module but I need to add more parsers for different cases.

parser_class1.py

parser_class2.py

parser_class3.py

Only one is required for every running instance. I'm thinking load it by  command line:

mmain.py -p parser_class1

I wrote this code in order to select the parser to load when the main module is called:

#!/usr/bin/env python

import argparse
aparser = argparse.ArgumentParser()
aparser.add_argument('-p',
            action='store',
            dest='module',
            help='-i module to import')
results = aparser.parse_args()

if not results.module:
    aparser.error('Error! no module')
try:
    exec("import %s" %(results.module))
    print '%s imported done!'%(results.module)
except ImportError, e:
    print e

But, I was reading that this way is dangerous, maybe not standard.

Then is this approach ok?

Or must I find another way to do it?

Why?

Regarding the safety aspect of your question.
The reason why exec() can be dangerous is that is can allow a nefarious agent to execute code that you never intended.
Let's assume for example that somewhere in your program, you have sensitive data elements such as:
username = secret_username
password = never_share

And let's also assume that someone calls your program like this:
mmain.py -p 'parser_class1;print globals()'

Then your exec() statement would actually be:
exec("import %s" %('parser_class1;print globals()'))

And this would result in your program printing out all variables in your global space for them... including your username and password.
By making your program utilize the __import__ method as mentioned by @Jaime, you can at least prevent people from executing non-import statements in your code.
But, you should, whenever possible also examine the input from a user before using it to execute any dynamic code.


I think I'm not using Try Except properly in this Python code Please offer some advice for this code.  It all works, but I know that its messy and I don't think the Exception catching is doing what I want it to.  Thanks in advance for any and all advice...

def main():
    # loop over this forever
    while True:
        print '--- Getting next job for Device: %s' % device['Name']
        # get a job from the db
        try:
            job = db.JobQ.find_one({'DeviceName':device['Name'],'Status':u'ToDo'})
        except Exception as e:
            print '!!! Error getting job for Device: %s' % device['Name']
        if job:
            # if there's a job, get all the other jobs for the same file
            try:
                job_list = [
                        j for j in db.JobQ.find(
                            {'Filename':job['Filename'],'Status':u'Todo'}
                            ).sort('Created',ASCENDING)
                        ]
            except Exception as e:
                print '!!! Error getting job list for Filename: %s' % job['Filename']

            for job in job_list:
                # mark the job 'InProgress'
                job = db.JobQ.find_and_modify(
                        {'_id':ObjectId(job['_id'])},
                        {'$set':{
                            'Status':u'InProgress','Started':datetime.now()
                            }},new=True
                        )
                print '--- Performing %s on %s' % (job['Type'], job['Filename'])
                # try to do the job.. each job raises its own exceptions
                try:
                    perform_job(job)
                    job['Status'] = u'Done'
                    job['Finished'] = datetime.now()
                    db.JobQ.save(job,safe=True)
                except Exception as e:
                    # if any job fails, ditch the rest of the jobs and move on
                    print '!!! Error performing job: %s' % e
                    print 'Subsequent jobs for this filename will not run: %s'\
                            % job['Filename']
                    for job in job_list:
                        job['Finished'] = datetime.now()
                        job['Status'] = u'Failed'
                        db.JobQ.save(job,safe=True)
        else:
            print '--- No more jobs for Device: %s' % device['Name']

        sleep(device['CycleTime'])

if __name__ == '__main__':
    main()

A couple things:

Instead of if job:, try if not job: ... continue .  This will allow the entire (large) content of that if-block to be outdented a level, making it a little more readable.
Instead of job_list = [ j for j in ... ]; use job_list = list( ... ).

Also, the logic in your failure code looks a bit suspect - you set subsequent jobs as 'failed' but nothing keeps them from being attempted again by the enclosing for loop.  I think you basically want a break at the end of the except block to exit the loop there, though that leaves the for loop still setting some potentially passed jobs as failed (consider the case when the first job passes but the second fails - the loop will set them all as failed).  So you might instead add a fail_the_rest = False flag just before the for job in joblist loop and then as the first line of the loop do if fail_the_rest: <set failed> continue and then change the except block to just set failure on that one job and then set fail_the_rest = True.  But that's all speculative due to a lack of understanding about how your job submission and pass/fails work.


Create a structure to store data about a movie Below are the instructions for my program and the source code. My program runs and works but I would like second opinions on how to make my program more efficient or if there are any loose ends in my source code.

Using functional decomposition, write a C++ program that will use a structure called MovieData to store the following information.

Title
Director
Year Released
Running time (in minutes)
Production cost
First Year Revenue.

Then use a value returning function called getMovieData() to read data to each
component of a struct variable as stated above. This function must return a
variable of type MovieData.

Also use a void function called printMovieData() to print each member of struct
in a nice format (with appropriate description). This function will accept a
pointer to type MovieData, that is, its prototype will be like this:
void printMovieData(MovieData *);

In your main() program, declare two variable of type MovieData with the following declaration: MovieData m1,m2;

And then call getMovieData() to assign value to each of m1 and m2.

Finally call printMovieData() on each of m1 and m2 to print values in each struct.

#include "stdafx.h"
#include <iostream>
#include <string>

using namespace std;

struct MovieData
{
    string title;
    string director;
    int year_released;
    int running_time;
    int production_cost;
    int first_year_revenue;
};

// Function prototypes
MovieData getMovieData();
void printMovieData(MovieData *);

int main()
{
    // Variables
    MovieData m1, m2;
    MovieData *ptr1, *ptr2;
    ptr1 = &m1;
    ptr2 = &m2;

    // Call getMovieData function to get information for both movies
    m1 = getMovieData();

    m2 = getMovieData();

    // Call printMoviedata function to print information for both movies
    printMovieData(ptr1);

    cout << "\n";

    printMovieData(ptr2);

    system("PAUSE");

    return 0;
}

/**
* Pre-Condition:
* Post-Condition:
*/
MovieData getMovieData()
{
    // Variables
    MovieData temp;

    // Prompt user for information
    cout << "Enter the title of the movie: ";
    getline(cin, temp.title);

    cout << "\n";

    cout << "Enter the name of the movie's director: ";
    getline(cin, temp.director);

    cout << "\n";

    cout << "Enter the year the movie was released: ";
    cin >> temp.year_released;

    cout << "\n";

    cout << "Enter the running time of the movie in minutes: ";
    cin >> temp.running_time;

    cout << "\n";

    cout << "Enter the production cost of the movie: $";
    cin >> temp.production_cost;

    cout << "\n";

    cout << "Enter the movie's first year revenue: $";
    cin >> temp.first_year_revenue;

    cin.ignore();

    cout << "\n";

    return temp;
}

/**
* Pre-Condition:
* Post-Condition:
*/
void printMovieData(MovieData *pointer)
{
    cout << "Title: " << pointer->title << endl;
    cout << "Director: " << pointer->director << endl;
    cout << "Year Released: " << pointer->year_released << endl;
    cout << "Running Time: " << pointer->running_time << endl;
    cout << "Production Cost: " << pointer->production_cost << endl;
    cout << "First Year Revenue: " << pointer->first_year_revenue << endl;
    cout << "\n";
}

Looks pretty good overall. The biggest issue that I see is that the input reading doesn't check for errors, and it's pretty rough. It can be helpful with assignments like this to build up a little library of IO helpers:

// Read a line from the provided stream.
std::string readLine(std::istream& is) {
    std::string line;
    if (!std::getline(is, line)) {
        // Could stand to be a more specific exception, but you get the point
        throw runtime_error("Unexpected stream failure");
    }
    return line;
}

// Helper to output a prompt and read a line.
std::string promptLine(std::ostream& os, const std::string& prompt, std::istream& is) {
    os << prompt; // Note that I've assumed output won't fail. Depending on situation,
                  // that might be a bad assumption.
    return readLine(is);
}

// Attempt to parse an int from a line of input that must contain only an int.
bool readIntLine(std::istream& is, int& val) {
    std::string line = readLine();
    char* parseEnd = nullptr; // NULL if < C++11
    int val = std::strtol(line.c_str(), &parseEnd, 10);
    return (parseEnd - line.c_str() == line.size());
}

// Keep showing the same prompt until the user inputs an integer (and only an integer)
int promptInt(std::ostream& os, const std::string& prompt, std::istream& is) {
    int val;
    do {
        os << prompt;
    } while (!readIntLine(is, val));
    return val;
}

// If you wanted, you could take it a step farther and make convenience wrappers that assume std::cin/std::cout.
// Example:
int promptInt(const std::string& prompt) {
    return promptInt(std::cout, prompt, std::cin);
}

This looks like quite a bit of code (and it is -- unfortuntely IO just kind of sucks to do non-carelessly), but it makes reading in the data a bit cleaner while also providing verification of data:

MovieData getMovieData()
{
    MovieData movie;

    movie.title = promptLine("Enter the title of the movie: ");
    movie.directory = promptLine("Enter the name of the movie's director: ")
    movie.year_released = promptLineInt("Enter the year the movie was released: ");
    // ...

    return movie;
}

Other minor issues:

Using using namespace std; is a bad habit to get into.
Overusing std::endl can likewise be a bad habit as it not only writes a new line but also flushes the buffer. This can result in surprisingly bad performance when heavvy IO is involved. It's much better to use '\n' by default and only use std::endl when you specifically want a flush to happen (for example, your print function could use all \n and then have one std::endl at the end).
Some of your comments are a bit pointless and should be removed to reduce noise.
system("PAUSE") is a bad habit

As an aside, it might be worth noting that there's actually a super useful pattern of reading things from a stream. It's relatively poor performance considered to something specialized like std::strtol, but it can be quite handy if you're in a bind (i.e. can't use Boost or some other library) and need something highly generic:

template<typename T>
bool readLine(std::istream& is, T& val) {
    std::string line;
    if (!std::getline(is, line)) {
        // throw exception ...
    }
    std::istringstream ss(line);
    return (ss >> val);
}

It also might be worth noting that instead of a bool, functions like this will often return the input stream with certain bits set. In other words, some people prefer to set the fail bit instead of return false.


Optimizing iOS method in objective-c I have such method and have a hard time optimizing it to run better. It takes roughly 1 sec with 100 units to get between NSLog(@"Debug2") and NSLog(@"Debug3"). And 2 sec on iPhone.

Method idea:

Input:

NSMutableArray times - containing times (2012-12-12, 2013-01-19) and etc
NSMutableArray objectArray - contains LogUnit objects.

Output: Array of computed views.

Main idea: a view for each time with objects from objectArray whose time is the same.

I have made it faster by showing a small proportion of objects (see counted in code and _breakPlease), yet it's still not fast enough.

- (NSMutableArray*) sortViews: (NSMutableArray *)times and:(NSMutableArray *)objectArray
{
    NSLog(@"debug2");
    NSMutableArray *views = [[NSMutableArray alloc] initWithCapacity:times.count];

    NSString *number = [NSString stringWithFormat:@"SortLog%i ",[[NSUserDefaults standardUserDefaults] stringForKey:@"ObjectNumber"].intValue];

    NSString *comp = [[NSUserDefaults standardUserDefaults] stringForKey:number];

    LogUnit *unit;

    int counted = 0;
    for(int x = 0; x != times.count; x++)
    {
        @autoreleasepool {

        UIView *foo = [[UIView alloc] initWithFrame:CGRectMake(0, 0, 320, 400)];
        foo.backgroundColor = [UIColor clearColor];

        int f = 0;
        int h = 0;

        NSString *attributeName = @"realTime";
        NSPredicate *predicate = [NSPredicate predicateWithFormat:@"%K IN %@", attributeName, [times objectAtIndex:x]];
        // Filter the objectArray by the time, so we don't have to run through all the objects in objectArray, and check if time is right.
        NSArray *filtered  = [objectArray filteredArrayUsingPredicate:predicate];

        for(int i = 0; i != filtered.count; i++)
        {
                unit = [filtered objectAtIndex:i];

                // Filter units by user choice (comp). Like comp = Warnings or comp = Errors and etc.
                if([[unit status] isEqualToString:comp] || [comp isEqualToString:NULL] || comp == NULL)
                {
                     // testing if the unit is correct to use
                    if([unit getEvent] != NULL)
                    {
                    // below is some computation for frames, to get them to proper position
                        unit.view.frame = CGRectMake(unit.view.frame.origin.x, f * unit.view.frame.size.height + 30, unit.view.frame.size.width, unit.view.frame.size.height);
                        [foo addSubview:unit.view];
                        counted++;
                        f++;
                        h = unit.view.frame.size.height * f;
                    }
                }
        }

        UILabel *myLabel = [[UILabel alloc] initWithFrame:CGRectMake(0, 0, 320, 30)];
        myLabel.backgroundColor = [UIColor clearColor];
        myLabel.font = [UIFont boldSystemFontOfSize:20];
        myLabel.textColor = [UIColor colorWithRed:88 green:154 blue:251 alpha:1];
        myLabel.textAlignment = UITextAlignmentCenter;
        myLabel.text = [times objectAtIndex:x];
        // Just adding a label with "2012-12-30" or etc on top of the view.
        [foo addSubview:myLabel];

        foo.frame = CGRectMake(0,0, 320, h + 30 );
        h = 0;
        [views addObject:foo];

        // counting added views for performance, if more than 30, return, and show only small portion of whole units, user has the option to show all the units if he wants to, but that takes a time to load..
        if(counted > 30 && filtered.count != counted)
        {
            if(_breakPlease == false)
            {
                _broken = true;
                break;
            }
        }
       }
    }
    NSLog(@"debug3");
    return views;
}

times.count in for(int x = 0; x != times.count; x++) is a method call, move it to a variable. Also, you should probably use the < instead of != as it is more common.

However, as you don't really need the index of the element, you can replace both iterations

for(int i = 0; i != filtered.count; i++) {
    unit = [filtered objectAtIndex:i];

by native iteration

for (LogUnit* unit in filtered) {

Is @autoreleasepool necessary in every iteration? Having it outside for should help performance as well. In most application you are not required to use @autoreleasepool at all.

NSString *attributeName = @"realTime";
NSPredicate *predicate = [NSPredicate predicateWithFormat:@"%K IN %@", attributeName, [times objectAtIndex:x]];
// Filter the objectArray by the time, so we don't have to run through all the objects in objectArray, and check if time is right.
NSArray *filtered  = [objectArray filteredArrayUsingPredicate:predicate];

Your filtering doesn't do what your comment describes. You don't want to run through all the objects but that's exactly the thing filteredArrayUsingPredicate: does. Only using reflexion and moving the objects into a new array. By using the filtering code, you are losing performance.

There are various code quality issues not related to performance:

usage of variable names x, f, h, objectArray. Use more descriptive names or use i, j, k for iterators.
Move some code to separate methods, e.g. view generation.
Don't use NULL in Obj-C, use nil. NULL is to be used only when you interact with C pointers, not Obj-C objects.
[comp isEqualToString:NULL] will never evaluate to YES (true)
if(_breakPlease == false) Don't use false in Obj-C. Use YES or NO. Never ever compare a BOOL with a bool literal. Use logical operators instead, e.g. if (!_breakPlease).

Algorithm performance:

The main problem of your algorithm is its complexity. Basically you have two arrays and you compare objects from an array with all objects from the other array. That's O(n^2)

There are many ways how you could improve this, I will list some of them:

Don't use a NSArray to store the log units. Use a NSDictionary where time is the key and log unit, or possibly array of log units with the same time, is the value. Finding log units with the given time will be trivial then.
Sort times and objectArray by time. You can then iterate through both arrays at the same time and you get O(n + n * log n).


Showing all records on a webpage I have a connection with my database to show all my records on a webpage, and I'm not sure if this code is safe:

define('DB_HOSTNAME', 'localhost');
define('DB_USERNAME', 'root');
define('DB_PASSWORD', null);
define('DB_DATABASE', 'publicacoes');
define('DB_PREFIX', 'bn');
define('DB_CHARSET', 'utf8');

function DBEscape($dados){
    $link = DBConnect();

    if(!is_array($dados))
        $dados = mysqli_real_escape_string($link, $dados);
    else{
        $arr = $dados;

        foreach ($arr as $key => $value) {
            $key = mysqli_real_escape_string($link, $key);
            $value = mysqli_real_escape_string($link, $value);

            $dados[$key] = $value;
        }
    }

    DBclose($link);
    return $dados;
}

function DBclose($link){
    @mysqli_close($link) or die(mysqli_error($link));
}

function DBConnect(){
    $link = @mysqli_connect(DB_HOSTNAME, DB_USERNAME, DB_PASSWORD, DB_DATABASE) or die(mysqli_connect_error());
    mysqli_set_charset($link, DB_CHARSET) or die(mysqli_error($link));

    return $link;
}

function DBRead($table, $params = null, $fields = '*'){
    $table  = DB_PREFIX.'_'.$table;
    $params = ($params) ? " {$params}" : null;

    $query  = "SELECT {$fields} FROM {$table}{$params}";
    $result = DBExecute($query);

    if(!mysqli_num_rows($result))
        return false;
    else{
        while ($res = mysqli_fetch_assoc($result)){
            $data[] = $res;
        }
        return $data;
    }
}

function DBExecute($query, $insertId = false){
    $link   = DBConnect();
    $result = @mysqli_query($link, $query) or die(mysqli_error($link));

    if($insertId)
        $result = mysqli_insert_id($link);

    DBClose($link);
    return $result;
}

$dadosIDrow1 = DBRead('publicacao', "ORDER BY id DESC LIMIT 2");

<?php foreach ($dadosUN as $UN): ?>

//some html here to display the records

<?php endforeach; ?

This is the first piece of code that I've written to show the records on my website. At the moment I have 2 connections with my DB. One of them I use PDO prepared statement to a pagination and search box.

I want to know if I should delete this script and make a new one with prepared statements.

I want to know if I should delete this script and make a new one with prepared statements.

Yes, definitely.
This set of functions is just unacceptable, being so bad on so many levels.

There will be dozens to hundreds connections made in your script, not just "2 connections" for starter, as your code will connect anew every time it executes a query.
There is, as you rightfully noted, no support for prepared statements, which is a disaster alone. That escaping routine is based on the false notion that mysqli_real_escape_string's purpose is to protect you from SQL injection. Well, it's but a grave delusion.
error reporting is completely flawed. What a site user is supposed to do with a blank page telling them that a column not found?
a DBRead() function is silly. All its purpose is to save you a typing of two words. Seriously? And this trifle gain at the expense of making a gibberish out of the powerful language of SQL.

To show 2 records from a table (as it's stated in the code as opposed to the question title) you need only two lines with vanilla PDO, so you better stick with it:

$sql = 'SELECT * FROM publicacao ORDER BY id DESC LIMIT 2';
$dadosUN = $pdo->query($sql)->fetchAll();
?>
<?php foreach ($dadosUN as $UN): ?>

//some html here to display the records

<?php endforeach ?>


Table View Controller class in Swift My TableView consists of four cells (for blacklisting categories) with a boolean property (indicated with a checkmark) and another cell with a UISwitch embedded in it. My current controller is as follows:

class TableViewController: UITableViewController {
    var blacklisted = Set<Category>()
    var flags = Set<String>()

    @IBAction func toggle(_ sender: UISwitch) {
        if (sender.isOn) {
            print("if")
        } else {
            print("else")
        }
    }

    @IBAction func cancel(_ sender: UIBarButtonItem) {
        self.dismiss(animated: true)
    }

    @IBAction func done(_ sender: UIBarButtonItem) {
        ViewController.allowed = ViewController.allowed.subtracting(blacklisted)
        ViewController.flags = ViewController.flags.union(flags)

        print(ViewController.allowed)
        print(ViewController.flags)
        self.dismiss(animated: true)
    }

    var states = [PastState.UNSELECTED, PastState.UNSELECTED, PastState.UNSELECTED, PastState.UNSELECTED]

    override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
        if (indexPath.section != 0) {
            return
        }

        tableView.cellForRow(at: indexPath)?.accessoryType
                = (states[indexPath.row] == PastState.SELECTED)
                ? .none
                : .checkmark

        switch (indexPath.row) {
            case 0:
                TableViewController.toggleItem(if: states[0], set: &flags, with: "nsfw")
                break

            case 1:
                TableViewController.toggleItem(if: states[1], set: &flags, with: "religious", "political")
                break

            case 2:
                TableViewController.toggleItem(if: states[2], set: &blacklisted, with: Category.Programming)
                break

            case 3:
                TableViewController.toggleItem(if: states[3], set: &blacklisted, with: Category.Dark)
                break

            default: break
        }

        states[indexPath.row] = (states[indexPath.row] == PastState.SELECTED)
                ? PastState.UNSELECTED
                : PastState.SELECTED
    }

    static func toggleItem<T: Hashable>(`if` state: PastState, set: inout Set<T>, with items: T...) {
        if (state == PastState.UNSELECTED) {
            for item in items {
                set.insert(item)
            }
        } else {
            for item in items {
                set.remove(item)
            }
        }
    }

    override func viewDidLoad() {
        super.viewDidLoad()
    }
}

I've been told the code is spaghetti code and to separate the model, but I'm not quite too sure how.

There are quite a few issues here:

In switch statement, you don’t need/want break statements. Unlike C languages, Swift switch statements don’t fallthrough by default. To use break where it is not needed is very unswifty.
I’d make toggleItem an instance method.
The properties of the ViewController should not be static.
This table view controller should not be reaching into ViewController to update its properties, which tightly couples these two classes. We should strive for far looser coupling in our code.

Admittedly, because ViewController is presenting TableViewController, the presenting view controller might go ahead and update properties in the presented view controller, in order to pass data to down the chain. But TableViewController should never be reaching back up the chain. That makes them too tightly coupled and only allows TableViewController to be called from that one view controller, whereas as your app grows, you might want to call it from elsewhere.

The typical solution to achieve loose coupling is the delegate-protocol pattern. Presented view controllers should not updating properties in the presenting view controller, but rather it should only communicate back via its protocol. This allows us to use this presented view controller from wherever we want.
If you’re going to have an early exit in didSelectRowAt (if section ≠ 0), all things being equal, you should prefer guard over if, as it makes the intent more clear (you see guard keyword and you immediately know “oh, this is an early exit for some edge case”). The compiler will also warn us if we accidentally forget to call return.
In if statements, the use of parentheses around the test clause feels a bit unswifty. Consider:

if (state == PastState.UNSELECTED) {
    ...
} else {
    ...
}

Instead, I’d suggest:

if state == PastState.UNSELECTED {
    ...
} else {
    ...
}

In the case of an enumeration, I’d favor switch over if. Thus, instead of the above, I’d suggest:

switch state {
case PastState.UNSELECTED:
    ...

case PastState.SELECTED:
    ...
}

This ensures that the test is exhaustive. It makes it easier to reason about the code at a glance. Plus, if you, for example, later add a .NOTDETERMINED enumeration, the compiler can warn you of all of the non-exhaustive switch statements, but if you use if-else, you have to manually pour through all of your code looking for this yourself.
Because PastState is an enumeration, you can eliminate all of those redundant references to the enumeration name. E.g., using the previous example, you can say:

switch state {
case .UNSELECTED:
    ...

case .SELECTED:
    ...
}

In your enumerations, you should use camelCase (start with lower case letter, only capitalize start of new word within the case name):

switch state {
case .unselected:
    ...

case .selected:
    ...
}

Alternatively, just retire this enumeration altogether and just use a [Bool] property. Or, as shown at the end of this answer, if you use a view model, you might refactor this out completely.
The blacklisted property is using an enumeration, which is good. But flags is using strings with values buried in the code. They both should be enumerations.
The done method is subtracting blacklisted from the ViewController’s allowed. But what if you unselected a blacklisted item and needed to add it back to the allowed? The existing code won’t do that.

The same issue applies for ViewController.flags, where you’re adding flags. But what if one was removed?
You are setting the accessoryView based upon which flags/blacklisted values exist in these sets and toggle this based upon selecting a row. But you then have this UISwitch, too. You’d generally use either a UISwitch or the accessoryView, but not both. Pick one or the other. (In my demo, I’m going to retire UISwitch.)
The references to self.dismiss(...) can be simplified to just dismiss(...).
The didSelectRowAt is:

toggling the indicatorView based upon the old state;
updating the blacklist/flags properties; and
toggling the state.

I would suggest that you simply want to toggle the state and then reload that row. Not only does it make this less fragile (you don’t have three different operations that you need to carefully coordinate), but it affords the opportunity to animate the change of the indicatorView.
I would suggest grouping your view controller methods into extensions. For example, I find it nice to put UITableViewDataSource methods in one extension, UITableViewDelegate methods in another, @IBAction methods in another, etc. This makes it easy to visually identify/find relevant methods. This also allows you to collapse sections (e.g. ⌘+option+◀) sections that you’re not currently working on, making it easy to focus on the work at hand. This, combined with use of the MARK: - designator makes navigation through your view controller easier.
You’ve got an enumeration called PastState. But this is not the “past state”, but rather the current state as updated as the user proceeds with selecting/unselecting various options. So I might rename that SelectedState or something like that.

Likewise, I might give TableViewController a name that indicates its purpose. E.g. maybe AllowedContentViewController.
I find it confusing that the main view controller uses allowed, but the TableViewController uses blacklisted (which is presumably an inverted set of the aforementioned). I’d suggest using the same model in both view controllers. If TableViewController wants to invert it as part of its determination of what’s checked and what’s not, then it should do that.
The most radical change I’m going to suggest is that you should completely remove the business logic from the view controller. The view controller should be handling the interface with the UIKit controls (the table view, the buttons, etc.), but all the logic about which flags and Category values is represented by which row, how you toggle them, etc., doesn’t belong here. You often put it in what I will call a “view model” (though this entity goes by different names)

The virtue of this is two fold. First the view controller suddenly becomes something very concise and easy to understand. Second, when you get around to writing unit tests, you want to be able to test your business logic without worrying about UIKit stuff (which should be part of the UI tests, not the unit tests).

So, pulling this all together, we end up with a view controller like this:

class AllowedContentViewController: UITableViewController {

    // properties set by presenting view controller

    weak var delegate: AllowedContentViewControllerDelegate?
    var allowed: Set<Category>!
    var flags: Set<Flag>!

    // properties used by this view controller

    private var viewModel: AllowedContentViewModel!

    override func viewDidLoad() {
        super.viewDidLoad()

        viewModel = AllowedContentViewModel(allowed: allowed, flags: flags)
    }

}

// MARK: - Actions

extension AllowedContentViewController {
    @IBAction func cancel(_ sender: UIBarButtonItem) {
        dismiss(animated: true)
    }

    @IBAction func done(_ sender: UIBarButtonItem) {
        delegate?.allowedContentViewController(self, didUpdateAllowed: viewModel.allowed, flags: viewModel.flags)
        dismiss(animated: true)
    }
}

// MARK: - UITableViewDataSource

extension AllowedContentViewController {
    // you didn't provide the following two, so I'll take a guess what that looked like

    override func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return viewModel.numberOfRows()
    }

    override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
        let cell = tableView.dequeueReusableCell(withIdentifier: "Cell", for: indexPath)

        cell.textLabel?.text = viewModel.text(for: indexPath.row)
        cell.accessoryType = viewModel.isSelected(row: indexPath.row) ? .checkmark : .none

        return cell
    }
}

// MARK: - UITableViewDelegate

extension AllowedContentViewController {
    override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
        guard indexPath.section == 0 else {
            return
        }

        viewModel.toggle(row: indexPath.row)
        tableView.reloadRows(at: [indexPath], with: .fade)
    }
}

That references a protocol to which our presenting view controller will conform:

protocol AllowedContentViewControllerDelegate: class {
    func allowedContentViewController(_ viewController: AllowedContentViewController, didUpdateAllowed allowed: Set<Category>, flags: Set<Flag>)
}

The presenting view controller would then do something like so:

class ViewController: UIViewController {

    var allowed = Set<Category>()
    var flags = Set<Flag>()

    override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
        if let destination = (segue.destination as? UINavigationController)?.topViewController as? AllowedContentViewController {
            destination.flags = flags
            destination.allowed = allowed
            destination.delegate = self
        }
    }
}

extension ViewController: AllowedContentViewControllerDelegate {
    func allowedContentViewController(_ viewController: AllowedContentViewController, didUpdateAllowed allowed: Set<Category>, flags: Set<Flag>) {
        self.allowed = allowed
        self.flags = flags
    }
}

I obviously made an assumption here of how you presented this AllowedContentViewController (namely that you must have embedded it in a UINavigationController, based upon the presence of dismiss but that you also had @IBActions with UIBarButton). Clearly, adjust the prepare(for:sender:) according to how you actually presented this table view controller.

Anyway, above, the presenting view controller is responsible for passing data to this presented AllowedContentViewController and for responding to AllowedContentViewControllerDelegate delegate method that supplies the updated user preferences for what is allowed and what isn’t. This gets the AllowedContentViewController from ever having to reach into ViewController to retrieve/update properties itself. Now AllowedContentViewController can be used in any context you want.

Finally, our view model has all the business logic that used to be in the TableViewController:

struct AllowedContentViewModel {
    var allowed: Set<Category>
    var flags: Set<Flag>

    func numberOfRows() -> Int {
        return 4
    }

    func isSelected(row: Int) -> Bool {
        switch row {
        case 0:  return flags.contains(.nsfw)
        case 1:  return flags.contains(.religious) && flags.contains(.political)
        case 2:  return !allowed.contains(.programming)
        case 3:  return !allowed.contains(.dark)
        default: fatalError("Unexpected row number")
        }
    }

    private mutating func select(row: Int) {
        switch row {
        case 0:  flags.insert(.nsfw)
        case 1:  flags.insert(.religious); flags.insert(.political)
        case 2:  allowed.remove(.programming)
        case 3:  allowed.remove(.dark)
        default: fatalError("Unexpected row number")
        }
    }

    private mutating func unselect(row: Int) {
        switch row {
        case 0:  flags.remove(.nsfw)
        case 1:  flags.remove(.religious); flags.remove(.political)
        case 2:  allowed.insert(.programming)
        case 3:  allowed.insert(.dark)
        default: fatalError("Unexpected row number")
        }
    }

    mutating func toggle(row: Int) {
        if isSelected(row: row) {
            unselect(row: row)
        } else {
            select(row: row)
        }
    }

    func text(for row: Int) -> String {
        switch row {
        case 0: return NSLocalizedString("NSFW", comment: "Label")
        case 1: return NSLocalizedString("Religious/political", comment: "Label")
        case 2: return NSLocalizedString("Blacklisted Programming", comment: "Label")
        case 3: return NSLocalizedString("Blacklisted Dark", comment: "Label")
        default: fatalError("Unexpected row number")
        }
    }
}

We can, if we want, then write unit tests for this view model. For example:

class AllowedContentViewModelTests: XCTestCase {

    func testDefaultState() {
        let viewModel = AllowedContentViewModel(allowed: [], flags: [])

        XCTAssert(!viewModel.isSelected(row: 0))
        XCTAssert(!viewModel.isSelected(row: 1))
        XCTAssert(viewModel.isSelected(row: 2))
        XCTAssert(viewModel.isSelected(row: 3))
    }

    func testTogglingNsfw() {
        var viewModel = AllowedContentViewModel(allowed: [], flags: [.nsfw])

        XCTAssert(viewModel.isSelected(row: 0))

        viewModel.toggle(row: 0)

        XCTAssert(!viewModel.isSelected(row: 0))
    }

}

See example at https://github.com/robertmryan/RefactoredViewController.


Basic tic-tac-toe matrix in Haskell Beginner functional programmer... (but not beginner programmer)

Currently, I have the following code:

import Control.Monad (mapM_)

main = gridMaker 3

gridMaker :: Int -> IO ()
{-Creates a nxn matrix filling the lower triangle with "O"'s
  and filling the rest with "X"s
  Example:
  O --- X --- X
  O --- O --- X
  O --- O --- O
  -}
gridMaker gridSize =
  -- Creates the indicators
  let startingIter = 1
      indicators = indicatorListCreator gridSize startingIter
  -- Print each indicator out to IO
  in mapM_ linePrinter indicators

indicatorListCreator :: Int -> Int -> [[String]]
{- Build the indicators of
   [["O", "X", "X"],["O", "O", "X"] ... and so forth.
   Recursively determines how many iterations we've been through,
   and determines how many "X"s and "O"s we should have
   per each line. -}
indicatorListCreator gridLen iterNum
  | iterNum > gridLen = []
  | otherwise =
    let itersRemaining = gridLen - iterNum
        indicator = replicate iterNum "O" ++
                    replicate itersRemaining "X"
    in
      indicator: indicatorListCreator gridLen (iterNum + 1)

linePrinter :: [String] -> IO ()
{- Takes the indicators and prints each line accordingly. -}
linePrinter [indicator1, indicator2, indicator3] =
  let between = " --- "
      outString = indicator1 ++ between ++
                  indicator2 ++ between ++
                  indicator3
  in putStrLn outString
linePrinter _ = error"Stupidly hardcoded to only show 3x3"

Running and compiling this code results in:

O --- X --- X
O --- O --- X
O --- O --- O

Some of my thoughts...

Is it possible to build indicatorListCreator out of folds?
How can I circumvent hardcoding linePrinter
Is this optimal?
Where should I improve on coding conventions / style?
Any other glaring shortcomings?

Thank you in advance!

I like that you are trying to not use explicit recursions everywhere. That being said, there are a few standard functions that we could use to get a code similar to the following.

import Data.List(intercalate)

grid :: Int -> [String]
grid n = [intercalate " --- " . take n . drop (n-k) $ osxs | k <- [1..n]]
    where osxs = replicate n "O" ++ replicate n "X"

printGrid :: Int -> IO ()
printGrid = putStr . unlines . grid

Most notably, Data.List.intercalate can be used to generalize your linePrinter, and unlines helps us avoid mapM_.

If you are writing a game, then a Data.Map.Strict.Map could be useful for storing the board's current values.


Efficient Ring Buffer (FIFO) I need to implement a Ring Buffer/FIFO for data coming from a TCP socket.

It must support the following operations:

Append a the recv()'ed chunk of bytes.
Allow me to peek at the beginning of the buffer, since i get differently-sized packets, and I must decode a small fixed-size header to know how many bytes to process.
Remove a chunk of bytes from the beginning of the buffer for processing.

I guess my needs are pretty standard for a TCP-based streaming protocol, but surprisingly I have never found a "best practice" method for doing this.

There are similar questions on SO already, most suggest to use collections.deque, which is fine but has some shortcomings that must be worked around for my needs:

It doesn't easily allow peeking.
It doesn't allow removal of chunks of bytes.

Mixing various suggestions I came up with the following implementation, which seems working but I wonder: can I do any better, performance-wise? Removing a single byte at a time in get() doesn't look optimal at all.

import collections
import itertools

class RingBuffer (object):
    """Ring buffer"""
    def __init__ (self, size = 4096):
            self._buf = collections.deque (maxlen = size)

    def put (self, data):
            """Adds data to the end of the buffer"""
            self._buf.extend (data)

    def get (self, size):
            """Retrieves data from the beginning of the buffer"""
            data = str ()
            for i in xrange (size):
                    data += self._buf.popleft ()
            return data

    def peek (self, size):
            """\"Peeks\" at the beginning of the buffer (i.e.: retrieves data without removing them from the buffer)"""
            return str (bytearray (itertools.islice (self._buf, size)))

    def len (self):
            """Returns the length of the buffer"""
            return len (self._buf)

If you are wondering why I am returning a string from peek(), that is because I need to process its return value with struct.unpack_from().

I suspect you’re stuck using one-pop-at-a-time if you want to get multiple elements at once.

I found this quote from one of the collections developers on Stack Overflow about using pop() for many elements in a deque:

  There is no multi-pop method for deques.

The OP on that question had a very similar construction to you (they were saving the data to a list rather than a str), and he said:

  Yes, it is correct. Yes, it is reasonably efficient though it can be further sped-up with boundmethods and itertools. No, it isn't stupid :-)

I don’t know enough about boundmethods or itertools to know what he was suggesting.

That doesn’t seem particularly helpful, so here are a few other comments on your general coding style:

You shouldn’t have a space immediately before the opening parens that starts a list of arguments to a function. (PEP 8: Pet Peeves)
It’s also recommended that you don’t have a space around the equals sign for keyword arguments or default values. (PEP 8: Other Recommendations)
In the interest of using descriptive variable names, I see no reason not to extend the length of the variable name _buf to _buffer. Characters are cheap.
Lines should be limited to 79 characters (PEP 8: Maximum Line Length), which includes docstrings: specifically, the docstring for peek().
If you have a single double-quote within a triple-quoted string, you don’t need to escape it. You only need to escape quotes within a triple-quoted string if you have three of them at once.
The docstring for RingBuffer() is almost useless. It tells me nothing that I couldn’t have learnt from the name of the class.

And a few comments on your get() method in particular:

In the for loop, your index variable is named i, but you never use the value of this variable. It’s common practice to give unused index variables the name _, to make it explicit that the value of this variable is unimportant. That is,

for _ in xrange(size):
    data += self._buf.popleft()

It used to be the case that it was better to construct a list of strings and then call join() on the list than be continually appending to strings, but I think that’s fixed in newer versions of Python.

Regardless, I happen to have a personal preference for that construction. It makes the method slightly shorter:

def alt_get(self, size):
    """Retrieves data from the beginning of the buffer."""
    data = ''.join(self._buf.popleft() for _ in xrange(size))
    return data

But I think that’s entirely personal preference: I did an short performance test, and I couldn’t see any difference between this and your get() method.
Suppose I have an empty ring buffer, and I try to get() an element from it:

r = RingBuffer()
r.get()

The user sees the IndexError that arises from trying to pop an empty deque:

Traceback (most recent call last):
  File "buffer.py", line 29, in <module>
    r.get(1)
  File "buffer.py", line 17, in get
    data += self._buf.popleft ()
IndexError: pop from an empty deque

Unless you knew that RingBuffer() was using a deque under the hood, this error message would be confusing. I’d recommend having a custom error message to explain that somebody’s trying to get more items than you have to give.

It’s also worth noting that if you use a try … except block here, the buffer will be emptied before you notice, and the contents could be lost. You need a check before you access anything from the buffer. Something like this:

def get(self, size):
    """Retrieves data from the beginning of the buffer"""
    if size > len(self._buf):
        raise IndexError("Too many items: trying to access %d items "
                         "from a buffer of length %d" % (size, len(self)))
    data = [self._buf.popleft() for _ in xrange(size)]
    return data

Your peak() method doesn't suffer from this problem.


How can I streamline my code? I'm fairly new to Javascript and am writing a piece of code to create a filtering system for a portfolio grid (a grid of work examples that filter by category when a button is selected).

While the code works just fine, I feel like the way I'm writing it is overly clunky. Can someone please let me know if there's a way I can streamline/improve this code block?

Any help would be much appreciated - thanks! :D

var gridItem = document.querySelectorAll('.grid div');
var featuredItem = document.querySelector('.featured');
var filterBtn = document.querySelectorAll('.filter-btn');

function getAttributes(event) {
  event.preventDefault();

  var thisValue = event.target.getAttribute('data-filter');

  // Get grid item filter attributes
  for (let a = 0; a < gridItem.length; a++) {
    const gridFilterAttr = gridItem[a].getAttribute('data-filterType');
    if (thisValue == gridFilterAttr) {
      gridItem[a].classList.add('active')
      gridItem[a].classList.remove('not-active')
    } else if (thisValue == 'all') {
      gridItem[a].className = 'active';
      featuredItem.className = 'featured active'

    } else {
      gridItem[a].classList.remove('active')
      gridItem[a].classList.add('not-active')
    }
  }
}

for (let b = 0; b < filterBtn.length; b++) {
  filterBtn[b].addEventListener('click', getAttributes);

}

HTML:

<!DOCTYPE html>

  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Js Projects</title>
  <link rel="stylesheet" href="/assets/css/app.css">

    <button data-filter="all" class="filter-btn">All</button>
    <button data-filter="web" class="filter-btn">Web Project</button>
    <button data-filter="app" class="filter-btn">App</button>
    <button data-filter="ux" class="filter-btn">UX/UI</button>

Featured Item

Web item

App item

UX item

Web item

UX item

App item

As mentioned in comment, it is harder to be more specific without being able to run the code, but what can I say:

Separate event code function and then the function, that will "do the work" itself, with all necessary parameters.
Extract for into separate function.

That will overall make it easier to read and cleaner + easier to test. Something like:

function getAttributes(event) {
  event.preventDefault();
  var thisValue = event.target.getAttribute('data-filter');
  setGridItems(gridItem, thisValue);
}

function setGridItems(gridItems, dataFilter) {
  for (let a = 0; a < gridItems.length; a++) {
    setGridItem(gridItems[a], dataFilter);
  }
}

function setGridItem(singleGridItem, dataFilter) {
    const gridFilterAttr = singleGridItem.getAttribute('data-filterType');
    if (dataFilter == gridFilterAttr) {
      singleGridItem.classList.add('active')
      singleGridItem.classList.remove('not-active')
    } else if (dataFilter == 'all') {
      singleGridItem.className = 'active';
      featuredItem.className = 'featured active'
    } else {
      singleGridItem.classList.remove('active')
      singleGridItem.classList.add('not-active')
    }

}

Would be cool to use foreach there, but it is a bit problematic with NodeList and support is not that broad yet, but maybe you could do that. Now when you have it a bit separated, you can continue with refactoring. I don't like that some variables are pre-cached and "global", that's why I passed gridItem variable anyway to make it local in function, but featuredItem is still global.
Last function could use more of refactoring, but I'd need more info about problem for that :-)


BigDecimal wrapper that ignores zero operands I want to perform mathematical operations on a range of values, where zeros should be ignored. This means that multiplication by zero does not result in zero but returns the first operand and likewise (\$5x0=5\$), division by zero does not throw an ArithmeticException but returns the first operand (\$5/0=5\$).

I would like to avoid testing for zero every time I need to make a calculation and would prefer that the calculation logic takes care of it.

My question is about the best way of approaching this problem. I will be using BigDecimal and thought of a simple implementation of the decorator pattern that adds functionality to the multiply and divide methods.

With this in mind, I developed the following code snippet:

public class BigDecimalWrapper extends BigDecimal {

public BigDecimalWrapper multiply(BigDecimal augend){
    if(augend.compareTo(BigDecimal.ZERO) != 0){
        return new BigDecimalWrapper(super.multiply(augend).doubleValue());
    } else {
        return this;
    }
}

public BigDecimalWrapper divide(BigDecimal augend){
    if(augend.compareTo(BigDecimal.ZERO) != 0){
        return new BigDecimalWrapper(super.divide(augend).doubleValue());
    } else {
        return this;
    }
}

public BigDecimalWrapper add(BigDecimal augend){
    return new BigDecimalWrapper(super.add(augend).doubleValue());
}

public BigDecimalWrapper subtract(BigDecimal augend){
    return new BigDecimalWrapper(super.subtract(augend).doubleValue());
}

// Some code omitted for brevity.
}

I expect to use it like this:

@Test
public void shouldMultiply() {
    BigDecimalWrapper v = new BigDecimalWrapper(10).multiply(BigDecimal.ZERO);
    assertThat(v).isEqualTo(new BigDecimal(10));
}

@Test
public void shouldDivide() {
    BigDecimalWrapper v = new BigDecimalWrapper(10).divide(BigDecimal.ZERO);
    assertThat(v).isEqualTo(new BigDecimal(10));
}

One of the problems that this is intended to resolve is in the calculation of quantity where it is not known if it is required to calculate length, area or volume. All that is known at the time of calculation are the dimensions. We may have only length, or length and width or height, width and length, therefor height may be 0 and while width and length are given. I would hope to be able to calculate with just one line of code as follows:

new BigDecimalWrapper(width).multiply(height).multiply(length);

Therefore, if height is zero, it will effectively ignore it and multiply width and length.

There are other similar situations where it will be used in this manner and is therefore worth the effort developing a solution for it.

Is this the best way to approach this problem? What alternative ways should I consider?

There are big problems with your approach.

Cloning BigDecimal with .doubleValue() is broken

Cloning a BigDecimal bd using new BigDecimal(bd.doubleValue()) doesn't work in most practical cases.
For example, try this code:

BigDecimal bd = BigDecimal.valueOf(0.02d);
BigDecimal clone = new BigDecimal(bd.doubleValue());
System.out.println(bd);
System.out.println(clone);
System.out.println(bd.equals(clone));

On my computer the above code prints:

0.02
0.0200000000000000004163336342344337026588618755340576171875
false

The equality test fails for as simple values as 2d. It passes for a simple 2, but that's not very reassuring.

As such, your current implementation of cloning doubles is effectively broken, for example this test case fails:

@Test
public void multiply_2d_with_3d() {
    BigDecimalWrapper v = new BigDecimalWrapper(2d).multiply(BigDecimal.valueOf(3d);
    assertThat(v).isEqualTo(new BigDecimal(2d).multiply(BigDecimal.valueOf(3d));
}

You can fix it by replacing .doubleValue() with .toString(),
the unit test will pass.

Cloning BigDecimal is a strange thing to do

BigDecimal instances are immutable, so you can safely pass around copies.
Creating new instances is completely unnecessary.
Your current approach forces you to clone.
Since cloning is non-sense, this indicates a code smell,
and that a better solution is probably available.

Composition over inheritance

A better approach would be to avoid the strange cloning, by making the decorated object a final field in your wrapper class. Add a constructor that takes a BigDecimal, and call that one instead of super.

By the way, putting the decorated object into a field of the decorator class is the common technique when implementing this pattern (composition instead of inheritance). It makes it possible to nest multiple decorators in arbitrary order. Inheritance is a very tight coupling, composition gives you much more flexibility.

For example something like this:

class BigDecimalWrapper {

    private final BigDecimal bigDecimal;

    public BigDecimalWrapper(double num) {
        bigDecimal = new BigDecimal(num);
    }

    public BigDecimalWrapper(BigDecimal bigDecimal) {
        this.bigDecimal = bigDecimal;
    }

    public BigDecimalWrapper multiply(BigDecimal augend) {
        if (augend.compareTo(BigDecimal.ZERO) != 0) {
            return new BigDecimalWrapper(bigDecimal.multiply(augend));
        } else {
            return this;
        }
    }

    public BigDecimal bigDecimalValue() {
        return bigDecimal;
    }
}

Since the class no longer extends BigDecimal,
I added a bigDecimalValue() method to get access to the wrapped object.
The unit tests can be modified to work with this, and they pass:

@Test
public void shouldMultiply() {
    BigDecimalWrapper v = new BigDecimalWrapper(10).multiply(BigDecimal.ZERO);
    assertThat(v.bigDecimalValue()).isEqualTo(new BigDecimal(10));
}

@Test
public void multiply_2d_with_3d() {
    BigDecimalWrapper v = new BigDecimalWrapper(2d).multiply(BigDecimal.valueOf(3d);
    assertThat(v.bigDecimalValue()).isEqualTo(new BigDecimal(2d).multiply(BigDecimal.valueOf(3d));
}

You might not like that fact the BigDecimalWrapper is no longer a BigDecimal.
But maybe that's a good thing.
With your implementation,
if somebody ever tries to use a BigDecimalWrapper instance as a BigDecimal,
he might be surprised that multiplication with zero doesn't work as expected from a BigDecimal.
For this reason I think it's a good thing for BigDecimalWrapper to not pretend to be a BigDecimal.

If, however, you really want BigDecimalWrapper to be a BigDecimal,
then the most sensible solution is to keep your original implementation,
with doubleValue() replaced with toString().


Iteration to recursive function In my first iterative algorithm, I do this

for (auto &widget : controls.getWidgets())
{
    if (!widget->visible) continue;

    widget->draw();

    for (auto &widget_component : widget->components)
    {
        if (!widget_component->visible) continue;

        widget_component->draw();

        for (auto &ww : widget_component->components)
        {
            if (!ww->visible) continue;

            ww->draw();
        }
    }
}

Now, I see the flaw in this algorithm because of the repetition of code.

I try to write a recursive function call to handle this thing.

void swcApplication::recursiveDisplay(swcWidget *next)
{
    if (next == nullptr) return;

    if ((next->parent != nullptr) &&
        !next->parent->visible) return;

    if (next->visible)
        next->draw();

    for (auto &i : next->components)
    {
        recursiveDisplay(i);
    }
}

void display()
{
    for (auto &widget : controls.getWidgets())
    {
        recursiveDisplay(widget);
    }
}

Display first the parent before all other children, if parent is not visible, then don't draw its children

Is the above phrase satisfy by this algorithm (As far as I can see, it is)? Is this optimal? I don't know what drawbacks might occur here because I just write in a few seconds.

If you didn't find anything wrong here, now, how can I put it back in iterative way? I know iteration is better.

Update

I didn't use any z attribute here and instead I go for painter's algorithm.

The flaw I am referring in my iteration version is that, it is limited to draw widgets depends on how deep I will code for iteration.

You can modify the recursive code to use an explicit stack:

#include <stack>

void display_widgets(control& controls)
{
    std::stack<widget*> widgets;
    for(auto &widget : controls.getWidgets()) {
        widgets.push(widget);
    }

    while(!widgets.empty()) {
        widget* w = widgets.top();
        widgets.pop();
        if(!widget->visible) {
            continue;
        }
        widget->draw();
        for(auto& component : widget->components) {
            widgets.push(component);
        }
    }
}

  I know iteration is better.

This is highly dependent on language and usage. It is "better" in that recursion can use O(n) stack space (although not always - tail call optimization gets rid of this limitation), and won't blow your stack. However, in this case, assuming the recursive algorithm actually did what you wanted, it would be better and easier.

You can always turn a recursive solution into an iterative one by replacing the call stack with an actual stack of your own, as above. For very deeply nested recursive calls, this may save you from stack overflows, however, it is more fiddly to code, and in this case, doesn't gain you anything.

Having written the code above, my advice would be to stick with the recursive solution.


Find indices of white space characters in a string This function returns the indices of all white spaces as an array of Integer. It works fine with a small string:

func whiteSpacesIndices(value : String) -> Array<Int> {
    var indices: Array<Int> = []
    for (index, char) in value.enumerated() {
        if char == " " || char == "\n" || char == "\t" {
            indices.append(index)
        }
    }
    return indices
}

However when the string is too long, it could be very slow, because it is looping in every character.

Is there a better way for doing it?

General remarks

value, as a parameter name, isn't very descriptive,
The code considers that white spaces can only be " " or "\n" or "\t". This a performance optimization and supposes prior knowledge of the contents of the string. More generally you could make the check this way:

if char.isWhitespace {
    indices.append(index)
}

Array<Int> is not the same as [String.Index] of IndexSet. A String can be traversed using String.Index and not and Int.

Performance

The following codeis twice as fast in my tests, but doesn’t work with emoji :

func whiteSpacesIndices(in str : String) -> Array<Int> {
    var indices: Array<Int> = []
    let blanks: [UInt32] = [32, 10, 9]  //these values correspond to space, new line, and tabulation respectively.
    for (index, scalar) in str.unicodeScalars.enumerated() {
        if blanks.contains(scalar.value) {
            indices.append(index)
        }
    }
    return indices
}

You can learn more about the Unicode scalar representation here.

Free function or instance method?

The whiteSpacesIndices function seems more like a property on strings. It is appropriate for a String to know about the indices of white spaces (and new lines) within itself:

extension String {
    var whiteSpaceIndices: [Int] {
        var indices = [Int]()
        let blanks: [UInt32] = [32, 10, 9]
        for (index, scalar) in self.unicodeScalars.enumerated() {
            if blanks.contains(scalar.value) {
                indices.append(index)
            }
        }
        return indices
    }
}

And could be used like so:

"Hello world!".whiteSpaceIndices    //[5]
"ä ö ü".whiteSpaceIndices           //[1, 3]


Divide by two a number in base -2 I was given this coding challenge when a applying for a summer internship but didn't get selected for the next round and I was curious if it could be improved.

The problem says:

  Given a number in base "-2" divide it by 2.

  For example:

  Given [1, 0, 0, 1, 1, 1] (-23) the output is [1, 0, 1, 0, 1, 1] (-11)

  Given [0, 0, 1, 0, 1, 1] (-12) the output is [0, 1, 1, 1] (-6)

  Note the array is read from left to right. [1, 0, 0, 1, 1, 1] = 1*(-2)^0 + 0*(-2)^1 + 0*(-2)^2 + 1*(-2)^3 + 1*(-2)^4 + 1 * (-2)^5 = 1
  + 0 + 0 + (-8) + 16 + (-32) = -23

The easiest solution that I found was to convert the number to an integer, divide it by two and then convert it back to base -2.

Thanks in advance for your input.

My code in Python:

import math

# Main function
def solution(A):
    value = math.ceil(calculate_number(A) / 2)
    p = convert_number_to_binary(value)
    return p

# This function calculates the value of the list
def calculate_number(A):
    total = 0
    for i in range(0, len(A)):
        total += A[i] * (-2) ** i
    return total

# This function converts a number to base -2
def convert_number_to_binary(number):
    binary = []
    if number == 0:
        return binary

    while number != 0:
        number, remainder = divmod(number, -2)
        if remainder < 0:
            number += 1
            remainder -= -2
        binary.append(remainder)
    return binary

print(solution([1, 0, 0, 1, 1, 1]))

General

As per PEP 8 conventions, the variables in functions should be in lowercase.
Adding docstrings is recommended. PEP 257
Names should be short and meaningful. For example, convert_number_to_binary should actually be to_base_minus_two. Also, calculate_number should be from_base_minus_two
Use type hints
Instead of x == 0 python allows you to use use not x. Similarly, x != 0 can be replaced with x.
Envelop the main code inside if __name__ == '__main__':. This will prevent the code from being run if imported from another module.
Following  @Martin R's advice, you can change -2 to a constant BASE

Function solution

value = math.ceil(from_base_minus_two(A) / 2)
p = to_base_minus_two(value)
return p

Can be replaced with the one liner:

return to_base_minus_two(math.ceil(from_base_minus_two(a) / 2))

Function from_base_minus_two

total = 0
for i in range(0, len(a)):
    total += a[i] * BASE ** i
return total

Instead, you can use:

return sum(j * BASE ** i for i, j in enumerate(a)))

Function to_base_minus_two

The value you are finding isn't exactly binary. Change it to base_minus_two

You don't need to use

if number == 0:
    return base_minus_two

Automatically, the while loop will terminate immediately as number is 0, and will return base_minus_two which is []

Here's what the final code might look like:

import math
from typing import List

BASE = -2

def from_base_minus_two(a: List[int]) -> int:
    """ Converts base 'BASE' to decimal """

    return sum(j * BASE ** i for i, j in enumerate(a))

def to_base_minus_two(number: int) -> List[int]:
    """ Converts decimal to 'BASE' """

    base_minus_two = []

    while number:
        number, remainder = divmod(number, BASE)

        if remainder < 0:
            number += 1
            remainder -= BASE

        base_minus_two.append(remainder)

    return base_minus_two

def solution(a: List[int]) -> List[int]:
    """

    * Converts from 'BASE' to decimal
    * Divides the decimal by 2
    * Converts the decimal back to 'BASE'

    """

    return to_base_minus_two(math.ceil(from_base_minus_two(a) / 2))

if __name__ == '__main__':
    print(solution([1, 0, 0, 1, 1, 1]))


Compare Oracle Table with SQL Server Table and Update/Insert Below is the current code which is used to update/insert records from Oracle view to SQL Server table using Dapper. There is not a field to check last record updated date in the oracle view so I have added a method to get hashcode by using property values. Since Oracle table has more than 15k records and each record has more more than 60 columns, this approach take more than 5 minutes. Any ideas/suggestions to improve below code?

using System;
using System.Configuration;
using System.Data.OracleClient;
using System.Data.SqlClient;
using System.Linq;
using System.Reflection;
using Dapper;

namespace SyncSQLSvrWithHRDB
{
    internal class Program
    {
        public static PropertyInfo[] PropertyNames = typeof(Employee).GetProperties();

        private static void Main(string[] args)
        {
            var OracleConStr = ConfigurationManager.ConnectionStrings["OracleCon"].ConnectionString;
            var SqlSvrConStr = ConfigurationManager.ConnectionStrings["SqlSvrCon"].ConnectionString;
            using (OracleConnection OraCon = new OracleConnection(OracleConStr))
            {
                var res = OraCon.Query<Employee>(Constants.SelectSql).ToList();

                res = res.GroupBy(x => x.EmpNumber.ToUpper()).Select(x => x.LastOrDefault()).ToList();
                using (SqlConnection Sqlcon = new SqlConnection(SqlSvrConStr))
                {
                    Sqlcon.Open();
                    for (int i = 0; i < res.Count; i++)
                    {
                        var item = Sqlcon.Query<Employee>(Constants.SelectEmpSql, new { EmpNumber= res[i].EmpNumber}).FirstOrDefault();
                        if (item == null) // new record found
                        {
                            Sqlcon.Execute(Constants.InsertSql, res[i]);
                        }
                        else if (GetHashcode(res[i]) != GetHashcode(item)) // record updated
                        {
                            Sqlcon.Execute(Constants.UpdateSql, res[i]);
                        }
                    }
                }
            }
        }

        public static int GetHashcode(Employee o)
        {
            int ret = 0;
            foreach (var prop in PropertyNames)
            {
                object propValue = o.GetType().GetProperty(prop.Name).GetValue(o, null);

                if (propValue != null)
                {
                    ret += propValue.GetHashCode();
                }
            }

            return ret;
        }
    }

    public class Employee
    {
        public String EmpNumber{ get; set; }

        // ... other properties (70)
    }
}

One thing I would say is that you don't need to create those connection string variables, you are only using them once in this program and you have them nicely hidden where they should be, so you can just call them when you use them, which in this case is once.

so instead of this

var OracleConStr = ConfigurationManager.ConnectionStrings["OracleCon"].ConnectionString;
var SqlSvrConStr = ConfigurationManager.ConnectionStrings["SqlSvrCon"].ConnectionString;
using (OracleConnection OraCon = new OracleConnection(OracleConStr))
{
    var res = OraCon.Query<Employee>(Constants.SelectSql).ToList();

    res = res.GroupBy(x => x.EmpNumber.ToUpper()).Select(x => x.LastOrDefault()).ToList();
    using (SqlConnection Sqlcon = new SqlConnection(SqlSvrConStr))
    {

Do this instead

using (OracleConnection OraCon = new OracleConnection(ConfigurationManager.ConnectionStrings["OracleCon"].ConnectionString))
{
    var res = OraCon.Query<Employee>(Constants.SelectSql).ToList();

    res = res.GroupBy(x => x.EmpNumber.ToUpper()).Select(x => x.LastOrDefault()).ToList();
    using (SqlConnection Sqlcon = new SqlConnection(ConfigurationManager.ConnectionStrings["SqlSvrCon"].ConnectionString))
    {

You have already named them nicely so you know exactly what they are, meaning there is no reason to create variables for these, they are only used once.

and instead of a for loop here:

using (SqlConnection Sqlcon = new SqlConnection(SqlSvrConStr))
{
    Sqlcon.Open();
    for (int i = 0; i < res.Count; i++)
    {
        var item = Sqlcon.Query<Employee>(Constants.SelectEmpSql, new { EmpNumber= res[i].EmpNumber}).FirstOrDefault();
        if (item == null) // new record found
        {
            Sqlcon.Execute(Constants.InsertSql, res[i]);
        }
        else if (GetHashcode(res[i]) != GetHashcode(item)) // record updated
        {
            Sqlcon.Execute(Constants.UpdateSql, res[i]);
        }
    }
}

you could use a foreach loop

using (SqlConnection Sqlcon = new SqlConnection(SqlSvrConStr))
{
    Sqlcon.Open();

    foreach (var record in res)
    {
        var item = Sqlcon.Query<Employee>(Constants.SelectEmpSql, new { EmpNumber= record.EmpNumber}).FirstOrDefault();
        if (item == null) // new record found
        {
            Sqlcon.Execute(Constants.InsertSql, record);
        }
        else if (GetHashcode(record) != GetHashcode(item)) // record updated
        {
            Sqlcon.Execute(Constants.UpdateSql, record);
        }
    }
}

This is a little bit cleaner and is more straight to the point about what you are doing.  I would probably change some variable names around, but I will let you have that fun.


Characters set permutations with repetitions The code bellow generates all the permutation(with repetitions) for given character set. Is there any better(simpler, more performant) way to do that?

var assert = require('chai').assert
var _ = require('underscore')

var multStr = function(left, rigth) {
  return _.chain(left).map(function(el) {
    return _(rigth).map(function(chr) {
      return el + chr
    })
  })
  .flatten()
  .value()
}

function solution(space, len) {
  return _.chain(
      _(len - 1).range()
    )
    .reduce(
      function(acc, next) {
          acc.push(
            multStr(
              _.last(
                acc
              ),
              space
            )
          )
          return acc
      },
      [space.split('')]
    )
    .flatten()
    .value()
}

assert.deepEqual(multStr(['ab'], 'ab'), ['aba', 'abb'])
assert.deepEqual(solution('abc', 1), ['a', 'b', 'c'])
assert.deepEqual(solution('a', 2), ['a', 'aa'])
assert.deepEqual(solution('ab', 2), ['a', 'b', 'aa', 'ab', 'ba', 'bb'])
assert.deepEqual(solution('abc', 3), [ 'a', 'b', 'c',
  'aa', 'ab', 'ac', 'ba', 'bb', 'bc', 'ca', 'cb', 'cc',
  'aaa', 'aab', 'aac', 'aba', 'abb', 'abc', 'aca', 'acb', 'acc',
  'baa', 'bab', 'bac', 'bba', 'bbb', 'bbc', 'bca', 'bcb', 'bcc',
  'caa', 'cab', 'cac', 'cba', 'cbb', 'cbc', 'cca', 'ccb', 'ccc'
])

I think the code and the approach look good overall. I have just a few comments:

1) The indentation makes it a bit hard to read. A line like:

acc.push(multStr(_.last(acc), space))

Reads better than across many lines in my opinion.

2) You may consider using concat instead of push:

return acc.concat(multStr(_.last(acc), space))

You trade a bit of performance for it though as concat creates a new array.

3) The most notable problem I see with your code is that it is not very reusable. It only works for strings, while the idea and implementation certainly apply to arrays too. We can think of a string as an array of characters, so abstracting for arrays makes the code more reusable, since we can then do the string one trivially with str.split('').

This leads to the name of the function multStr; it is too specific, and hungarian notation means you are probably over-specifying its functionality, it ought to be more abstract.

Intuitively you wrote code that is more abstract than it seems. I find the Haskell equivalent to your code quite elegant and concise, almost by accident:

[1..3] >>= \n -> replicateM n "ab"

The concept at hand is very general: lists are monads.

We can replicate this in JavaScript easily with lodash. We need flatMap, and range:

var {flatMap, range} = require('lodash')

Next we can loosely translate the Haskell implementations of replicateM and sequence to build combinations:

function combinations(n, xs) {
  return flatMap(range(1, n + 1), m => replicateM(m, xs))
}

function replicateM(n, x) {
  return sequence(Array(n).fill(x))
}

function sequence(ms) {
  return ms.reduce((ma, mb) => {
    return flatMap(ma, a => mb.map(b => a.concat(b)))
  }, [[]])
}

Now this is already more general, we pass an array of characters and get an array of arrays of characters that represent all the possible combinations of n length.

To make it work for your case you only need to split then join:

combinations(3, 'ab'.split('')).map(cs => cs.join(''))
/*^
[ 'a', 'b',
  'aa', 'ab', 'ba', 'bb',
  'aaa', 'aab', 'aba', 'abb',
  'baa', 'bab', 'bba', 'bbb'
]
*/