Equalized PRNG number distribution The main idea behind this code is to distribute multiple random numbers, but guarantee that the probability of each coming is equal.

The code is really basic:

( function( window ){

    window.RNG = function(min, max){

        if( min / 1 != min )
        {
            throw new TypeError('Expecting Number or NumericString, got ' + ( min != min ? 'NaN' : typeof min ) );
        }
        if(arguments.length > 1)
        {
            if( max / 1 != max )
            {
                throw new TypeError('Expecting Number or NumericString as the second parameter, got ' + typeof max);
            }
            else if( ( min = min >> 0 ) >= ( max = max >> 0 ) )
            {
                throw new RangeError('The 2nd parameter should be higher than the first');
            }
        }
        else
        {
            max = min << 0;
            min = 0;
        }

        var dif = max - min, refill=function(){
            for(var i = min, result={length:0}; i <= max; i++)
            {
                result[i]=true;
                result.length++;
            }
            return result;
        }, avail = refill();

        return {
            valueOf:function(){
                if( !avail.length )
                {
                    avail = refill();
                }

                if( avail.length > 1 )
                {
                    var result;
                    do
                    {
                        result = ( window.Math.random() * ( dif + 1 ) + min ) << 0;
                    }
                    while( !avail[result] );

                    delete avail[result];
                    avail.length--;

                    return result;
                }
                else
                {
                    for(var k in avail)
                    {
                        if(k != 'length')
                        {
                            break;
                        }
                    }

                    avail = refill();

                    return k / 1;

                }
            },
            toString:function(){
                return this.valueOf() + '';
            }
        };

    };

} )( Function('return this')() );

The reason why I use that Function('return this')() is just to ensure I have the actual window object.

In terms of readability, what can I improve?

Is there what bad practices that I'm using?

I can't comment on JS in particular too much because I'm no expert. I'll leave that for others more qualified. I can offer some advice on general programming principles though.

Code Styling

Code styling is a preference and yours is fairly consistent at least but since you asked about readability, I will talk a bit about it. Because code styling is generally considered to be opinionated, I will try not to be too specific and stick to the generally accepted norms.

Place opening braces on the same line as the block statement they belong to. There is no need to take up a whole line for a single character.  Use white space to format for neatness where appropriate instead. Same for else blocks. They belong to the if and cannot exist on their own.

if ( foo == bar ) {
} else if ( foo == foobar ) {
} else {
}

Also, space out your syntax appropriately. You should have a space before the opening brace.

window.RNG = function( min, max ) {

'if', 'for', 'while', etc are not functions. place a space between the keyword and the conditional (). See the 'if' example above. Do not place a space between function identifiers and the argument list (you are already doing this).

If you mix variable declarations and assignments, do not use a list of declarations. Create a separate declaration for each.

var dif = max - min, refill=function(){
    for (var i = min, result={length:0}; i <= max; i++) {
        result[i]=true;
        result.length++;
    }
    return result;
}, avail = refill();

... is better written as ...

var dif = max - min;

var refill = function() {
    for ( var i = min, result = { length:0 }; i <= max; i++ ) {
        result[i] = true;
        result.length++;
    }
    return result;
};

var avail = refill();

Programming Habits

Please don't do this. Mixing assignments in expressions is almost always a bad idea.

else if( ( min = min >> 0 ) >= ( max = max >> 0 ) )

You are essentially doing 5 different things in a single statement. This is very confusing and exceptionally prone to error. Without detailed inspection, it is not apparent that you are calculating and assigning min and max at the same time you are comparing them. Not good. Figure out how to split it into 3 statements: 2 assignments and the if statement with a simple conditional.

Why are you calculating dif inside of this expression and inside of a loop? The value of dif never changes.

result = ( window.Math.random() * ( dif + 1 ) + min ) << 0;

Precalculate whenever possible:

var dif = ( max - min ) + 1;

What is this? What's wrong with isNaN()? Try to avoid trickery. It's difficult to know what you intend to happen here. I only know what it's supposed to do by reading the error message.

if( min / 1 != min )

Also use parentheses to make your precedence explicit. Try not to rely on operator precedence. It requires deeper knowledge and is prone to mistakes. Also, the reader can't know if you made a logical error. In the above case we can assume what the precedence is supposed to be because it generally doesn't make sense to return a value to a conditional. That is not always the case though and you make the reader think harder than they should need to.

It has been raised that isNan() is not always available. From my own link, the comments also indicate that isNaN() will not work in every case. If these issues are a concern, then I would suggest creating a new function to self-document what is being done. Since the same test is used in two different places, it is a good candidate for it's own function anyway.


Spreadsheet to calculate how to cut pipes I have written a piece of code that calculates how to cut pipes based on an order and fixed raw pipe length (in this case 288 feet), but I am not 100% happy with my current code as it counts the pipe pieces that have been created for each length in column C (see comment in code) and clears column C at the end. Is there an way to store the values without having to create a variable for each pipe length?

This is what my input sheet looks like:

Here is my code:

Sub CalcPipe()

Dim StartPipeLng As Long, LastLng As Long, TotalLng As Long, PipeName As Long

StartPipeLng = Cells(2, 4).Value

LastLng = ActiveSheet.UsedRange.Rows(ActiveSheet.UsedRange.Rows.Count).Row

TotalLng = Application.WorksheetFunction.SumProduct(Range("A2:A" & LastLng), Range("B2:B" & LastLng))
TotalLng = Application.WorksheetFunction.RoundUp(TotalLng / StartPipeLng, 0)

Cells(5, 4).Value = TotalLng

PipeName = 1

    For y = 6 To TotalLng + 5

        Cells(1, y).Value = "Pipe " & PipeName
        PipeName = PipeName + 1
        StartPipeLng = Cells(2, 4).Value

        For x = 2 To LastLng
            If StartPipeLng - Cells(x, 1).Value >= 0 And Cells(x, 2).Value <> Cells(x, 3).Value Then
                StartPipeLng = StartPipeLng - Cells(x, 1).Value
                Cells(x, 3).Value = Cells(x, 3).Value + 1 ' Here is the issue: Rather than write the values in column C, I would like to store them rather then write them in a cell
                Cells(x, y).Value = Cells(x, y).Value + 1
                x = x - 1
            End If
        Next x
    Next y

Columns(3).Clear

End Sub

And this is what my output looks like:

You didn't declare y or x.

Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

When you don't define your variable, VBA will declare it as a Variant type that can hold any type of data. While this may be more flexible, it adds processing time to your macro as VBA decides or tests for the type.

Additionally, since a Variant can be any type of data, you may miss out on valuable troubleshooting information on Type Mismatch

By not declaring variables, you could possibly be paying a penalty.

It's good practice to indent all of your code that way Labels will stick out as obvious. You also have a ton of unneeded white space. To begin, it might look like this:

Option Explicit

Sub CalcPipe()
    Dim StartPipeLng As Long, LastLng As Long, TotalLng As Long, PipeName As Long
    StartPipeLng = Cells(2, 4).Value
    LastLng = ActiveSheet.UsedRange.Rows(ActiveSheet.UsedRange.Rows.Count).Row
    TotalLng = Application.WorksheetFunction.SumProduct(Range("A2:A" & LastLng), Range("B2:B" & LastLng))
    TotalLng = Application.WorksheetFunction.RoundUp(TotalLng / StartPipeLng, 0)
    Cells(5, 4).Value = TotalLng
    PipeName = 1
    Dim y As Long
    Dim x As Long
        For y = 6 To TotalLng + 5
            Cells(1, y).Value = "Pipe " & PipeName
            PipeName = PipeName + 1
            StartPipeLng = Cells(2, 4).Value
            For x = 2 To LastLng
                If StartPipeLng - Cells(x, 1).Value >= 0 And Cells(x, 2).Value <> Cells(x, 3).Value Then
                    StartPipeLng = StartPipeLng - Cells(x, 1).Value
                    Cells(x, 3).Value = Cells(x, 3).Value + 1
                    Cells(x, y).Value = Cells(x, y).Value + 1
                    x = x - 1
                End If
            Next x
        Next y
    Columns(3).Clear
End Sub

Variable names - give your variables meaningful names.
Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

Dim initialPipeLength As Long
Dim lastPipeRow As Long
Dim totalPipes As Long
Dim pipeNumber As Long
Dim rowCounter As Long
Dim y As Long

A loop like this -

For x = 2 To lastPipeRow
    If firstPipeRow -  Then
        x = x - 1
    End If
 Next x

Could probably just be

For x = lastPipeRow to 2 Step -1
Next

But, since it's not a standard loop and that won't work, you should use variable names to make it more obvious what is happening there.

There is a standard way to find lastRow and lastColumn. That post explains why.

lastPipeRow = ActiveSheet.Cells(Rows.Count, 1).End(xlUp).Row

Also, since you reuse your range, I'd add something to catch if you're accidentally overwriting data

Dim result As VbMsgBoxResult
If Not IsEmpty(ActiveSheet.Cells(1, 6)) Then
    result = MsgBox("There is still data on this sheet, do you want to overwrite it?", VbMsgBoxStyle:=vbYesNo)
    If result = vbNo Then Exit Sub
End If

I'd also clear out old data

If Not IsEmpty(ActiveSheet.Cells(1, 6)) Then
    result = MsgBox("There is still data on this sheet, do you want to overwrite it?", vbYesNo)
    If result = vbNo Then Exit Sub
    Dim lastColumn As Long
    lastColumn = ActiveSheet.Cells(1, Columns.Count).End(xlToLeft).Column
    ActiveSheet.Range(Columns(6), Columns(lastColumn)).Clear
End If

To avoid using column 3, use an array. But, I'd say since you want to use an array, you could optimize your solution to the cutting stock problem with the Greedy algorithm.


File editing program I have written a Java class to replace text in file.  The file contains database configurations and is as follows:

test.properties

#local
db.url=jdbc:oracle:thin:@localhost:test

#db.user=testa
#db.password=testa

db.user=testb
db.password=testb

#db.user=testc
#db.password=testc

# is a comment character. Now db schema testb is in use.

Here is my Java class to switch between schemas

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.FileReader;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

public class SwitchTo
{
  public static void main(String[] args)
  {
    Map<String, String> schemaMap = new HashMap<String, String>();
    schemaMap.put("a", "testa");
    schemaMap.put("b", "testb");
    schemaMap.put("c", "testc");

    String newText = "";
    for (int i = 0; i < args.length; i++)
    {
      newText = args[i];
    }

    String newdb = "";
    newdb = schemaMap.get(newText);
    if (newdb == null || (newdb != null && newdb.length() <= 0))
    {
      System.out.println("Schema not available, please select : ");
      for (Map.Entry<String, String> entry : schemaMap.entrySet())
      {
        System.out.println(entry.getKey() + " for " + entry.getValue());
      }
      return;
    }

    try
    {
      BufferedReader file = new BufferedReader(new FileReader("test.properties"));
      String total = "";
      String line = "";
      while ((line = file.readLine()) != null)
      {
        if (!line.startsWith("#") && line.contains("db.user"))
        {
          line = "#" + line;
        }

        if (!line.startsWith("#") && line.contains("db.password"))
        {
          line = "#" + line;
        }

        if (line.startsWith("#") && line.contains(newdb))
        {
          line = line.substring(1);
        }

        total = total + line + "\n";
      }

      FileOutputStream File = new FileOutputStream("test.properties");
      File.write(total.getBytes());
      System.out.println("Switched to schema " + schemaMap.get(newText));
    }
    catch (FileNotFoundException e)
    {
      e.printStackTrace();
    }
    catch (IOException e)
    {
      e.printStackTrace();
    }
  }
}

and I have created a batch file to run this program from command line which is as follows :

switch.bat
java SwitchTo %1

Now when write at command line

c:\> switch a

Then it will comment testb in the file and testa will be uncommented.

Please suggest other improvement for this code.

Is using map for storing schemas is a good choice?
How can I make it generic to accommodate newly added schemas without need to compile this class again?
Is there any better algorithm for solving this?

Actually, this can be more generic.

Instead of hard-coding your map; fill it from your properties file.
So change your prop file to :

#db.id=a
#db.user=testa
#db.password=testa

db.id=b
db.user=testb
db.password=testb

Fill your map like this :

private Map<String, String> schemaMap = new HashMap<String, String>();

public void fillMap() throws FileNotFoundException, IOException {
    String line;
    String key = null;
    String value = null;
    try (BufferedReader file = new BufferedReader(new FileReader("test.properties"))){
        while ((line = file.readLine()) != null) {

            if (line.contains("db.id")) {
                key = line.substring(line.indexOf('='));// could be you have to do an + 1
            }
            if (line.contains("db.user")) {
                value = line.substring(line.indexOf('='));
            }
            if (key!=null && value != null) {
                schemaMap.put(key, value);
                key = null;
                value = null;
            }
        }
    } catch (IOException ex) {
         // report with Logger or System.out what happend.
    }
}

Secondly : In your main you use the args.
You do not check for how many args you have, you just take the last arg.
Maybe a check for how many args you have is appropriate here, and make the program do what you want then. (like quitting with error message or taking default arg).

Third : Always close your resources(reader/writer in this case).
Here it is done with a Try-with-resources available from java 7, otherwise you will need a finally block where you close it in.


filter SE posts with specific keywords I am trying to find all the SE answers which contain "on context" in a single sentence, in the meanwhile the "on" is not a part of common collocations in any form, such as "depending on context", "depend on context", "Based on context", "rely on context", "Counting on Context", "Count on Context" ...

Here is the code

SELECT Id, CreationDate, Score, Body
INTO #tb_postcount
  FROM Posts
  WHERE Body LIKE '% on context%'
    AND PostTypeId=2

SELECT Id AS [Post Link] from #tb_postcount
WHERE Body collate SQL_Latin1_General_CP1_CI_AS NOT LIKE '%depend%on context%'
and Body collate SQL_Latin1_General_CP1_CI_AS NOT LIKE '%base%on context%'
and Body collate SQL_Latin1_General_CP1_CI_AS NOT LIKE '%rely%on context%'
and Body collate SQL_Latin1_General_CP1_CI_AS NOT LIKE '%count%on context%'
and Body collate SQL_Latin1_General_CP1_CI_AS NOT LIKE '%lean%on context%'
and Body collate SQL_Latin1_General_CP1_CI_AS NOT LIKE '%take%on context%'

It works well, at least to me:) Could someone please take a look at it?

Is there a more efficient way to do the job?

The following are a few suggestions on how I'd write the SQL statement.

Source Control

If you don't already have a database project, create one in Visual Studio. Then check it in to source control. Microsoft Azure DevOps Services is free & private for teams of 5 or less (this is per project, so 5 developers per project). Then you'll be able to track changes you make to your stored procedures, views, tables, etc.

Formatting

I would download the following tool for SSMS and Visual Studio, SQL formatter from ApexSQL. I use it when I have to edit other developer's code. It's a great way to standardize your SQL. I find it does most of the formatting for me, but I'll still make a few changes after.

Copy & Paste

If you find yourself copying and pasting the same string or number over and over in your query, then you should define it as a variable or create a table. Copy and paste is a design error ~ David Parnas

Commas

I would put the commas in front to clearly define new columns. Versus code wrapped in multiple lines. It also makes trouble-shooting code easier.

Where Clause

If you put 1=1 at the top of a WHERE condition, it enables you to freely change the rest of the conditions when debugging a query. The SQL query engine will end up ignoring the 1=1 so it should have no performance impact. Reference

Common Table Expressions (CTE)

CTE's in your SQL, help with documentation. The expression name can then let other developers know why you used that expression e.g. post_body_filter.

Schema Names

Always reference the schema when selecting an object e.g. [dbo].[Posts].

Keywords

Avoid using keywords as object names. Microsoft Reference

Also check out the book Clean Code. It will change the way you think about naming conventions.

Revised SQL

Without table definitions and sample records I was unable to test this, but it should give you a good start.

WITH
Posts
AS
(
   SELECT tbl.* FROM (VALUES
    ( 1, '01-Jan-2020', 85, 'depend on context', 2)
   , ( 2, '02-Jan-2020', 86, 'base on Context', 2)
   , ( 3, '03-Jan-2020', 87, 'rely on context', 2)
   , ( 4, '04-Jan-2020', 88, 'Count on Context', 2)
   , ( 5, '05-Jan-2020', 89, 'lean on context', 2)
   , ( 6, '06-Jan-2020', 90, 'Take on Context', 2)
   , ( 7, '07-Jan-2020', 91, '... on context', 2)
   , ( 8, '08-Jan-2020', 92, ' on context ...', 2)
   , ( 9, '09-Jan-2020', 93, '... on context ...', 2)
   , ( 10, '10-Jan-2020', 94, '... on ... context ...', 2)
   , ( 11, '11-Jan-2020', 95, '...', 2)
   , ( 12, '12-Jan-2020', 96, '... on context ...', 1)
   ) tbl ([Id], [CreationDate], [Score], [Body], [PostTypeId])
)
,
post_body_filter --here it would be good to name the table expression so others will know why you're filtering them out.
AS
(
   SELECT tbl.* FROM (VALUES
    ( '%depend%')
   , ( '%base%')
   , ( '%rely%')
   , ( '%count%')
   , ( '%lean%')
   , ( '%take%')
   ) tbl ([Body])
)
SELECT
     [po].[Id]
    , [po].[CreationDate]
    , [po].[Score]
    , [po].[Body]
FROM
    [Posts] AS [po]
    LEFT JOIN [post_body_filter] AS [pf] ON [po].[Body] LIKE [pf].[Body] COLLATE SQL_Latin1_General_CP1_CI_AS
WHERE
  1 = 1
  AND [pf].[Body] IS NULL
  AND [po].[Body] LIKE '% on context%'
  AND [po].[PostTypeId] = 2
  ;

Results


Method that checks whether to disable or enable the buttons depending on whether data is null or not I have this bit of code that needs refactoring/simplification. The below grabs the planning data for view A and B. Then it joins the planning and matching image for view details. So then it determines if it should set the buttons to be disabled or not based on whether or not, if the data is empty/null or not.

I think the major refactoring might be needed for adding the elements to the dictionaries for each view.

[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult CheckButtons()
{
    int planId = (int)UserSession.GetValue(StateNameEnum.ID, "PlanID");

    planning resultSetA = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
      (p, i) => new
      {
          planning = p,
          i.View
      }).Where(j => j.View == "A").Select(j => j.planning).FirstOrDefault();

    structure sA = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == "A");

    planning resultSetB = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
      (p, i) => new
      {
          planning = p,
          i.View
      }).Where(j => j.View == "B).Select(j => j.planning).FirstOrDefault();

    structure sB = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == "B");

    Dictionary<string, string> buttonADictionary = new Dictionary<string, string>();
    Dictionary<string, string> buttonBDictionary = new Dictionary<string, string>();
    buttonADictionary.Add("C", resultSetA.CorrespondingId != null ? "disabled" : "enabled");
    buttonADictionary.Add("R", resultSetA.ResponseId != null ? "disabled" : "enabled");
    buttonADictionary.Add("S", (sA != null && sA.Point_x != null) ? "disabled" : "enabled");
    buttonADictionary.Add("D", resultSetA.DistalId != null ? "disabled" : "enabled");
    buttonADictionary.Add("P", resultSetA.ProximalId != null ? "disabled" : "enabled");
    buttonBDictionary.Add("C", resultSetB.CorrespondingId != null ? "disabled" : "enabled");
    buttonBDictionary.Add("R", resultSetB.ResponseId != null ? "disabled" : "enabled");
    buttonBDictionary.Add("S", (sB != null && sB.Point_x != null) ? "disabled" : "enabled");
    buttonBDictionary.Add("D", resultSetB.DistalId != null ? "disabled" : "enabled");
    buttonBDictionary.Add("P", resultSetB.ProximalId != null ? "disabled" : "enabled");
    bool aHasData = buttonADictionary.ContainsValue("disabled");
    bool bHasData = buttonBDictionary.ContainsValue("disabled");

    return Json(new { buttonADictionary, buttonBDictionary, aHasData, bHasData }, JsonRequestBehavior.AllowGet);
}

In looking at your method, you have a lot of repetition in building your two dictionaries before composing your final result. If you were to simply regroup your code to separate the building of your first and second dictionaries, and the data that supports the construction, you would produce the following method:

[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult CheckButtons()
{
    int planId = (int)UserSession.GetValue(StateNameEnum.ID, "PlanID");

    planning resultSetA = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
        (p, i) => new
        {
            planning = p,
            i.View
        }).Where(j => j.View == "A").Select(j => j.planning).FirstOrDefault();

    structure sA = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == "A");

    Dictionary<string, string> buttonADictionary = new Dictionary<string, string>();
    buttonADictionary.Add("C", resultSetA.CorrespondingId != null ? "disabled" : "enabled");
    buttonADictionary.Add("R", resultSetA.ResponseId != null ? "disabled" : "enabled");
    buttonADictionary.Add("S", (sA != null && sA.Point_x != null) ? "disabled" : "enabled");
    buttonADictionary.Add("D", resultSetA.DistalId != null ? "disabled" : "enabled");
    buttonADictionary.Add("P", resultSetA.ProximalId != null ? "disabled" : "enabled");
    bool aHasData = buttonADictionary.ContainsValue("disabled");

    planning resultSetB = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
        (p, i) => new
        {
            planning = p,
            i.View
        }).Where(j => j.View == "B").Select(j => j.planning).FirstOrDefault();

    structure sB = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == "B");

    Dictionary<string, string> buttonBDictionary = new Dictionary<string, string>();
    buttonBDictionary.Add("C", resultSetB.CorrespondingId != null ? "disabled" : "enabled");
    buttonBDictionary.Add("R", resultSetB.ResponseId != null ? "disabled" : "enabled");
    buttonBDictionary.Add("S", (sB != null && sB.Point_x != null) ? "disabled" : "enabled");
    buttonBDictionary.Add("D", resultSetB.DistalId != null ? "disabled" : "enabled");
    buttonBDictionary.Add("P", resultSetB.ProximalId != null ? "disabled" : "enabled");
    bool bHasData = buttonBDictionary.ContainsValue("disabled");

    return Json(new { buttonADictionary, buttonBDictionary, aHasData, bHasData }, JsonRequestBehavior.AllowGet);
}

It's evident at this point that your code at the top and at the bottom is effectively identical, only differing on a value passed into two queries and then on variable names. Inside this single method, you are violating the DRY principle: don't repeat yourself. So let's make the first extraction and create the method here:

private Dictionary<string, string> GetButtonDictionary(int planId, string value)
{
    planning resultSet = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
        (p, i) => new
        {
            planning = p,
            i.View
        }).Where(j => j.View == value).Select(j => j.planning).FirstOrDefault();

    structure st = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == value);

    Dictionary<string, string> buttonDictionary = new Dictionary<string, string>();
    buttonDictionary.Add("C", resultSet.CorrespondingId != null ? "disabled" : "enabled");
    buttonDictionary.Add("R", resultSet.ResponseId != null ? "disabled" : "enabled");
    buttonDictionary.Add("S", (st != null && st.Point_x != null) ? "disabled" : "enabled");
    buttonDictionary.Add("D", resultSet.DistalId != null ? "disabled" : "enabled");
    buttonDictionary.Add("P", resultSet.ProximalId != null ? "disabled" : "enabled");
    return buttonDictionary;
}

The method above takes two parameters: a planId that you have obtained, as well as a value. This second variable probably isn't a great name, but you have overloaded the meaning of your string in your original method (it's used to compare against a View property and a Type property) such that I couldn't immediately think of a better name. If you can think of one, just sub it in.

With this extraction, it cleans up your original method nicely, because it can be cleaned up to the following:

[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult CheckButtons()
{
    int planId = (int)UserSession.GetValue(StateNameEnum.ID, "PlanID");
    Dictionary<string, string> buttonADictionary = GetButtonDictionary(planId, "A");
    Dictionary<string, string> buttonBDictionary = GetButtonDictionary(planId, "B");

    bool aHasData = buttonADictionary.ContainsValue("disabled");
    bool bHasData = buttonBDictionary.ContainsValue("disabled");

    return Json(new { buttonADictionary, buttonBDictionary, aHasData, bHasData }, JsonRequestBehavior.AllowGet);
}

Could we clean this up a little more? Possibly, but it's simplified enough for now so let's go back to the method we extracted. It has a couple of database queries that should probably individually be their own methods (SRP - Single Responsibility Principle), so let's go ahead and do that.

private Dictionary<string, string> GetButtonDictionary(int planId, string value)
{
    planning resultSet = GetPlanningResultSet(planId, value);
    structure st = GetStructure(planId, value);

    Dictionary<string, string> buttonDictionary = new Dictionary<string, string>();
    buttonDictionary.Add("C", resultSet.CorrespondingId != null ? "disabled" : "enabled");
    buttonDictionary.Add("R", resultSet.ResponseId != null ? "disabled" : "enabled");
    buttonDictionary.Add("S", (st != null && st.Point_x != null) ? "disabled" : "enabled");
    buttonDictionary.Add("D", resultSet.DistalId != null ? "disabled" : "enabled");
    buttonDictionary.Add("P", resultSet.ProximalId != null ? "disabled" : "enabled");
    return buttonDictionary;
}

private planning GetPlanningResultSet(int planId, string value)
{
    planning resultSet = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
        (p, i) => new
        {
            planning = p,
            i.View
        }).Where(j => j.View == value).Select(j => j.planning).FirstOrDefault();
    return resultSet;
}

private structure GetStructure(int planId, string value)
{
   return db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == value);
}

After doing this, you have the obvious bit of repetition left where you get your enabled settings where you repeat your null checks. So let's extract a method to deal with that.

private Dictionary<string, string> GetButtonDictionary(int planId, string value)
{
    planning resultSet = GetPlanningResultSet(planId, value);
    structure st = GetStructure(planId, value);

    Dictionary<string, string> buttonDictionary = new Dictionary<string, string>();
    buttonDictionary.Add("C", GetEnabledSetting(resultSet.CorrespondingId));
    buttonDictionary.Add("R", GetEnabledSetting(resultSet.ResponseId));
    buttonDictionary.Add("S", GetEnabledSetting(st != null ? st.Point_x : null));
    buttonDictionary.Add("D", GetEnabledSetting(resultSet.DistalId));
    buttonDictionary.Add("P", GetEnabledSetting(resultSet.ProximalId));
    return buttonDictionary;
}

private string GetEnabledSetting(object value)
{
    return value != null ? "disabled" : "enabled";
}

After we have completed this extraction, the full code now looks like the following:

[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult CheckButtons()
{
    int planId = (int)UserSession.GetValue(StateNameEnum.ID, "PlanID");
    Dictionary<string, string> buttonADictionary = GetButtonDictionary(planId, "A");
    Dictionary<string, string> buttonBDictionary = GetButtonDictionary(planId, "B");

    bool aHasData = buttonADictionary.ContainsValue("disabled");
    bool bHasData = buttonBDictionary.ContainsValue("disabled");

    return Json(new { buttonADictionary, buttonBDictionary, aHasData, bHasData }, JsonRequestBehavior.AllowGet);
}

private Dictionary<string, string> GetButtonDictionary(int planId, string value)
{
    planning resultSet = GetPlanningResultSet(planId, value);
    structure st = GetStructure(planId, value);

    Dictionary<string, string> buttonDictionary = new Dictionary<string, string>();
    buttonDictionary.Add("C", GetEnabledSetting(resultSet.CorrespondingId));
    buttonDictionary.Add("R", GetEnabledSetting(resultSet.ResponseId));
    buttonDictionary.Add("S", GetEnabledSetting(st != null ? st.Point_x : null));
    buttonDictionary.Add("D", GetEnabledSetting(resultSet.DistalId));
    buttonDictionary.Add("P", GetEnabledSetting(resultSet.ProximalId));
    return buttonDictionary;
}

private planning GetPlanningResultSet(int planId, string value)
{
    planning resultSet = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
        (p, i) => new
        {
            planning = p,
            i.View
        }).Where(j => j.View == value).Select(j => j.planning).FirstOrDefault();
    return resultSet;
}

private structure GetStructure(int planId, string value)
{
   return db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == value);
}

private string GetEnabledSetting(object value)
{
    return value != null ? "disabled" : "enabled";
}

This is enough contribution for me. I'll leave it up to you to possibly create better variable names and get the code into a bit better compliance with C# standards. For example, you seem to have classes starting in lowercase (planning, structure) and those should start with capital letters. You seem to have DbSets that should be plural (and capitalized). Finally, this seems to be a lot of work to be in a controller, perhaps it could be be further extracted to a pure business layer. Those modifications would, of course, be up to you.


Security of login and register system I need some help with my register and log in functions. I'm not sure if I have understood bcrypt correctly either. I'm doubting the security of it, what do you think?

Is this good, or are there any changes I should make? Have I missed anything?

For the database table users, I created (ID, username and password[char:60]):

function loginAccount() {
    global $connect;

    $username = mysqli_real_escape_string($connect, $_POST['username']);
    $password = mysqli_real_escape_string($connect, $_POST['password']);

    $existingHashFromDb = "";
    if(!isset($existingHashFromDb)) {
        // existing password from database is not set
    } else {

        $login_sql = "SELECT password FROM users WHERE username = '".$username."'";
        $login_result = mysqli_query($connect, $login_sql);

        if($login_result) {
            while($row = mysqli_fetch_assoc($login_result)) {
                $existingHashFromDb = $row['password'];
            }

            # Check if the hash of the entered login password, matches the stored hash.
            # The salt and the cost factor will be extracted from $existingHashFromDb.
            $isPasswordCorrect = password_verify($password, $existingHashFromDb);

            if($isPasswordCorrect) {
                // you are logged in
                echo "
Welcome " . ucfirst($username) . "
";

                // logout?
                echo "
Logout
";
            } else {
                echo "
Password did not match.
";
            }
        } else {
            // query failed
            mysqli_free_result($login_result);
            mysqli_close($connect);
        }

    }

}

function registerAccount() {
    global $connect;

    $username       = mysqli_real_escape_string($connect, $_POST['username']);
    $password       = mysqli_real_escape_string($connect, $_POST['password']);
    $password_again = mysqli_real_escape_string($connect, $_POST['password_again']);

    # This way you can define a cost factor (by default 10). Increasing the
    # cost factor by 1, doubles the needed time to calculate the hash value.
    $hashToStoreInDb = password_hash($password, PASSWORD_BCRYPT, array("cost" => 11));

    # See if the username already exists in the database
    $userexist_query = "SELECT username FROM users WHERE username = '".$username."'";
    $userexist_result = mysqli_query($connect, $userexist_query);

    if($userexist_result) {
        if(mysqli_num_rows($userexist_result) >= 1) {
            echo "
Username already exists!
";
        } else {
            # See if the password and password_again is matching.
            if($password == $password_again) {
                $register_query  = "INSERT INTO users (username, password) VALUES ('$username', '$hashToStoreInDb')";
                $register_result = mysqli_query($connect, $register_query);

                if($register_result) {
                    echo "
" . ucfirst($username) . " has been created!
 Password: {$hashToStoreInDb}
";
                } else {
                    // query failed
                    mysqli_free_result($register_result);
                    mysqli_close($connect);
                }
            } else {
                echo "
Password does not match!
";
            }
        }
    } else {
        // query failed
        mysqli_free_result($userexist_result);
        mysqli_close($connect);
    }

}

As others have mentioned, use the PDO library for database work and there is no reason to use gobals. Another thing I would highly recommend, is to use exceptions, they're amazing. I'm glad to see you using the password_* library though.

I have wrote a simple authentication class to get you started, and should by no means be used in a production environment.

class Auth
{
    private $dbh;
    private $config;
    private $user;

    public function __construct(PDO $dbh, array $config = array())
    {
        $this->dbh = $dbh;
        $this->config = $config;
        session_start();

        if ($this->isLoggedIn()) {
            $this->user = $this->getLoggedInUser();
        }
    }

    public function login($username, $password)
    {
        // check if user is already logged in

        // now validate your input data

        $user = $this->findUserByUsername($username);
        var_dump($user);

        if ($user) {
            if (password_verify($password, $user->password)) {
                $_SESSION['username'] = $user->username;
                $this->user = $user;

                return true;
            }

            throw new Exception('The password is incorrect');
        }

        throw new Exception("The username {$username} could not be found");
    }

    public function register(array $data)
    {
        // data validation goes here

        if ($data['password'] !== $data['password_again']) {
            throw new Exception("The passwords do not match");
        }

        if ($this->findUserByUsername($data['username'])) {
            throw new Exception('The username ' . $data['username'] . ' already exists');
        }

        $hash = password_hash($data['password'], PASSWORD_BCRYPT, array('cost' => 11));
        $sql = 'INSERT INTO users(username, password) VALUES (:username, :password)';

        $stmt = $this->dbh->prepare($sql);
        $stmt->bindParam(':username', $data['username'], PDO::PARAM_STR);
        $stmt->bindParam(':password', $hash, PDO::PARAM_STR);
        $result = $stmt->execute();

        if ($result) {
            $this->login($data['username'], $data['password']);

            return true;
        }

        throw new Exception('Something went seriously wrong');
    }

    public function logout()
    {
        if ($this->isLoggedIn()) {
            unset($_SESSION['username']);

            return true;
        }

        return false;
    }

    public function isLoggedIn()
    {
        return array_key_exists('username', $_SESSION);
    }

    public function getLoggedInUser()
    {
        if ($this->user === null) {
            $this->user = $this->findUserByUsername($_SESSION['username']);
        }

        return $this->user;
    }

    protected function findUserByUsername($username)
    {
        $stmt = $this->dbh->prepare('SELECT * FROM users WHERE username = :username');
        $stmt->bindParam(':username', $username, PDO::PARAM_STR);
        $stmt->execute();

        return $stmt->fetch(PDO::FETCH_OBJ);
    }
}

$dbh = new PDO('mysql:host=localhost;dbname=test', 'root', '');
$auth = new Auth($dbh);

try {

    // login example
    //$auth->login($_POST['username'], $_POST['password']);

    // register example
    //$auth->register($_POST);

} catch (Exception $e) {

    die($e->getMessage());
}

// logout
if (isset($_GET['logout'])) {
    $auth->logout();
}

// check if logged in
if ($auth->isLoggedIn()) {
    $username = $auth->getLoggedInUser()->username;

    echo '
Welcome ' . ucfirst($username) . '
';
    echo '
Logout
';
}


Seasons by month and day Write a program reads two integers representing a month and day and prints the season for that month and day. Assume that months are specified as an integer between 1 and 12 (1 for January, 2 for February, and so on) and that the day of the month is a number between 1 and 31.
If the date falls between 12/16 and 3/15, you should print "Winter". If the date falls between 3/16 and 6/15, you should print "Spring". If the date falls between 6/16 and 9/15, you should print "Summer". And if the date falls between 9/16 and 12/15, you should print "Fall".

public static void main(String[] args)
{
    Scanner scan = new Scanner(System.in);

    int month = scan.nextInt();
    int day = scan.nextInt();

    if((month == 12 && day >= 16 && day <= 31) || (month == 1 && day >=1 && day <= 31) || (month == 2 && day >= 1 && day <= 28) || (month == 3 && day >=1 && day <=15))
    {
        System.out.println("Winter");
    }
    else if((month == 3 && day >= 16 && day <= 31) || (month == 4 && day >=1 && day <= 30) || (month == 5 && day >= 1 && day <= 31) || (month == 6 && day >=1 && day <=15))
    {
        System.out.println("Spring");
    }
    else if((month == 6 && day >= 16 && day <= 30) || (month == 7 && day >=1 && day <= 31) || (month == 8 && day >= 1 && day <= 31) || (month == 9 && day >=1 && day <=15))
    {
        System.out.println("Summer");
    }
    else if((month == 9 && day >= 16 && day <= 30) || (month == 10 && day >=1 && day <= 31) || (month == 11 && day >= 1 && day <= 30) || (month == 12 && day >=1 && day <=15))
    {
        System.out.println("Fall");
    }
}

}

Readability

Your conditional checks are very very long. They run off the screen. At the very least, add some new lines:

else if ((month == 9 && day >= 16 && day <= 30) ||
         (month == 10 && day >=1 && day <= 31) ||
         (month == 11 && day >= 1 && day <= 30) ||
         (month == 12 && day >=1 && day <=15))

But this still makes it very hard to actually understand what is going on. There's no really any notion of "range" in this code. You are simply hardcoding each individual month separately.

Ranges

Representing the range "3/16 to 6/15" is difficult. Representing the range 316 to 615 is easy. If we don't care about checking for invalid dates, we can uniquely represent the year by doing month * 100 + day and looking at it that way.

int month_day = month * 100 + day;
if (month_day <= 315) {
    System.out.println("Winter");
}
else if (month_day <= 615) {
    System.out.println("Spring");
}
else if (month_day <= 915) {
    System.out.println("Summer");
}
else if (month_day <= 1215) {
    System.out.println("Fall");
}
else {
    System.out.println("Winter");
}

By treating the ranges as one-sided, it's easy to organize the code to actually look like we're looking for ranges. Winter simply appears on both ends for simplicity.


How to order DataColumns with LINQ? Is there a very short way to do this same thing in a pure LINQ based way?  It seems like I should not have to do all of this below to get to the OrderBy.

Dictionary<DataColumn, DateTime> columns = new Dictionary<DataColumn, DateTime>();
int startIndex;
int endIndex;
for (int i = 0; i < table.Columns.Count; ++i)
{
    // these columns need to be sorted
    startIndex = table.Columns[i].Caption.IndexOf('(');
    endIndex = table.Columns[i].Caption.IndexOf(')');
    if (startIndex > 0 && endIndex > 0)
    {
        // create a standard date
        string monthYear = table.Columns[i].Caption.Substring(
            startIndex + 1, endIndex - startIndex - 1);
        columns.Add(
            table.Columns[i],
            DateTime.Parse(monthYear.Replace("/", "/01/")).Date);
    }
    else
    {
        // all other columns should be sorted first
        columns.Add(table.Columns[i], DateTime.MinValue);
    }
}

// perform the LINQ order by
columns = columns.OrderBy(o => o.Value).ToDictionary(o => o.Key, o => o.Value);

// order the original table uses the dictionary
for (int i = 0; i < columns.Count; ++i)
{
    table.Columns[columns.Keys.ElementAt(i).Caption].SetOrdinal(i);
}

Throwing the contents into a dictionary will lose any sorting you may have had. Dictionaries are unordered collections.

The cleanest way to deal with this really is to implement your own custom comparer to compare the columns and reset their order based on the ordering.

I believe this would be more or less logically equivalent:

public class MyColumnComparer : Comparer<DataColumn>
{
    static Regex re = new Regex(@"[^\(\)][^\(]*\((\d{2}/\d{4})\).*");
    public override int Compare(DataColumn x, DataColumn y)
    {
        // a regular expression might be overkill here but it will be easier to maintain
        var xMatch = re.Match(x.Caption);
        var yMatch = re.Match(y.Caption);

        // both did not contain the pair
        if (!xMatch.Success && !yMatch.Success)
        {
            // use original ordering
            return x.Ordinal.CompareTo(y.Ordinal);
        }

        // one contained the pair
        if (xMatch.Success != yMatch.Success)
        {
            return yMatch.Success ? -1 : 1;
        }

        // both contains a pair
        var xDate = DateTime.ParseExact(xMatch.Groups[1].Value, "MM/yyyy",
            CultureInfo.InvariantCulture);
        var yDate = DateTime.ParseExact(yMatch.Groups[1].Value, "MM/yyyy",
            CultureInfo.InvariantCulture);
        return xDate.CompareTo(yDate);
    }
}

static void ProcessTable(DataTable table)
{
    var sortedColumns = table.Columns
        .Cast<DataColumn>()
        .OrderBy(col => col, new MyColumnComparer())
        .ToList();
    var index = 0;
    foreach (var column in sortedColumns)
        column.SetOrdinal(index++);
}


Adding new or existing tags to a blog post I'm working on a simple blogsystem (for learning purposes, not to reinvent the wheel). In this system the user can add existing or new tags to a blogpost. I wrote a method to achieve this, but it's ugly, inconvenient and probably of bad performance.

Some facts:

I make use of Propel as mapper to a PostgreSQL database
relevant tables in this part are tag and post_tag

CREATE-Statements:

CREATE TABLE tag (
    id integer NOT NULL,
    name character varying(100),
    slug character varying(100),
    modified timestamp(0) without time zone
);

CREATE SEQUENCE tag_id_seq
    START WITH 1
    INCREMENT BY 1
    NO MINVALUE
    NO MAXVALUE
    CACHE 1;

ALTER TABLE ONLY tag
    ADD CONSTRAINT tag_pkey PRIMARY KEY (id);

ALTER TABLE ONLY tag ALTER COLUMN id SET DEFAULT nextval('tag_id_seq'::regclass);

CREATE TABLE post_tag (
    post_id integer NOT NULL,
    tag_id integer NOT NULL
);

ALTER TABLE ONLY post_tag
    ADD CONSTRAINT post_tag_pkey PRIMARY KEY (post_id, tag_id);

ALTER TABLE ONLY post_tag
    ADD CONSTRAINT post_tag_post FOREIGN KEY (post_id) REFERENCES post(id) ON UPDATE CASCADE ON DELETE CASCADE;

ALTER TABLE ONLY post_tag
    ADD CONSTRAINT post_tag_tag FOREIGN KEY (tag_id) REFERENCES tag(id) ON UPDATE CASCADE ON DELETE CASCADE;

The method is called after submitting the post. It gets the postID and an array containing all tags assigned to this post. It adds new tags in the table tag and makes sure that in table post_tag all new, all previously existing and all previously existing but not assigned tags, which the user chose, are connected to the post.

This is the method:

      public function addTagByPost($posttags, $postid){

        //collect all existing tags
        $all = $this->getAllTags();

        $existing = array();
        foreach($all AS $tags){
            $existing[$tags->getName()] = $tags->getId();
        }

        //delete all tags linked to the post
        $links = PostTagQuery::create()
            ->filterByPostId($postid);

        $links->delete();

        foreach($posttags AS $tag){
            if(!isset($existing[$tag])){
                //add new tags from post
                $newtag = new Tag();
                $newtag->setName($tag);
                $newtag->setSlug($this->generateSlug($tag));
                $newtag->save();
                $tagid = $newtag->getId();
            }else{
                $tagid = $existing[$tag];
            }

            //link all tags to the post
            $link = new PostTag();
            $link->setPostId($postid);
            $link->setTagId($tagid);
            $link->save();

        }
    }

Imagine in table tag are three rows, PHP, MySQL and PostgreSQL. Imagine there is a blogpost tagged with PHP and PostgreSQL. The user edits the post, removing the tag PHP and adding the tag Python. When he submits, the current tags are PostgreSQL and Python.

At first, all existing tags are fetched from db (PHP, MySQL and PostgreSQL).
Then all links between the current post and its tags are deleted from table post_tag (PHP and PostgreSQL).

Now the tags assigned by the user (PostgreSQL and Python) are matched against the existing ones (PHP, MySQL and PostgreSQL). As Python does not already exist, it is added to table tag.

Now, all userassigned tags are accessible via id. These IDs are then inserted into table post_tag (PostgreSQL and Python), so the post is connected to them.

Please feel free to make suggestions.

Take a step back and look at your code. Do the different classes/objects ha ve Single responsibility? Or is the same class responsible for adding posts, managing tags and coupling a tag to a post?

I think it is time to go back to the drawing board and see what you actually want/need.

Then every class you write should only have a single responsibility. e.g. representing a comment or a TaggableComment class that extends the Comment class and adds the ability to add and remove tags.

Also make sure that a Class shouldn't have to know how to handle the creation of other objecs. Using the 'new' operator inside a class methods is bad practice. If you would like to change the way a Tag is constructed, you will now have to change the Comment class aswell, not useful.

Look into some programming patterns, A really good read is: http://addyosmani.com/resources/essentialjsdesignpatterns/book/ I know, it's not php but javascript. But moest of the patterns there also apply to php, it's the idea behind the patterns that will help you


Reorganizing inventory items into a data structure I'm currently refactoring some old code of mine that I had done a few years ago, where it takes in two Lists to compare their contents, and then return the mapped contents that were matched. With 750 items for potential needles, and over 25000 items as the haystack, the original code took about 22-25 seconds to process. After a bit of refactoring however I was able to reduce it to just over a second. Is there a way to improve it further?

Here's the original code

Map<String, ArrayList<TreeMap<String, String>>> map = new TreeMap<String, ArrayList<TreeMap<String, String>>>();

String charsToFilter = "[" + Pattern.quote("_-") + "]";

for (int i = 0 ; i < baseDataArray.size() ; i++) {
  String currentProduct = baseDataArray.get(i).get("product_name").replaceAll(charsToFilter, "  ").toLowerCase();

  for (int j = 0 ; j < inventoryArray.size() ; j++) {
      String currentListing = inventoryArray.get(j).get("title").replaceAll(charsToFilter, " ").toLowerCase();

      if (currentListing.contains(currentProduct)) {
          ArrayList<TreeMap<String, String>> currentListingArray = new ArrayList<TreeMap<String, String>>();
          TreeMap<String, String> currentListingMap = new TreeMap<String, String>();

          for (Map.Entry<String, String> keyValue : inventoryArray.get(j).entrySet()) {
              currentListingMap.put(keyValue.getKey(), keyValue.getValue());
          }

          currentListingArray.add(currentListingMap);

          if (map.get(currentProduct) == null) {
              map.put(currentProduct, currentListingArray);
          } else {
              map.get(currentProduct).add(currentListingMap);
          }
      }
  }
}

return map;

And here's the new code:

Map<String, ArrayList<TreeMap<String, String>>> map = new TreeMap<String, ArrayList<TreeMap<String, String>>>();

String charsToFilter = "[" + Pattern.quote("_-") + "]";

ArrayList<String> productNameArray = new ArrayList<String>();

for (Map<String, String> baseDataItem : baseDataArray) {
    productNameArray.add(baseDataItem.get("product_name")
      .replaceAll(charsToFilter, " ").toLowerCase());
}

for (Map<String, String> inventoryItem : inventoryArray) {

    //moving this to the for loop below increase execution time by 100%
    String processedInventoryItem = inventoryItem.get("title").replaceAll(charsToFilter, " ").toLowerCase();

    for (String productName : productNameArray) {
        if (processedInventoryItem.contains(productName)) {
            ArrayList<TreeMap<String, String>> currentListingArray = new ArrayList<TreeMap<String, String>>();
            TreeMap<String, String> currentListingMap = new TreeMap<String, String>();

            for (Map.Entry<String, String> keyValue :  inventoryItem.entrySet()) {
                currentListingMap.put(keyValue.getKey(),  keyValue.getValue());
            }

            currentListingArray.add(currentListingMap);

            if (map.get(productName) == null) {
                map.put(productName, currentListingArray);
            } else {
                map.get(productName).add(currentListingMap);
            }

            break;
        }
    }
}

return map;

There's some differences such as going from for to foreach loops, but nothing that I can see that would make such a change. Anyone have any thoughts on how to improve it further, or why it has come so far already.

The first thing to note is that Java no longer requires the generic types to be specified in the declaration of the collections you have, so, for example, the code

Map<String, ArrayList<TreeMap<String, String>>> map = new TreeMap<String, ArrayList<TreeMap<String, String>>>();

can be replaced with:

Map<String, ArrayList<TreeMap<String, String>>> map = new TreeMap<>();

That simplifies your line-lengths in a few places.

This is how I would simplify the code cosmetically:

Map<String, ArrayList<TreeMap<String, String>>> map = new TreeMap<>();
String charsToFilter = "[" + Pattern.quote("_-") + "]";
ArrayList<String> productNameArray = new ArrayList<>();

for (Map<String, String> baseDataItem : baseDataArray) {
    productNameArray.add(baseDataItem.get("product_name")
                    .replaceAll(charsToFilter, " ")
                    .toLowerCase());
}

for (Map<String, String> inventoryItem : inventoryArray) {
    //moving this to the for loop below increase execution time by 100%
    String processedInventoryItem = inventoryItem.get("title")
          .replaceAll(charsToFilter, " ")
          .toLowerCase();

    for (String productName : productNameArray) {
        if (processedInventoryItem.contains(productName)) {
            ArrayList<TreeMap<String, String>> currentListingArray = new ArrayList<>();
            TreeMap<String, String> currentListingMap = new TreeMap<>();
            for (Map.Entry<String, String> keyValue :  inventoryItem.entrySet()) {
                currentListingMap.put(keyValue.getKey(),  keyValue.getValue());
            }

            currentListingArray.add(currentListingMap);
            if (map.get(productName) == null) {
                map.put(productName, currentListingArray);
            } else {
                map.get(productName).add(currentListingMap);
            }

            break;
        }
    }
}
return map;

OK, that fits on a single screen, and is almost narrow-enough for StackExchange code blocks.

Right, now, what are you doing? You're iterating over the inventoryArray, and, for each inventoryItem, you check to see whether there's a baseDataArray member who's filtered name is part of the filtered inventoryItem's name.

You extract the processing of the baseDataArray names to a separate loop to prevent recalculating it often. That's a good idea. It would be better if you reused a single compiled Pattern:

Pattern charfilter = Pattern.compile("[" + Pattern.quote("_-") + "]");

Then, use that pattern in the loop, replacing the following

for (Map<String, String> baseDataItem : baseDataArray) {
    productNameArray.add(baseDataItem.get("product_name")
      .replaceAll(charsToFilter, " ").toLowerCase());
}

with:

for (Map<String, String> baseDataItem : baseDataArray) {
    productNameArray.add(charfilter.matcher(baseDataItem.get("product_name"))
      .replaceAll(" ").toLowerCase());
}

Similarly, inside the inventory loop, you can also do:

String processedInventoryItem = charfilter.matcher(inventoryItem.get("title")).replaceAll(" ").toLowerCase();

That should reuse the pattern more efficiently, and improve the replaceall time (though Java probably does a good job of detecting that misuse, and compiling it efficiently anyway.

There is a potential bug in your code, though, and it possibly accounts for the speed-up too.

In the original code, you check every inventory item against every product.

In the second code, you check each product against only the first inventory. You "break" the inner loop.

As a result, your "faster" code does not do a full join between the data sets, but only a partial one. This may or may not impact your results, but it will impact the performance.

As an aside, this code would look great in Java 8 with streams... do you have Java 8 available?


Simple trial division in JavaScript I began programming recently. I am trying to implement a simple trial-division algorithm for finding all primes up to some number (it is much more "primitive" algorithm than the Sieve of Eratosthenes). Can you please find what's wrong with my code?

#range function:

range = function (a,b,c){
    var range1=[]
    for (i=a; i<b; i=i+c){
        range1.push(i);
    }
    return range1;
}

#The algorithm:

n=prompt("n");
var numbers=range(2,n,1);
var primes=[];
for (number in numbers){
 var sublist=range(2,number,1);
 console.log(sublist);
 for (x in sublist){
  if (number%x ===0){
   break;
  }
 primes.push(number);
 }
}

Thanks in advance!

Try using this instead.

// `isPrime` adopted from http://www.javascripter.net/faq/numberisprime.htm
var isPrime = function (n) {
    if (isNaN(n) || !isFinite(n) || n % 1 || n < 2) {
        return false;
    }
    if (n % 2 === 0){
        return (n === 2);
    }
    if (n % 3 === 0){
        return (n === 3);
    }
    for (var i = 5, m = Math.sqrt(n); i <= m; i += 6) {
        if ((n % i === 0) || (n % (i + 2) === 0)){
            return false;
        }
    }
    return true;
}
var getPrimesUntilN = function (n) {
    n = Math.abs(n);
    var primes = (1 < n) ? [2] : [];
    if (isNaN(n) || !isFinite(n)) {
        return primes;
    }
    for (var i = 3; i <= n; i+=2) {
        if (isPrime(i)) {
            primes.push(i);
        }
    }
    return primes;
};

Input:

getPrimesUntilN(50);

Output:

[2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47]


Starting MVC PHP - am I doing it right? Im starting with MVC and I'd like you to see my code. Am I doing right? What can I improve in my classes?

Controller

class Logar extends Controller {

    private $view;
    private $modelDAO;

    function __construct() {
        $this->modelDAO = new LogarModel();
        $this->view = new LogarView();
    }

    public function Login() {
        if ($this->modelDAO->Login($_POST['funcionario'], $_POST['senha'])) {
            $idFuncionario = $this->modelDAO->rows['idFuncionario'];
            parent::redirect("view/funcionario/index.php");
        } else {
            $this->view->show("Funcionario nao cadastrado");
        }
    }

}

class controller {

    function redirect($url){
        header("location: {$url}");
    }
}

Model

class LogarModel extends Model {

    function __construct() {
        $this->conectar();
    }

    function Login($funcionario, $senha) {
        $sql = "select * from funcionario where login = ? and senha = ? ";
        $this->read($sql);
        $this->bindar(1, $funcionario, PDO::PARAM_STR);
        $this->bindar(2, $senha, PDO::PARAM_STR);
        $this->executar();
        $this->contar();
        $this->pegar_dados();
        $this->desconectar();
        if ($this->linhas == 1) {
            return true;
        } else {
            return false;
        }
    }
}

class model {

    function conectar(){
        $this->conn = new Connection();
    }
    function read($readQuery){
       $this->stm =  $this->conn-> prepare($readQuery);
    }
    function bindar($n,$var,$extensao){
        $this->stm->bindparam($n,$var,$extensao);
    }
    function contar(){
        $this->linhas = $this->stm->rowcount();
    }
    function pegar_dados(){
        while($this->rows = $this->stm->fetch()){
            return $this->rows;
        }
    }
    function executar(){
        $this->stm->execute();
    }
    function desconectar(){
        $this->conn = null;
    }
}

View

class LogarView extends view {

    function __construct(){
    }
    function mostrarlogado(){

    }
}

class view {

    function view(){

    }

    function get_var($var,$value){
        $this->var = $value;
    }

    function show($value){
        $this->value = $value;
        echo $this->value;
    }
    function alertar($value){
        echo "<script>alert('{$value}')</script>";
    }
}

So, how am I going?

Yes, you understood how MVC works, and you are doing it right. The helper functions such as redirect() are in the correct classes. A few comments on the code itself:

I'd recommend to add an exit; statement after sending headers in redirect(). It will be easier to understand the code, since you can now think of the redirect() as a return, which will help you avoid mistakes.
Don't store your password in plain text.
Symply use return $this->linhas == 1; at the end of your login function.
Your model class simply translates the PDO interface into Portuguese, this is not an interesting layer of abstraction in my book. :)
Declare the variables you use, eg. $this->stm or $this->conn.
You are probably going to want an easier way to make requests when you will have dozens of them, but right now it's OK.


Fill a collection from a database with all values filled I have some data in a database, organized like (time, category, value), and basically, I want to show it in a histogram. But the library I use to draw historgrams only works properly if every value is filled.

For instance, this does not work:

bin category value
1   A        5
2   B        3

But this is ok:

bin category value
1   A        5
2   A        0
1   B        0
2   B        3

So I have to insert zeros for every time/category combination that are not in the database. I came up with the following code:

List<Info> allCategories = GetAllCategories(cnn); //select distinct ...
var data = new List<Info>();
using (var cmd = cnn.CreateCommand())
{
    cmd.CommandText = sqlSelect;
    while (blah blah)
    {
        [setting command parameters]

        //prepare a copy of reference values
        var valuesToAdd = new List<Info>(allCategories.Count);
        foreach (var x in allCategories)
            valuesToAdd.Add(x.Clone());
        int bin = [get bin]
        foreach (var info in valuesToAdd)
            info.Bin = bin;

        using (var reader = cmd.ExecuteReader())
        while (reader.Read())
        {
            int category = reader.GetInt32(1);
            Info info = valuesToAdd.Find(i => i.category == category); //get the the object to modify
            info.value = reader.GetInt64(2);
        }
        data.AddRange(valuesToAdd);
    }
}
return data;

So, basically I make a copy of the list of possible combination for each bin of the graph, and then I update the values found in the database.

I'm not really fond of this solution, partly because it involves a .Clone, and I'm looking for a more elegant solution...

Have you considered linq? Something like:

        var j = from i in new[] {1, 2}
                from s in new[] {"a", "b"}
                select new {i, s};

or in method syntax:

        var j = new[] {1, 2}.SelectMany(i => new[] {"a", "b"}, (i, s) => new {i, s});


Printing the power set of a given set of elements Input: Set of elements (String)
Output: Printing the elements of the power set of the given set

public class PrintPowerSet {
public static List<String> permuteSet(String inputSet){

    List<String> powerSet = new ArrayList<String>();
    int n = inputSet.length(); //size for binary

    for(int i=0;i<Math.pow(2,n);i++){
        if(i==0){
            powerSet.add("Ø"); //empty set Case
            continue;
        }
        String binaryOfIndex = getBinaryString(i, n);
        StringBuffer subset = new StringBuffer();
        for(int j=0;j<binaryOfIndex.length();j++){
            if(binaryOfIndex.charAt(j)=='1'){
                subset.append(inputSet.charAt(j));
            }
        }
        powerSet.add(subset.toString());
    }
    return powerSet;
}

public static String getBinaryString(int i, int size) {
    String bin = Integer.toBinaryString(i);
    bin = String.format("%0" + size + "d", Integer.parseInt(bin));
    return bin;
}
public static void main(String args[]){
    String set = "gle";
    List<String> result;
    result = permuteSet(set);
    for(String word: result){
        System.out.print(word+" ");
    }
}
}

The integer to string to integer representation of string to string conversion is very wasteful. You can just do bit shifts.

private static String getSubset(String inputSet, int mask) {
    StringBuffer subset = new StringBuffer();

    for (int i = 0; i < inputSet.length(); i++) {
        // Check if ith bit is set
        if ((mask & (1 << i)) != 0) {
            subset.append(inputSet.charAt(i));
        }
    }

    return subset.toString();
}

public static List<String> permuteSet(String inputSet) {
    List<String> powerSet = new ArrayList<String>();
    int n = inputSet.length(); // size for binary
    int powerSetLength = 1 << n; // you should check that n is small enough

    for (int i = 0; i < powerSetLength; i++) {
        powerSet.add(getSubset(inputSet, i));
    }

    return powerSet;
}

Demo


Is my way for parser written properly? I have zones

public class Zone
{
     public string Caption { get; set; }
     public IList<Module> Contents { get; set; }
}

public class Zone1 : Zone
{
}
public class Zone2 : Zone
{
}
public class Zone3 : Zone
{
    public enum Direction
    {
        Up = 1,
        Down = 2,
        Left = 3,
        Right = 4
    }
    public Direction Direction { get; set; }
}

I have json string [{"Caption":"Tab1","IDs":[4,6]},{"Caption":"Tab2","IDs":[9]}] that corresponds to the regular zones like Zone1,Zone2

{"Caption":"Tab1","IDs":[4,6]} means that the name of this zone is Tab1 and it will contain Modules #4 and #6.

I have default parser that will create modules according to IDs(4,6 AND 9).

public interface IZoneParser<T>
{
    IList<T> Parse(string json);
}
public abstract class ZoneParserBase : IZoneParser<Zone>
{
    public virtual IList<Zone> Parse(string json)
    {
        IList<Zone> zones = new List<Zone>();
        try
        {
            IList<ZoneJson> parsedZones = json.FromJSON<IList<ZoneJson>>();

            foreach (var parsedZone in parsedZones)
            {
                var zone = new Zone
                {
                    Caption = parsedZone.Caption,
                    Contents = new List<Module>()
                };
                foreach (var id in parsedZone.IDs)
                {
                    var module = currentPage.CreateModule(id);
                    zone.Contents.Add(module);
                }
                zones.Add(zone);
            }

            return zones;
        }
        catch
        {
            return null;
        }
    }
}

So, in order to parse the above json I'll need some class like

[DataContract]
private class ZoneJson
{
     [DataMember]
     public string Caption { get; set; }
     [DataMember]
     public IList<int> IDs { get; set; }
}

If I'll need to parse Zone3 with json string [{"Caption":"Tab1","IDs":[4,6],"Direction":"1"},{"Caption":"Tab2","IDs":[9]},"Direction":"3"], so I'll create

public class ZoneParserA : ZoneParserBase
{
   publice override IList<Zone3> Parse(string json)
   {
       ....
       IList<Zone3Json> parsedZones = json.FromJSON<IList<Zone3Json>>();

       foreach (var parsedZone in parsedZones)
       {
           var zone = new Zone3
           {
                Caption = parsedZone.Caption,
                Direction = parsedZone.Direction,
                Contents = new List<Module>()
           };
           foreach (var id in parsedZone.IDs)
           {
                var module = currentPage.CreateModule(id);
                zone.Contents.Add(module);
           }
           zones.Add(zone);
       }
       ....
   }
}

and

[DataContract]
[KnownType(typeof(Direction))]
public class Zone3Json : ZoneJson
{
    public Direction Direction { get; set; }
}

So, for each type of Zone(Zone,Zone1,Zone2,Zone3...) I'll need some kind Zone(X)Json that will differ from Zone(X) just with IList<int> IDs property. I can add this property to class Zone, but I don't want that anybody could access it from outside.

In addition, the functionality of method Parse is more or less the same for ZoneParserA and ZoneParserBase.

Is there way to avoid Zone(X)Json classes and/or maybe there are any suggestions to rewriting ZoneParsers?

I hope I explained myself properly.

Thank you

I can add this property to class Zone, but I don't want that anybody
  could access it from outside.

Which 'access' do you have in mind? through JSON (send you a 'fake' direction for zone1,2) or through code?

Either way, I don't think that by not adding it to the Zone class you will improve the security of your solution?

How do you decide on which parser to use when you receive a json string? BTW, your second json string looks wrong to me (the last direction param)

  [{"Caption":"Tab1","IDs":[4,6],"Direction":"1"},{"Caption":"Tab2","IDs":[9]},"Direction":"3"]

Why do you need Zone1 and Zone2? They're identical so why not simply use Zone? Even your ZoneBaseParser creates Zone objects and has no notion of Zone1 or Zone2.

Maybe you have a predefined set of zones (up to 4) and you need an index. Add an index property, if that's the case.

Your question is interesting but, instead of implementing a solution to the current problem (which would involve reflection), I'd focus on rethinking the json format and the classes and remove the problem altogether.

Quick and easy:

[DataContract]
private class ZoneJson
{
     [DataMember]
     public int ZoneType { get; set; }//could use an enum instead of int
     [DataMember]
     public int ZoneIndex { get; set; }//if a zone index is needed
     [DataMember]
     public string Caption { get; set; }
     [DataMember]
     public IList<int> IDs { get; set; }
     [DataMember]
     public Direction Direction { get; set; }
}

Or, a more generic approach:

[DataContract]
private class ZoneJson
{
     [DataMember]
     public int ZoneType { get; set; }//could use an enum instead of int
     [DataMember]
     public int ZoneIndex { get; set; }//if a zone index is needed
     [DataMember]
     public string Caption { get; set; }
     [DataMember]
     public IList<int> IDs { get; set; }

     //a json string containing additional params
     //might be set for certain ZoneTypes and it's contents can vary, based on ZoneType
     [DataMember]
     public string JsonParams { get; set; }
}

Next, you can add a Zone3Params class to implement the storage for Zone3 params (direction):

[DataContract]
[KnownType(typeof(Direction))]
public class Zone3ParamsJson : ZoneParamsBase
{
    [DataMember]
    public Direction Direction { get; set; }
}

and parse any defined zone type like this:

public class ZoneParserBase : IZoneParser<Zone>
{
    public virtual IList<Zone> Parse(string json)
    {
        IList<Zone> zones = new List<Zone>();
        try
        {
            IList<ZoneJson> parsedZones = json.FromJSON<IList<ZoneJson>>();

            foreach (var parsedZone in parsedZones)
            {
                var zone = new Zone
                {
                    //don't forget the index and type
                    ZoneType = parsedZone.ZoneType,
                    Caption = parsedZone.Caption,
                    Contents = new List<Module>()
                };
                foreach (var id in parsedZone.IDs)
                {
                    var module = currentPage.CreateModule(id);
                    zone.Contents.Add(module);
                }
                //parseParams
                if(zone.ZoneType==3)
                {
                    var params = parsedZone.JsonParams.FromJSON<Zone3ParamsJson>();
                    zone.Params = params;//zone.Params would be something like ZoneParamsBase
                }
                zones.Add(zone);
            }

            return zones;
        }
        catch
        {
            return null;
        }
    }
}


Implementing a fast 'split' function on a stream of characters in Java I was writing a piece of code (a custom EntityProcessor for Solr, but that isn't too relevant) designed to break lines of input based on a string delimiter (toMatch below).

Characters are read from a stream and then passed to addChar.
The assumption is that each character comes in a stream and look-ahead isn't allowed - the break needs to happen after the last character of a match is passed in.

I wrote two implementations below addChar and addChar2.
The first is a simple, dumb, keep a buffer and compare it each iteration, the second is a simple state machine and the latter is slightly faster.

import java.util.Iterator;
import java.util.LinkedList;

public class CharStreamMatcher {
    protected char[] toMatch = null;

    public MagicCharStreamMatcher(char[] match){
        toMatch = match;
        for(int i=0;i<match.length;i++) li.add((char)0);
    }

    public void clear(){
        correspondences.clear();
    }

    LinkedList<Character> li = new LinkedList<Character>();
    public boolean addChar2(char c){
        li.addLast(c);
        li.removeFirst();
        int i=0;
        for(Character ch : li){
            if(ch.charValue() != toMatch[i++])
                return false;
        }
        return true;
    }

    private class MutableInteger{
        public int i;
        public MutableInteger(int i){
            this.i = i;
        }
    }

    LinkedList<MutableInteger> correspondences = new LinkedList<MutableInteger>();
    public boolean addChar(char c){
        boolean result = false;

        if(c == toMatch[0])
            correspondences.add(new MutableInteger(-1));

        Iterator<MutableInteger> it = correspondences.iterator();
        while(it.hasNext()){
            MutableInteger mi = it.next();
            mi.i++;

            // check the match
            if(c != toMatch[mi.i]){
                it.remove();
            }
            // are we done?
            else if(mi.i == toMatch.length-1){
                result = true;
                it.remove();
            }
        }

        return result;
    }
}

There are more-sophisticated algorithms you can use, depending on the input and the search string. Even keeping the "one character at a time" interface though, there are optimizations to the implementation. Someone linked to KMP in a comment to your question - that's a good way to go if you can do some initial setup and buffer an arbitrary amount of the incoming stream.

I think you're on the right track with a state machine, but the implementation in addChar2 looks kind of heavyweight, with the MutableInteger class and a linked list, and an iterator-driven loop, and all. If you want this to be fast, you'll likely want to use primitive types (ints and arrays of ints) rather than OO wrappers around the primitive types.


My second requests script for a wargame This is a addition to my previous question earlier today.

I've taken the suggested changes into account and have written a solution to Natas7, a problem that requires some more 'meat' on what I'd consider quite a barebone solution in the previous question.

As with the previous question I'm more keen on knowing if I'm following good coding standard and if there are any potential performance issues.

Here is the script:

import requests, re
from requests.auth import HTTPBasicAuth

HOST = 'http://natas7.natas.labs.overthewire.org/index.php?page=about'
KEYWORD = 'natas8'
PATTERN = re.compile("^([A-Za-z0-9]{32})$")

def get_path(host, credentials):
    """
    This will serve up the path to the password
    using some basic scraping techniques
    :param host:
    :param credentials:
    :return: The path to the password
    """

    try:
        response = requests.get(host, auth=HTTPBasicAuth(*credentials))
        response_lines = response.content.split('\n')
        return next((line.split()[-2:][0] for line in response_lines if 'hint'in line),
                    "Couldn't find hint...")
    except requests.RequestException as e:
        print(e)
def exploit(host, credentials):
    """
    The actual exploit. This will attempt
    to find a string that matches the global
    regex pattern PATTERN.
    :param host:
    :param credentials:
    :return: The password, if one is found
    """

    try:
        response = requests.get(host, auth=HTTPBasicAuth(*credentials))
        response_lines = response.content.decode('ascii').split('\n')
        return next((line for line in response_lines if PATTERN.match(line)),
                    "Couldn't find the password...")

    except requests.RequestException as e:
        print(e)

def main():
    global HOST
    credentials = ('natas7', '7z3hEENjQtflzgnT29q7wAvMNfZdh0i9')
    path = get_path(HOST, credentials)
    HOST = (HOST.split('='))
    HOST = HOST[0] + '=' + path
    print(KEYWORD + ":" + exploit(HOST, credentials))

if __name__ == '__main__':
    main()

Just a few points of advice:

avoid using globals. The reason they are bad is that they allow functions to have hidden (as in non-obvious and undeclared) and thus hard to understand side effects. Also, this can lead to spaghetti code.
try to use str.format when you print out strings:

print(KEYWORD + ":" + exploit(HOST, credentials))

should be:

print('{}:{}'.format(KEYWORD, exploit(HOST, credentials)))

your docstrings are partially good. I'm saying partially because telling me things that I already know isn't helping. For example, :param host: or :param credentials:. I already know these are parameters to that function. You could at least specify the data structure they denote.
consider rewriting this:

(line for line in response_lines if PATTERN.match(line))

like this:

(line
for line in response_lines
if PATTERN.match(line))

The reason I like writing it like this is that it allows you to see more easily that you've changed you're code (on a svn / git repo).


Populating a Data Grid View in MVP I'm developing a win forms application in MVP pattern. In the application, there is a small module to show the branch offices (called points) where the attendance entering process has been completed. Those details should be shown in a data grid view.

So I have a form (View), a presenter and a data service classes. For the simplicity I'm not using a "model class" for this as the only intended functionality is populating the grid view.

Form frmAttendancePoints and DataService class have implemented  IAttendancePointsView and IDataService interfaces respectively.

My question: Is the way I'm populating the grid correct? If not could you please let me know a better solution.

View

public partial class frmAttendancePoints : Form, IAttendancePointsView
{
        private void frmEnterAttendance_Load(object sender, EventArgs e)
        {
            ShowAttendancePoints(sender, e);
        }

        public void LoadAttendacePointsGridWithData(DataTable dt)
        {
            dgAttendancePoints.DataSource = dt;
        }

        public event EventHandler ShowAttendancePoints;
}

Presenter

class AttendancePointPresenter : BasePresenter
{
    IAttendancePointsView _View;
    IDataService _ds;

    public AttendancePointPresenter(IAttendancePointsView View, IDataService ds)
    {
        _View = View;
        _ds = ds;
        WireUpViewEvents();
    }

    private void WireUpViewEvents()
    {
        this._View.ShowAttendancePoints +=new EventHandler(_View_ShowAttendancePoints);
    }

    private void _View_ShowAttendancePoints(object sender, EventArgs e)
    {
        ShowAttendancePoints();
    }

    private void ShowAttendancePoints()
    {
        var dt = _ds.GetAttendancePoints();
        this._View.LoadAttendacePointsGridWithData(dt);
    }

    public void Show()
    {
        _View.ShowDialog();
    }
}

DataService

public class DataServie
{
        public DataTable GetAttendancePoints()
        {
            string selectStatement = "SELECT PID, point_name + ', ' + point_add AS point, area, sm, atten_status FROM Point WHERE Status='Active'";
            using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
            {
                using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
                {
                    using (SqlDataAdapter da = new SqlDataAdapter(sqlCommand))
                    {
                        using (DataSet ds = new DataSet())
                        {
                            using (DataTable dt = new DataTable())
                            {
                                ds.Tables.Add(dt);
                                sqlConnection.Open();
                                da.Fill(dt);
                                return dt;
                            }
                        }
                    }
                }
            }
        }
}

Programme

    IAttendancePointsView AttendancePointView = new frmAttendancePoints();
    IDataService ds = new DataService();

    var Presenter = new AttendancePointPresenter(AttendancePointView, ds);
    Presenter.Show();

For the simplicity I'm not using a "model class" for this [...]

Then it's not Model-View-Presenter you have here.

var Presenter = new AttendancePointPresenter(AttendancePointView, ds);

I love it. What you've done here, is called Dependency Injection - more specifically, constructor injection. It's a technique that, when applied consistently, greatly contributes to making your code more cohesive and less coupled.

I think AttendancePointView being a local variable, should be named attendancePointView, or simply view - same for Presenter, it's a local variable, should be presenter; and ds could simply be called service, so the presenter code would read:

var presenter = new AttendancePointPresenter(view, service);
presenter.Show();

Which is awesome! Well done!

The only thing is that in Program.cs it doesn't matter that the view and the service are injected as interfaces, since Program is coupled with frmAttendancePoints and DataService anyway. I'd write it like this:

var view = new frmAttendancePoints();
var service = new DataService();

var presenter = new AttendancePointPresenter(view, service);
presenter.Show();

The presenter still receives its dependencies as abstractions, per its constructor.

  View

You're declaring an event that looks more like a method:

public event EventHandler ShowAttendancePoints;

Seeing that it's raised when the form gets loaded...

private void frmEnterAttendance_Load(object sender, EventArgs e)
{
    ShowAttendancePoints(sender, e);
}

That event does nothing that Load doesn't do. I think you should have an IView interface that exposes that Load event, and everything a presenter needs to know about any view:

public interface IView
{
    event EventHandler Load;
    void ShowDialog();
    void BindModel(IModel model);
}

Now the form's code can look like this*:

public partial class frmAttendancePoints : Form, IView
{
    public frmAttendancePoints()
    {
        InitializeComponents();
    }

    public void BindModel(IModel model)
    {
        dgAttendancePoints.DataSource = model;
    }
}

  DataService

This class should spit out your model, not a boilerplate-level DataTable.

One thing though, is that you can stack the using blocks for fewer levels of indentation:

using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
using (SqlDataAdapter da = new SqlDataAdapter(sqlCommand))
using (DataSet ds = new DataSet())
using (DataTable dt = new DataTable())
{
    ds.Tables.Add(dt);
    sqlConnection.Open();
    da.Fill(dt);
    return dt;
}

*This might need a bit of work, I'm not sure a WinForms DataGridView.DataSource can be assigned just like that, but you get the idea ;)


Bash script creator app I am a beginner and have created this bash script that helps me with creating bash scripts.

The script shows no issues on shellcheck.net

The code can be found here. https://github.com/plutesci/Iwanttobash/blob/master/Icode.bash

I would like some feedback from more experienced users. Any suggestions or thoughts about this.

#!/bin/bash
# This Program is to Show user the commands and or to create a bash script
# to execute this program type at terminal ./ICode.bash
# I am attempting to have the program open the named file with the header
# shebang at the top #!/bin/bash as it is not there you will need to add  it.
# created by plutesci for self or public use.

clear
# Banner shows a banner to install should it not show on Ubuntu is.
# sudo apt-get install sysvbanner
# banner "Icode"
# figlet is a banner aswell to install that just type in your terminal
# sudo apt-get install figlet
figlet "Icode"
echo Showing a way to use the command line steps to start a Hello World   program
echo Where you see test.bash, Please rename test.bash
echo Follow Steps Below...
echo ####################
echo "echo '#!/bin/bash' > test.bash " #change test
echo "'echo Hello World' >> test.bash " # change test.bash
echo "chmod 755 test.bash"  # change test.bash
echo "nano test.bash"   # change test.bash
echo "Exit Ctrl c"
echo ###################
#echo '#!/bin/bash' >> "$text.bash"

# I want to add an option to ask user if they want to start a new bash \
# program yes \ no?
# if yes\Yes query user for new name, What to call this bash ? ......
# I am happy with the the selection part but would like it going into a      menu
# afterwards with press 9 return to previous menu

######## Things to improve on.

# when playing i found a program called openvt, It would be great to have the
# bash program open virtual termin and run the output
echo -n "Please Enter a Filename For Your New Bash Script > "
read -r text
echo "Your New Bash Script is named: $text"

# chmod +x $text.bash

# I need to figure out how to add .bash to the $text varable

selection=
until [ "$selection" = "0" ]; do
echo "
ICODE MENU
1 - Start a new Bash Script
2 - Make it Executable
3 - Run Bash Program
4 - Continue working on script
5 - Start python idle
6 - Open a new shell
7 - Create Automatic Shell scripts
8 - Delete the script
9 - Help
0 - Exit Program
"
    echo -n "Enter selection: "
    read -r selection
    echo ""
    case $selection in
    1 ) echo "#!/bin/bash" > "$text.bash" ; nano "$text.bash"  ;;
    2 ) chmod 755 "$text.bash" ;;
    3 ) gnome-terminal ; echo "./$text.bash" ;;
    4 ) nano "$text.bash" ;;
    5 ) idle ;;
    6 ) gnome-terminal ;;
    7 ) nano ;; # would be something like grep a special crafted document
    8 ) echo "#!/bin/bash" > "$text.bash" ;;
    9 ) cat icodehelp ;;
    0 ) exit ;;
    * ) echo "Please enter 1, 2, 3, 4, 5, 6, 7, 8, 9, or 0"
    esac
done

# Things I would like to add to the program would be a auto statment maker
# and auto create bash programs like open the randomly grep for certain
# keywords, and random cut from a libary of scripts 10 - 100 scripts
# run it in a virtual terminal, looking for errors and print out example
# 3 out of 100  run with no errors, would you like to view these?
# 17 out of 100 return a slight error, would you like to inspec these.
# 83% percent should be removed. something like that just visualising it   out

# My first bash application would like any help, still trying to learn how
# github works

Good things

lots of comments are good.
clear keeps things clean when you start
This is an interesting way to learn shell scripting.  My friends would call it "very meta" which is high praise indeed.
I usually tell people they should have run their code through https://www.shellcheck.net/ before worrying about a review.  Amazingly enough you didn't trip any of the checks over there so: try it on your next shell project.  Or read their docs to see what to avoid in the future.

Suggestions

In your lines echo #################### all of the hashes (####################) are completely superfluous.  As you have it or simply writing echo will do the same thing.  Just reading the code it implies that the hashes are getting output, which they're not since the shell views it as one big comment.  You could put the hashes in quotes and then the shell would not "remove" it from the output.
Indenting most of the stuff inside the until is good.  It'd be nice to do the same thing for the case statement.  Your compact layout actually works in this case so I wouldn't mind seeing:

case $selection in
    1 ) echo "#!/bin/bash" > "$text.bash" ; nano "$text.bash"  ;;
    2 ) chmod 755 "$text.bash" ;;
    3 ) gnome-terminal ; echo "./$text.bash" ;;
    4 ) nano "$text.bash" ;;
    5 ) idle ;;
    6 ) gnome-terminal ;;
    7 ) nano ;; # would be something like grep a special crafted document
    8 ) echo "#!/bin/bash" > "$text.bash" ;;
    9 ) cat icodehelp ;;
    0 ) exit ;;
    * ) echo "Please enter 1, 2, 3, 4, 5, 6, 7, 8, 9, or 0"
esac

but it would look more like most people write shell scripts if you spaced it out like:

case $selection in
    1 )
        echo "#!/bin/bash" > "$text.bash"
        nano "$text.bash"
        ;;
    2 )
        chmod 755 "$text.bash"
        ;;
    3 )
        gnome-terminal
        echo "./$text.bash"
        ;;
    4 )
        nano "$text.bash"
        ;;
    5 )
        idle
        ;;
    6 )
        gnome-terminal
        ;;
    7 )
        nano
        ;; # would be something like grep a special crafted document
    8 )
        echo "#!/bin/bash" > "$text.bash"
        ;;
    9 )
        cat icodehelp
        ;;
    0 )
        exit
        ;;
    * )
        echo "Please enter 1, 2, 3, 4, 5, 6, 7, 8, 9, or 0"
esac

That's obviously take up three times as many lines, but if you decide to add more to any of the sections it will remain readable.
Much of your code is outputting blocks of text.  Rather than doing the echo after echo thing it is easier to scale this to multiple lines of text at once with a "Here Document".  To replace your block of echos after figlet you could do:

cat <<-EXAMPLE
Showing a way to use the command line steps to start a Hello World   program
Where you see test.bash, Please rename test.bash
Follow Steps Below...

echo '#!/bin/bash' > test.bash
'echo Hello World' >> test.bash
chmod 755 test.bash
nano test.bash
Exit Ctrl-c

EXAMPLE

This will do the same thing as what you already had, but it avoids needing to worrying about the quoting on each line.  It does remove the ability to comment within it, but usually that's a smaller issue than keeping straight whether you are in the text to be output or in scripting code.  By including the - in <<- we are saying to ignore initial tabs in the Heredoc content.  The tabs get removed on the fly and not exposed to the end user.  This lets you use it inside the until to indent the whole cat and keep everything looking neat.
You could avoid the need for banner or figlet by the users of your script if you took the output of them and saved it into your script, probably in a Heredoc.  Since you're not passing anything variable into them, their output won't change and so "caching" a static version won't reduce any existing functionality.

cat <<'ICODE'
 ___              _
|_ _|___ ___   __| | ___
 | |/ __/ _ \ / _` |/ _ \
 | | (_| (_) | (_| |  __/
|___\___\___/ \__,_|\___|
ICODE

In this case the Heredoc starts with single quotes (<<'ICODE') so that the contents are passed through without any values being replaced.
Your option 9 for help could be done without an external file.  Typically this would be done by defining a function and then calling it inside your case statement.  Inside the function could simply be a cat with a Heredoc.  But doing that means you don't have to worry about the help file getting lost.

Ideas

If you like figlet try cowsay!  :=)
Try reading the bash man page a few times.  It is long and arduous, but full of lots of useful info if you can make it.
https://kvz.io/blog/2013/11/21/bash-best-practices/ has some easier to digest suggestions.  Looking at your menu section I was really hoping to find a set -x so we could see the commands running too.  This may not fit your UX, but it is worth trying to see how tracing works.


TinCan (Esolang) Interpreter in Haskell Haskell newbie here. Wanted to improve my skills by re-implementing an esolang I designed back in 2011 called TinCan. If there are things that are perhaps badly coded or unclear things due to my inexperience with the language then I would love to know about them so I can move forward in my Haskell usage.

{-# LANGUAGE OverloadedStrings #-}

import qualified Data.Text as T
import Text.Read
import Data.Char
import Data.Map (Map)
import qualified Data.Map as Map

data Token = Dollar | At | Ampersand | Variable Char | Value Int deriving (Eq, Show)

data Program = Program { program :: [(Token, Token, Token)]
                       , len :: Int
                       , currentLine :: Int
                       , pc :: Int
                       , stack :: [Int]
                       , executionFinished :: Bool
                       , variables :: Map Char Int
                       } deriving (Show)

stripLines :: [String] -> [String]
stripLines = filter (\x -> ((==40) . length) x && (head x == '#') && (last x == '#'))

removeHash :: String -> String
removeHash = filter (/='#')

toToken :: String -> Token
toToken t = case readMaybe t :: Maybe Int of
    Just i -> Value i
    Nothing -> if (length t == 1 && (t !! 0) `elem` ['A'..'Z'])
               then Variable (t !! 0)
               else (case t of
                    "$" -> Dollar
                    "@" -> At
                    "&" -> Ampersand
                    _ -> error ("Could not parse '" ++ t ++ "'"))

clean :: T.Text -> Token
clean = toToken . T.unpack . T.strip

tokenize [x,y,z] = (clean x, clean y, clean z)
tokenize _ = error "Could not tokenize"

tokenizeLine :: String -> (Token, Token, Token)
tokenizeLine line = if (size == 3) then (tokenize split) else (error line)
    where size = length split
          split = T.splitOn "," (T.pack line)

getVariable :: Program -> Char -> Int
getVariable p = ((variables p) Map.!)

tokenToInt :: Program -> Token -> Int
tokenToInt p t = case t of
    Dollar -> currentLine p
    At -> pc p
    Ampersand -> currentLine p + 1
    Variable c -> getVariable p c
    Value i -> i

updateVariable :: Program -> Char -> Int -> Program
updateVariable p c i = p { variables = Map.insert c i (variables p) }

jump :: Int -> Program -> Program
jump i p
    | i >= len p || i < 0 = p { currentLine = i, executionFinished = True }
    | otherwise = p { currentLine = i }

push :: Int -> Program -> Program
push i p = p { stack = i : stack p }

executeLine :: Program -> (Token, Token, Token) -> Program
executeLine p (a,(Variable b),c) =
    if pushToStack
    then jump (currentLine p + 1) $ push diff $ updateVariable p b diff
    else if diff <= 0
         then jump address $ updateVariable p b diff
         else jump (currentLine p + 1) $ updateVariable p b diff
            where diff = value - differential
                  differential = tokenToInt p a
                  value = getVariable p b
                  pushToStack = address == -1
                  address = tokenToInt p c
executeLine _ _ = error "Could not execute line."

executeProgram :: Program -> Program
executeProgram p = executeLine p line
    where line = program p !! currentLine p

generateProgram :: [String] -> Program
generateProgram xxs = Program (map tokenizeLine xxs) (length xxs) 0 0 [] False (Map.fromList $ zip ['A'..'Z'] (repeat 0))

main = readFile "countdown.txt" >>= putStrLn . reverse . map chr . stack . until executionFinished executeProgram . generateProgram . map removeHash . stripLines . lines

I'd rewrite toToken a slight bit:

toToken :: String -> Token
toToken [t]
   | isAsciiUpper t = Variable t
   | t == '$' = Dollar
   | t == '@' = At
   | t == '&' = Ampersand
toToken str = Value $ read str

Notice how I'm handling the single character string cases first and that this is much less indented. I've also omitted the explicit error message you have and eliminated the explicit type-signature.

This saves on vertical indentation and just reads a bit cleaner to me.

let's have a look at executeLine

executeLine :: Program -> (Token, Token, Token) -> Program
executeLine p (a,(Variable b), c)
    | pushToStack = jump (currentLine p + 1) $ push diff $ updateVariable p b diff
    | (diff <= 0) = jump address $ updateVariable p b diff
    | otherwise = jump (currentLine p + 1) $ updateVariable p b diff
    where
        diff = value - differential
        differential = tokenToInt p a
        value = getVariable p b
        pushToStack = address == -1
        address = tokenToInt p c
executeLine _ _ = error "Could not execute line."

This again removes indentation reads a bit cleaner to me.

Let's also add some intermediate variables to main, just to clarify what happens:

main = do
    code <- readFile "countdown.txt"
    let program = (generateProgram . map removeHash . stripLines . lines) code
    let result = (reverse . map chr . stack . until executionFinished executeProgram) program
    putStrLn result

While we're at adding variables, let's also look at names. The functions are generally named really well, that's a little less the case for their arguments. Especially for executeLine we can improve the names from a, b, and c. Let's instead use the terminology from the language itself:

executeLine program (differential, (Variable storage), adress)

Names can really help with understanding a program and choosing good names is really hard. On that note pc feels somewhat misleading. I think something like executedInstructions could be more helpful :)